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

Add landmark regions #2380

Merged
merged 3 commits into from
Sep 8, 2017
Merged

Add landmark regions #2380

merged 3 commits into from
Sep 8, 2017

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Aug 11, 2017

Partially addresses #467

This PR tries to introduce an easy way for screen reader users to jump through the main editor sections via ARIA landmark regions.

All the major screen readers support landmarks: they're just HTML elements with special semantic meaning (nav, main, aside, etc.) or div elements with special ARIA roles (main, complementary, region, etc.).

We've discussed this in a recent accessibility team meeting (see #467) and initially decided to use aside. However, I'd propose to use the generic role region because:
https://www.w3.org/TR/wai-aria-practices/#aria_lh_complementary

A complementary landmark is a supporting section of the document, designed to be complementary to the main content at a similar level in the DOM hierarchy, but remains meaningful when separated from the main content.

... complementary landmarks should be top level landmarks (e.g. not contained within any other landmarks).

Instead, the editor is nested within the main content.

If the complementary content is not related to the main content, a more general role should be assigned (e.g. region).

So, region seemed more appropriate to me. Reference and examples:
https://www.w3.org/TR/wai-aria-practices/#aria_lh_region
https://www.w3.org/TR/wai-aria-practices/examples/landmarks/region.html

I'd appreciate some feedback about the aria-label values, especially thinking at how they would be translated in other languages. /cc @joedolson @rianrietveld and everyone.

Screenshots with Safari + VoiceOver:

landmarks visual

landmarks text

I had to change some elements to clean-up some semantics. For example the header used for the text mode quicktags was reported as a "banner" landmark:

landmarks text with banner

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Aug 11, 2017
@codecov
Copy link

codecov bot commented Aug 11, 2017

Codecov Report

Merging #2380 into master will increase coverage by 7.4%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2380     +/-   ##
========================================
+ Coverage    25.9%   33.3%   +7.4%     
========================================
  Files         157     183     +26     
  Lines        4849    7020   +2171     
  Branches      820    1379    +559     
========================================
+ Hits         1256    2338   +1082     
- Misses       3033    3845    +812     
- Partials      560     837    +277
Impacted Files Coverage Δ
editor/header/index.js 0% <ø> (ø) ⬆️
editor/modes/visual-editor/index.js 0% <ø> (ø) ⬆️
editor/sidebar/index.js 0% <0%> (ø) ⬆️
editor/modes/text-editor/index.js 0% <0%> (ø) ⬆️
blocks/library/audio/index.js 13.33% <0%> (-23.04%) ⬇️
blocks/inspector-controls/range-control/index.js 83.33% <0%> (-16.67%) ⬇️
editor/header/saved-state/index.js 75% <0%> (-10.72%) ⬇️
blocks/library/paragraph/index.js 40% <0%> (-7.06%) ⬇️
blocks/library/gallery/index.js 26.31% <0%> (-7.02%) ⬇️
components/panel/body.js 94.73% <0%> (-5.27%) ⬇️
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd502be...142127e. Read the comment docs.

@afercia
Copy link
Contributor Author

afercia commented Aug 11, 2017

Needs a bit more testing: seems NVDA doesn't get the landmark used for the editor toolbar (see screenshot below). JAWS 17 gets all the landmarks correctly (tested using the landmarks dialog: JAWS key + Ctrl + R).

screenshot 47

@jasmussen
Copy link
Contributor

First off, thanks so very much for working on this. 🍷

This seems good to me. I think moreso than me, this PR needs the eyes of accessibility experts like yourself or Joe or Rian. I wish we could've kept the semantic header, but if this change is necessary then so be it.

I noticed the two regions, "Editor text mode region" and "Editor visual mode region". Is there any way we can label these to make it clearer that they are both to versions of the same? Something something "Editor content region, text mode", or something in that vein?

@afercia
Copy link
Contributor Author

afercia commented Sep 2, 2017

I wish we could've kept the semantic header

I'm not sure the whole semantics of header + H1 is so appropriate, since we're in an authoring context. Will try to discuss this in the next a11y meeting.

Re: labels: sure they can definitely be improved. Just keep in mind the word "region" is added by the screen reader and it's not part of the actual label.

@afercia
Copy link
Contributor Author

afercia commented Sep 2, 2017

Re: NVDA not announcing the editor toolbar landmark: seems NVDA doesn't like nested landmarks when there's no content between them. Actually, between the div with role="main" and the editor toolbar with role="region" there's just the screen-meta thing, that's hidden with display: none.

I've noticed this when completely disabling styles in Firefox: then the toolbar region gets reported:

screenshot 51

Also adding just a <br> between the two regions, fixes it:

screenshot 52

To fix this I'd say there are two options:

  • add something between the two regions: this could be the missing main H1, see Add headings for the Editor screen main sections #503
  • review the page structure when Gutenberg will be merged: admin-header.php could use some conditions to output a custom markup in the Gutenberg page, removing the main landmark

I'd lean towards the first option, for consistency and because we need a main H1 to give users feedback about what the page is about. This should be addressed in #503 so I'm considering to use a temporary fix for now.

@afercia afercia requested a review from aduth September 2, 2017 13:42
@afercia
Copy link
Contributor Author

afercia commented Sep 8, 2017

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants