-
Notifications
You must be signed in to change notification settings - Fork 233
fix(pending-state): correct reflection of aria in pending controller #5730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
// Restore the cached `aria-label` if it exists | ||
if (this.cachedAriaLabel) { | ||
this.host.setAttribute('aria-label', this.cachedAriaLabel); | ||
} else if (!pending) { | ||
// If no cached `aria-label` and not `pending`, remove the `aria-label` | ||
} else { | ||
this.host.removeAttribute('aria-label'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are now always removing aria-label
If there is no cached label. Do you intend this to be removed even if it was never set? Maybe check for if this exists first?
// If the current `aria-label` is different from the pending label, cache it | ||
// or if the cached `aria-label` is different from the current `aria-label`, cache it | ||
if ( | ||
(!this.cachedAriaLabel && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this.cachedAriaLabel is undefined, isn't the second branch (this.cachedAriaLabel !== currentAriaLabel) always true? Also this is quite complex to read. Can you abstract these into a helper function, added a small snippet below if you want to take reference. Just making sure the main method is more readable
if (shouldCacheAriaLabel(this.cachedAriaLabel, currentAriaLabel, pendingLabel)) {
this.cachedAriaLabel = currentAriaLabel;
}
...
function shouldCacheAriaLabel(cached, current, pending) {
return (
(!cached && current && current !== pending) ||
(cached !== current && current && current !== pending)
);
}
Description
This PR resolves a small accessibility bug related to caching aria labels when pending state controllers are used on components. The controller will now cache aria labels before updating as well as re-cache if there was a change to the aria label since the last pending state. This also solves a bug where the aria label was being removed entirely, even if an aria label was present before pending.
Motivation and context
correctly toggles aria-labels based on pending state
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review