fix(meter, progress-bar, progress-circle): use innerText when label is not provided #3483
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
Rajdeepc
requested changes
Jul 21, 2023
| protected override render(): TemplateResult { | ||
| return html` | ||
| <sp-field-label size=${this.size} class="label"> | ||
| ${this.slotHasContent ? html`` : this.label} |
There was a problem hiding this comment.
why an html construct is needed?
|
Good Start!! Also please make sure the tests are updated accordingly if needed. |
Tachometer resultsChromeaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
avatar permalink
badge permalink
button-group permalink
button permalink
card permalink
checkbox permalink
color-slider permalink
color-wheel permalink
dialog permalink
field-label permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
Firefoxaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
avatar permalink
badge permalink
button-group permalink
button permalink
card permalink
checkbox permalink
color-slider permalink
color-wheel permalink
dialog permalink
field-label permalink
link permalink
menu permalink
meter permalink
number-field permalink
overlay permalink
picker permalink
popover permalink
progress-bar permalink
progress-circle permalink
radio permalink
search permalink
sidenav permalink
slider permalink
split-button permalink
swatch permalink
switch permalink
table permalink
tabs permalink
tags permalink
textfield permalink
toast permalink
tooltip permalink
top-nav permalink
tray permalink
|
Westbrook
changed the title
Westbrook
reviewed
Jul 21, 2023
There was a problem hiding this comment.
I made notes on ProgressCircle, but it looks like they apply to all three elements.
Also, this changes the reality of the Dev Mode warnings in the Progress* elements. Please review, update or remove, and if the message persists see if it is worth adopting in Meter.
| validSizes: ['s', 'm', 'l'], | ||
| }) { | ||
| export class ProgressCircle extends SizedMixin( | ||
| ObserveSlotText(SpectrumElement, ''), |
There was a problem hiding this comment.
In that Progress Circle doesn't show the label visibly, it might not need this feature addition. It's not "free" from a performance standpoint, so we should be sure that it is 100% needed.
Progress Bar may need it, as the label is visible there.
There was a problem hiding this comment.
what could be the other way of allowing the innerText to act as a label in case label is not provided without using a slot?
There was a problem hiding this comment.
ObserveSlotText will only let you know if there is text there or not in a reactive way. You do much the same in getLabelFromSlot right now.
1 task
Rajdeepc
reviewed
Jul 24, 2023
Westbrook
reviewed
Jul 24, 2023
added 3 commits
July 24, 2023 18:58
….com/adobe/spectrum-web-components into issue/3146-meter-progress-aria-label
|
@TarunAdobe If you see some build failing on one test suite. Please re run the workflow. It should pass eventually. |
Westbrook
reviewed
Jul 25, 2023
packages/meter/src/Meter.ts
Outdated
| @@ -83,6 +90,17 @@ export class Meter extends SizedMixin(ObserveSlotText(SpectrumElement, '')) { | |||
| `; | |||
| } | |||
|
|
|||
| protected onSlotChange(): void { | |||
There was a problem hiding this comment.
Suggested change
| protected onSlotChange(): void { | |
| protected handleSlotchange(): void { |
Across the board here, 🙏🏼.
This method being used in each of three components points to this possibly benefiting a Reactive Controller or mixin for reusability. Let's not worry about that today, but keep an eye out for this pattern in case we end up using it again somewhere else.
There was a problem hiding this comment.
We are using onSlotChange naming convention in multiple components already.... so this feels more consistent. for instance checkout -> ActiveOverlay.ts, Tabs.ts and TopNav.ts
There was a problem hiding this comment.
Thanks for digging in on normalization! That's a great trait, and something we should do more of as a project. However, we have not normalized here very well, see handleSlotchange usage... 😞
on* is a common practice in frameworks like React, and surfacing methods on our custom elements with a similar name can cause issues when working in that context without a wrapping React Component. We see this locally in our Storybook from time to time, and while we've recently started shipping React Wrappers to reduce the changes this happens for our consumers that leverage React, at some point in the future React will finally ship framework native support for custom events and setting properties on DOM elements and users will be freed to return to direct consumption of SWC in that context, so we want to be prepared for that.
There was a problem hiding this comment.
alright got it. I'll change it then
| 'https://opensource.adobe.com/spectrum-web-components/components/progress-circle/#accessibility', | ||
| { | ||
| type: 'accessibility', | ||
| issues: [ | ||
| 'value supplied to the "label" attribute, which will be displayed visually as part of the element, or', | ||
| 'value supplied to the "aria-label" attribute, which will only be provided to screen readers, or', | ||
| 'if the value is not supplied to "label" attribute and the "content" is also not set for the component, or', |
There was a problem hiding this comment.
Suggested change
| 'if the value is not supplied to "label" attribute and the "content" is also not set for the component, or', | |
| 'value supplied to the "label" attribute, which will be displayed visually as part of the element, or', | |
| 'text content supplied directly to the <sp-progress-circle> element, or', | |
| 'value supplied to the "aria-label" attribute, which will only be provided to screen readers, or', |
Westbrook
reviewed
Jul 25, 2023
Co-authored-by: Westbrook Johnson <wesjohns@adobe.com>
Rajdeepc
previously approved these changes
Jul 26, 2023
Westbrook
reviewed
Jul 26, 2023
There was a problem hiding this comment.
The changes here sags us below test coverage requirements: https://app.circleci.com/pipelines/github/adobe/spectrum-web-components/16799/workflows/e30e81dd-a6e1-47d5-b054-8a5415fcbe3a/jobs/366581?invite=true#step-108-2421 Please add or update some tests to ensure the stability of this change. See https://output.circle-artifacts.com/output/job/fe48a7c5-01cb-4294-a7d0-8f777099416d/artifacts/0/coverage/lcov-report/tools/shared/src/get-label-from-slot.ts.html for some uncovered contexts.
packages/meter/src/Meter.ts
Outdated
| @@ -83,6 +90,17 @@ export class Meter extends SizedMixin(ObserveSlotText(SpectrumElement, '')) { | |||
| `; | |||
| } | |||
|
|
|||
| protected onSlotChange(): void { | |||
| validSizes: ['s', 'm', 'l'], | ||
| }) { | ||
| export class ProgressCircle extends SizedMixin( | ||
| ObserveSlotText(SpectrumElement, ''), |
Rajdeepc
previously approved these changes
Jul 27, 2023
Westbrook
reviewed
Jul 27, 2023
| protected override firstUpdated(changes: PropertyValues): void { | ||
| super.firstUpdated(changes); | ||
| if (!this.hasAttribute('role')) { | ||
| this.setAttribute('role', 'progressbar'); | ||
| } | ||
| this.slotHasContent = this.slotEl.assignedNodes().length > 0; |
There was a problem hiding this comment.
This is the work that I was attempting to do in a lazy manner by removing the ObserveSlotText mixin from this class. Below in getLabelFromSlot you already do a check for this, so if you were to remove slotHasContent from the method signature there you would be able to do less work in this element unless it was needed, as signified by the lack of a label.
There was a problem hiding this comment.
so should I remove the slotHasContent all together from the method itself?
There was a problem hiding this comment.
If the functionality continues as expected without it, yes.
There was a problem hiding this comment.
So Im running into a small issue here... If I remove the slotHasContent Attribute then in the dev warnings the condition to give warning would be true if the label is not there. So let's say someone has provided some content in the slot but still that condition would be true for the first render call and then it would be false as the label would get updated from the slot content. But this also means the warning would still be there in console from the first call. That is why I added this variable to check if the slot has content then we don't show the warning even if the label is not there bcz we know eventually label would be updated with the slot content once the onSlotChange method is completed!
There was a problem hiding this comment.
Super helpful delineation!
ObserveSlotText will always do some work, whether you have content or not, have a label or not, have Dev Mode or not...
Any work within if (window.__swc.DEBUG) { ... } will only happen when Dev Mode is on. So if we replace !this.slotHasContent in line 154 with !this.slotEl.assignedNodes().length we'd only require using in that context to do that work.
There was a problem hiding this comment.
Got it... I'll fix it and make a commit now... Thank you so much for clarifying
…nt condition check
Westbrook
approved these changes
Jul 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use innerText when label is not provided (meter, progress-bar, progress-circle)
Description
To improve accessibility, the component should be updated to set the aria-label attribute based on the following conditions:
Related issue(s)
Motivation and context
Currently, the component sets the aria-label attribute only if the label attribute is provided. When only textContent is provided, the component doesn't set the aria-label, leading to an accessibility issue flagged by Axe DevTools.
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.