This repository was archived by the owner on Sep 5, 2024. It is now read-only.
fix(nav-bar): improve screen reader support #11486
Merged
Conversation
This file contains hidden or 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
googlebot
added
the
cla: yes
|
The build failure looks like a fluke. I have restarted it. |
Splaktar
added
type: bug
a11y
|
It looks like the test failures are not a fluke as they keep happening and I can reproduce them on my local machine. Are you able to get all of the tests to pass on your local machine with |
Splaktar
added
the
in progress
Splaktar
suggested changes
Oct 16, 2018
There was a problem hiding this comment.
This looks good to me and passed manual testing on VoiceOver. Can you please update the API docs as mentioned below?
The test failures don't appear to be related to this PR. It appears that the tests in master are now broken after 12 days in which no commits occured... I'm still investigating if it has something to do with NodeJS 10.12.0 being released or some other dependency.
|
I've opened #11487 to track the test failures on |
Splaktar
reviewed
Oct 16, 2018
|
@Splaktar Thanks for the review! I also changed the documentation further up to point to the tablist and tab specifications instead of the outdated site navigator pattern. |
Splaktar
approved these changes
Oct 16, 2018
Splaktar
reviewed
Oct 16, 2018
Splaktar
reviewed
Oct 16, 2018
Splaktar
reviewed
Oct 16, 2018
Splaktar
reviewed
Oct 16, 2018
Splaktar
reviewed
Oct 16, 2018
|
I've tracked down the CI failure and the disconnection issue to this test. |
3 tasks
|
I've opened PR #11488 to fix the test issues here. Sorry that you had to run into them! |
Thank you for this, and for your patience and guidance with my PR! I think I've addressed all your comments and will push an updated revision in a minute. |
mmalerba
pushed a commit
that referenced
this pull request
Oct 24, 2018
…11488) <!-- Filling out this template is required! Do not delete it when submitting a Pull Request! Without this information, your Pull Request may be auto-closed. --> ## PR Checklist Please check that your PR fulfills the following requirements: - [x] The commit message follows [our guidelines](https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#-commit-message-format) - [x] Tests for the changes have been added or this is not a bug fix / enhancement - [x] Docs have been added, updated, or were not required ## PR Type What kind of change does this PR introduce? <!-- Please check the one that applies to this PR using "x". --> ``` [x] Bugfix [ ] Enhancement [ ] Documentation content changes [ ] Code style update (formatting, local variables) [ ] Refactoring (no functional changes, no api changes) [ ] Build related changes [ ] CI related changes [ ] Infrastructure changes [ ] Other... Please describe: ``` ## What is the current behavior? - A new `npm i` may pick up `jasmine-core@2.99.1` which will cause many test failures since we don't currently support that version. - Setting `md-nav-href=""` on an `md-nav-item` doesn't apply the proper `ng-href=""` because of a bad `if` check. - The docs indicate that a navigation attribute **must** be defined on each `md-nav-item`, but the code doesn't throw an exception if this occurs. This can lead to unexpected behaviors like infinite loops or hanging async tasks. - The Closure properties are incorrect in a number of cases. <!-- Please describe the current behavior that you are modifying and link to one or more relevant issues. --> Issue Number: Relates to #11485. Relates to #11486. Fixes #11487. ## What is the new behavior? - set ng-href on nav-item even if md-nav-href is empty - fix closure properties to better represent the existing code - pin the version of jasmine-core since we don't support 2.99.1 yet - slightly improve the tone of the exceptions - pull in some karma configuration tweaks from Angular Material ## Does this PR introduce a breaking change? ``` [ ] Yes [x] No ``` <!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. --> <!-- Note that breaking changes are highly unlikely to get merged to master unless the validation is clear and the use case is critical. --> ## Other information
|
@MarcoZehe OK, PR #11488 has been merged. Can you please rebase this and then force push to your branch? That should hopefully get all of the tests passing in TravisCI. Thank you! |
Yes, all passed! -O-/ |
Splaktar
added
pr: merge ready
Splaktar
changed the title
|
The first line of your commit is a bit too long. Can you please update it to just be "fix(nav-bar): improve screen reader support"? Thanks! |
* make the nav-bar component a tablist. * make the nav-item's li element presentational and the button a tab. * apply the aria-label, if provided, to the button directly, not the li element. Fixes #11485
Done. :) Thank you again for your patience and guidance! |
|
Looks good, thank you! I'll let you know if we run into any issues in the "presubmit" process. This will run the change through all of the relevant tests in Google to make sure that it passes unit tests, screenshot tests, and a11y tests. |
6 tasks
3 tasks
|
I've submitted PR #11494 which should improve the a11y state of nav-bar quite a bit more. I tested it with the changes in this PR and it worked well and doesn't seem to conflict. |
marosoft
pushed a commit
to marosoft/material
that referenced
this pull request
Nov 11, 2018
…ngular#11488) <!-- Filling out this template is required! Do not delete it when submitting a Pull Request! Without this information, your Pull Request may be auto-closed. --> ## PR Checklist Please check that your PR fulfills the following requirements: - [x] The commit message follows [our guidelines](https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#-commit-message-format) - [x] Tests for the changes have been added or this is not a bug fix / enhancement - [x] Docs have been added, updated, or were not required ## PR Type What kind of change does this PR introduce? <!-- Please check the one that applies to this PR using "x". --> ``` [x] Bugfix [ ] Enhancement [ ] Documentation content changes [ ] Code style update (formatting, local variables) [ ] Refactoring (no functional changes, no api changes) [ ] Build related changes [ ] CI related changes [ ] Infrastructure changes [ ] Other... Please describe: ``` ## What is the current behavior? - A new `npm i` may pick up `jasmine-core@2.99.1` which will cause many test failures since we don't currently support that version. - Setting `md-nav-href=""` on an `md-nav-item` doesn't apply the proper `ng-href=""` because of a bad `if` check. - The docs indicate that a navigation attribute **must** be defined on each `md-nav-item`, but the code doesn't throw an exception if this occurs. This can lead to unexpected behaviors like infinite loops or hanging async tasks. - The Closure properties are incorrect in a number of cases. <!-- Please describe the current behavior that you are modifying and link to one or more relevant issues. --> Issue Number: Relates to angular#11485. Relates to angular#11486. Fixes angular#11487. ## What is the new behavior? - set ng-href on nav-item even if md-nav-href is empty - fix closure properties to better represent the existing code - pin the version of jasmine-core since we don't support 2.99.1 yet - slightly improve the tone of the exceptions - pull in some karma configuration tweaks from Angular Material ## Does this PR introduce a breaking change? ``` [ ] Yes [x] No ``` <!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. --> <!-- Note that breaking changes are highly unlikely to get merged to master unless the validation is clear and the use case is critical. --> ## Other information
marosoft
pushed a commit
to marosoft/material
that referenced
this pull request
Nov 11, 2018
… reader users * make the nav-bar component a tablist. * make the nav-item's li element presentational and the button a tab. * apply the aria-label, if provided, to the button directly, not the li element. Fixes angular#11485 <!-- Filling out this template is required! Do not delete it when submitting a Pull Request! Without this information, your Pull Request may be auto-closed. --> ## PR Checklist Please check that your PR fulfills the following requirements: - [x] The commit message follows [our guidelines](https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#-commit-message-format) - [x] Tests for the changes have been added or this is not a bug fix / enhancement - [x] Docs have been added, updated, or were not required ## PR Type What kind of change does this PR introduce? <!-- Please check the one that applies to this PR using "x". --> ``` [x] Bugfix [ ] Enhancement [ ] Documentation content changes [ ] Code style update (formatting, local variables) [ ] Refactoring (no functional changes, no api changes) [ ] Build related changes [ ] CI related changes [ ] Infrastructure changes [ ] Other... Please describe: ``` ## What is the current behavior? <!-- Please describe the current behavior that you are modifying and link to one or more relevant issues. --> Issue Number: angular#11485 Currently, the nav-bar element is exposed as a listbox accessible to screen reader users. Its nav-item child consists of a li element which currently has role "option" (as appropriate for a listbox child), and the button inside that li is exposed as is. Screen readers do have a hard time interacting with this construct, since they do not expect an option to contain a button. ## What is the new behavior? The new behavior is that the nav-bar element is exposed as a tablist, and the button inside the li of the nav-item as a tab accessible. In addition, the aria-label that was originally applied to the nav-item's li element is now applied to the buton, since the li in the tablist/tab pattern is to be marked presentational. aria-label must therefore go onto the element that has role "tab". No changes besides semantic ARIA exposure has been changed, the keyboard behavior is the same as well as any other behavior this component exposes. Web developers needn't change anything, either. ## Does this PR introduce a breaking change? ``` [ ] Yes [x] No ``` <!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. --> <!-- Note that breaking changes are highly unlikely to get merged to master unless the validation is clear and the use case is critical. --> ## Other information The tests have been adjusted to look for the aria-label on the tab directly, not the parent.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
… reader users
Fixes #11485
PR Checklist
Please check that your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #11485
Currently, the nav-bar element is exposed as a listbox accessible to screen reader users. Its nav-item child consists of a li element which currently has role "option" (as appropriate for a listbox child), and the button inside that li is exposed as is.
Screen readers do have a hard time interacting with this construct, since they do not expect an option to contain a button.
What is the new behavior?
The new behavior is that the nav-bar element is exposed as a tablist, and the button inside the li of the nav-item as a tab accessible. In addition, the aria-label that was originally applied to the nav-item's li element is now applied to the buton, since the li in the tablist/tab pattern is to be marked presentational. aria-label must therefore go onto the element that has role "tab".
No changes besides semantic ARIA exposure has been changed, the keyboard behavior is the same as well as any other behavior this component exposes. Web developers needn't change anything, either.
Does this PR introduce a breaking change?
Other information
The tests have been adjusted to look for the aria-label on the tab directly, not the parent.