-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(material/stepper): add text for screen readers to indicate when step is complete #23519
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
fix(material/stepper): add text for screen readers to indicate when step is complete #23519
Conversation
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.
LGTM, but need to update API goldens
5b1c00e
to
d7cf8c3
Compare
@@ -10,6 +10,10 @@ | |||
[ngTemplateOutletContext]="_getIconContext()"></ng-container> | |||
<ng-container *ngSwitchDefault [ngSwitch]="state"> | |||
<span *ngSwitchCase="'number'">{{_getDefaultTextForState(state)}}</span> | |||
<span class="cdk-visually-hidden" | |||
*ngIf="state == 'done' || state == 'edit'"> |
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.
Sorry for chim in, but wouldn't it be better to do it like this?
*ngIf="state == 'done' || state == 'edit'"> | |
*ngIf="state === 'done' || state === 'edit'"> |
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.
Shouldn't we be adding this text only when the state is done
? edit
means that the user is still editing it.
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.
Thanks @dev054!
@crisbeto @jelbourn I just updated this to have also separately have special text for "edit". I do mainly see the edit state so I figured it's worth having one too. Although your right its different than "done". Although I am not too sure if just saying editable makes sense. Thoughts?
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.
Maybe it should say something like "Current"? The only reason we have it as edit
is because that's what the icon is called.
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.
We should already be capturing which step is selected/active by applying aria-selected
to the role="tab"
element, no?
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.
src/material/stepper/stepper-intl.ts
Outdated
@@ -21,6 +21,9 @@ export class MatStepperIntl { | |||
|
|||
/** Label that is rendered below optional steps. */ | |||
optionalLabel: string = 'Optional'; | |||
|
|||
/** Label that is used to indicate step as completed to screen readers. */ | |||
completedScreenReaderLabel: string = 'Completed'; |
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.
This should just be completedLabel
(that fact that it's read by a screen-reader related to how it will be used rather than what it is)
d7cf8c3
to
fb41276
Compare
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.
We should also add unit tests that assert the correct labels/descriptions are present
<span class="cdk-visually-hidden" *ngIf="state === 'done'"> | ||
{{_intl.completedLabel}} | ||
</span> | ||
<span class="cdk-visually-hidden" *ngIf="state === 'edit'"> | ||
{{_intl.editableLabel}} | ||
</span> |
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.
One thing that also occurred to me: we should compare doing this (inline text) vs. aria-describedby
on the role="tab"
element and see if one is preferable.
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.
When I tried using the aria-describedby
attribute, nothing was announced when testing with ChromeVox so I just left it as is. I also added tests
…tep is complete or editable
fb41276
to
f605fcf
Compare
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.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.