Skip to content
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-experimental/mdc-progress-spinner): fix aria-valuenow #22429

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

annieyw
Copy link
Contributor

@annieyw annieyw commented Apr 8, 2021

Previously the aria-valuenow on mat-progress-spinner was being set through the mdc foundation which deals with values from 0 to 1. This caused the aria-valuenow to be incorrect (eg. 0.6 instead of 60).

Solution: Use mat-progress-spinner as a wrapper element instead and have the different circle containers under a separate mdc-circular-progress div instead of right under mat-progress-spinner. This way the mdc foundation can interact with the underlying mdc-circular-progress div instead of the overall mat-progress-spinner selector.

@annieyw annieyw added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent Accessibility This issue is related to accessibility (a11y) target: patch This PR is targeted for the next patch release labels Apr 8, 2021
@annieyw annieyw requested review from jelbourn and crisbeto April 8, 2021 00:03
@annieyw annieyw requested a review from andrewseguin as a code owner April 8, 2021 00:03
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 8, 2021
setAttribute: (name: string, value: string) =>
this._elementRef.nativeElement.setAttribute(name, value),
this._rootElement.setAttribute(name, value),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having to introduce another layer in the DOM and move styles around, we could change this method so that it doesn't set the aria-valuenow. E.g.

setAttribute: (name, value) => {
  if (name !== 'aria-valuenow') {
    this._elementRef.nativeElement.setAttribute(name, value)
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, that would be a better approach here. Separately, though, we should ask mdc why they use 0 to 1 instead of 0 to 100 (as percent).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also do the same for mdc progress bar?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it has the same issue then yes. Both have the same accessibility setup.

@annieyw annieyw force-pushed the valuenow branch 2 times, most recently from 8a6d178 to 5c0e11a Compare April 8, 2021 17:27
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

setAttribute: (name: string, value: string) =>
this._elementRef.nativeElement.setAttribute(name, value),
setAttribute: (name, value) => {
if (name !== 'aria-valuenow') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe we should have a comment here explaining why we're special-casing it? Also do we have to account for aria-valuemin and aria-valuemax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-valuenmin and aria-valuemax are static and set in the host

@annieyw annieyw added the action: merge The PR is ready for merge by the caretaker label Apr 9, 2021
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mmalerba mmalerba merged commit 768534d into angular:master Apr 13, 2021
mmalerba pushed a commit that referenced this pull request Apr 13, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 14, 2021
@annieyw annieyw deleted the valuenow branch May 14, 2021 16:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants