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/issue 1387 sidebar icons nvda #11816

Merged
merged 3 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@timwright12
Contributor

timwright12 commented Nov 13, 2018

Description

Fixes #1387

How has this been tested?

Linting and in-browser.

Types of changes

Added a span with aria-hidden="true" to prevent NVDA from over-announcing the sidebar component state.

Checklist:

  • [ x ] My code is tested.
  • [ x ] My code follows the WordPress code style.
  • [ x ] My code follows the accessibility standards.
  • [ x ] My code has proper inline documentation.

@tofumatt tofumatt self-requested a review Nov 13, 2018

{ /*
This span is wrapping the icon so when `isOpened` is updated the
state of `aria-expanded` isn't re-announced to NVDA
*/ }

This comment has been minimized.

@afercia

afercia Nov 14, 2018

Contributor

Thanks @timwright12! Actually, it's the opposite case: Firefox + NVDA don't announce aria-expanded because the browser repaints the whole element. Not sure we should patch it though, as next NVDA mitigates the issue.

This comment has been minimized.

@timwright12

timwright12 Nov 14, 2018

Contributor

@afercia I updated the comment in case we end up wanting to merge this in. I do agree with your statement in #1387 that we probably shouldn't rely on alpha releases of software that may or may not be released by the 5.0 milestone. Especially when it's something so small and easy to patch like this.

So if we want to push this fix, it's here, if not we should close out all the open items related to it, so no one else jumps in - and certainly remove it from the 5.0 milestone list.

@tofumatt

Coolio, looks good!

@tofumatt

This comment has been minimized.

Member

tofumatt commented Nov 14, 2018

Hi @timwright12: just so you know the failing snapshots here are legitimate; in the future it's worth running npm run test-unit and npm run test-e2e when changing HTML or behaviour in the app 😄

In this case the HTML has changed so snapshots need updating. In this case I've just gone ahead and updated them with npm run test-unit:update, but in the future if you changed HTML and expect the snapshots to change, please run those if you see tests failing. 😄

@tofumatt tofumatt added this to the 4.4 milestone Nov 14, 2018

Tim Wright and others added some commits Nov 13, 2018

@tofumatt tofumatt force-pushed the timwright12:fix/issue-1387-sidebar-icons-nvda branch from 925db7f to 21548e7 Nov 14, 2018

@tofumatt tofumatt merged commit 9e74a5f into WordPress:master Nov 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@timwright12

This comment has been minimized.

Contributor

timwright12 commented Nov 14, 2018

@tofumatt oh excellent, thanks for clearing that up. I was wondering about the tests!

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix/issue 1387 sidebar icons nvda (WordPress#11816)
* added hiding span around sidebar icons

* updated comment

* chore: Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment