Skip to content
This repository has been archived by the owner on Jul 9, 2018. It is now read-only.

a11y: Use screen-reader-text styles for speak container #42

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 3, 2017

Fixes #38

This pull request seeks to resolve the a11y speak container styling issues noted at #38. Specifically, the updated styles help avoid a scroll overflow issue, doing so by copying the .screen-reader-text styles used elsewhere in the WordPress codebase:

https://github.com/WordPress/WordPress/blob/4.8.2/wp-admin/css/common.css#L120-L133

Open questions:

After publishing this branch, I noted that the styles have since been updated in the trunk branch to use modern CSS properties. We may consider adopting those styles here (cc @afercia).

WordPress/WordPress@c65fe27

Testing instructions:

The original issue was observed in overflow apparent in Gutenberg prior to the workaround put in place in WordPress/gutenberg@0f77b9c . Verify that prior to this commit, with these changes, no overflow exists:

  1. After checking out this branch, create an npm symlink for the a11y package:
    • cd packages/a11y && npm ln
  2. In Gutenberg, check out a commit prior to the workaround fix:
    • git checkout 04727b731a2ec307c9bad1d9d239d3c5e01f89a4
  3. In Gutenberg, establish a link to the local @wordpress/a11y branch:
    • npm ln @wordpress/a11y
  4. In Gutenberg, start the local development build
    • npm run dev
  5. Navigate to the local WordPress installation running your copy of Gutenberg and verify that there is no scroll overflow on the Gutenberg editor screens

Questionable value of the test assertions, which are specific to the implementation and not the expected intent of the test case
@codecov
Copy link

codecov bot commented Oct 3, 2017

Codecov Report

Merging #42 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage   97.57%   97.56%   -0.02%     
==========================================
  Files          17       17              
  Lines         165      164       -1     
  Branches       42       42              
==========================================
- Hits          161      160       -1     
  Misses          4        4
Impacted Files Coverage Δ
packages/a11y/src/addContainer.js 100% <100%> (ø) ⬆️

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 74212b9...49e57d9. Read the comment docs.

@afercia
Copy link
Contributor

afercia commented Oct 3, 2017

We should discuss a bit 'clip: rect( 0 0 0 0 );' this is an old syntax used to support old browsers. The correct syntax now is rect(1px, 1px, 1px, 1px);. Also, clip is deprecated.

Also, the WordPress screen-reader-text CSS class has been updated very recently, see https://core.trac.wordpress.org/changeset/41622

Worth also noting there's a Safari / VoiceOver (pre-existing) bug triggered by the absolute positioning, see https://core.trac.wordpress.org/ticket/42006

@aduth
Copy link
Member Author

aduth commented Oct 3, 2017

Thanks for the feedback @afercia . https://github.com/h5bp/html5-boilerplate/issues/1985 is a rollercoaster of an issue 😄 It doesn't appear there's a good solid fix here yet? Not sure how we'd want to move forward, if the changes from WordPress/WordPress@c65fe27 would be sufficient as an interim improvement.

@afercia
Copy link
Contributor

afercia commented Oct 4, 2017

@aduth unfortunately the recent changes don't fix the issue. Better luck if anyone has friends at Apple maybe 🙂 as the wrong order issue is really a Safari / VoiceOver bug.
Yep, seems at boilerplate they've tried a lot of things and then reverted .

@omarreiss
Copy link
Member

I think this is an improvement in itself. Aligning with recent screen reader text styling update can be done in a separate pull. Merging.

@omarreiss omarreiss merged commit 8e6bfcf into master Nov 28, 2017
@omarreiss omarreiss deleted the fix/38-speak-region-overflow branch November 28, 2017 14:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a11y: Speak region causes page overflow
3 participants