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: wc menu keyboarding and aria-expanded #3496

Merged
merged 6 commits into from
Jul 14, 2020

Conversation

scomea
Copy link
Collaborator

@scomea scomea commented Jul 10, 2020

Description

A timing issue was causing the menu to check the role attribute of children to determine focusability before those children had a chance to update their roles. Waiting to check on the value change callback resolved that. Also, we were always setting an "aria-expanded" attribute on menu items which would cause menu items to announce as not expanded even when they had no expanded state to begin with.

Motivation & context

partially fixes #3481

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.

Adding or modifying component(s) in @microsoft/fast-components checklist

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Comment on lines +39 to +40
@attr({ attribute: "expanded" })
public expanded: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be aria-expanded or AT will not recognize it as expanded. The attr should stay the same here. I think we you'll want is to keep the boolean attribute converter but change "reflect" to "fromView".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We write to the "aria-expanded" attribute in the template, just like we do for disabled:
aria-expanded="${x => x.expanded}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it's undefined we don't render a value, which fixes the bug

Copy link
Member

Choose a reason for hiding this comment

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

gotcha, thanks for the additional context.

@scomea scomea force-pushed the users/scomea/wc-menu-keys branch from f66985f to cbb26c0 Compare July 14, 2020 05:54
@scomea scomea force-pushed the users/scomea/wc-menu-keys branch from 06d3991 to 970422e Compare July 14, 2020 13:52
@chrisdholt chrisdholt merged commit 0530a42 into master Jul 14, 2020
@chrisdholt chrisdholt deleted the users/scomea/wc-menu-keys branch July 14, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu has erratic keyboard behavior, announced by AT as collapsed, can't check radio items
4 participants