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

Improve .screen-reader-text #10

Merged
merged 4 commits into from
Apr 24, 2017
Merged

Improve .screen-reader-text #10

merged 4 commits into from
Apr 24, 2017

Conversation

ffoodd
Copy link
Contributor

@ffoodd ffoodd commented Mar 10, 2017

Bulletproofing this utility class:

It's been widely tested on this codepend and proofread then translated on Hugo Giraudel's blog.

Please let me know if you find any issue related to this :)

And thanks a lot for those patterns, they're great <3

Bulletproofing this utility class:
* prevents border to appear, if any;
* progressively enhance the clip thing: `clip` [is deprecated](https://www.w3.org/TR/css-masking-1/#clip-property) and @ryuran [suggested the sorter possible version](https://twitter.com/ryuran78/status/778943389819604992) for `clip-path`;
* [J. Renée Beach warned about the single pixel width having side effects on text rendering and therefore on its vocalisation by screen readers](https://medium.com/@jessebeach/beware-smushed-off-screen-accessible-text-5952a4c2cbfe#.vcd5xlpgg).

It's been widely tested [on this codepend](http://codepen.io/ffoodd/pen/gwKZyq?editors=1100#) and [proofread then translated on Hugo Giraudel's blog](http://hugogiraudel.com/2016/10/13/css-hide-and-seek/).

Please let me know if you find any issue related to this :)

And thanks a lot for those patterns, they're great <3
@rianrietveld
Copy link
Member

@ffoodd Thanks for your PR.
This is interesting, we will look into it.

@afercia
Copy link
Contributor

afercia commented Mar 11, 2017

@ffoodd thanks, very appreciated. Yep I'd also say this pattern should be improved a bit. However, there are some differences in what is actually used in the WordPress Core and what is used by bundled themes instead. Seems like themes have a shorter version of the screen-reader-text class that probably should be updated.

Core:

.screen-reader-text,
.screen-reader-text span,
.ui-helper-hidden-accessible {
	position: absolute;
	margin: -1px;
	padding: 0;
	height: 1px;
	width: 1px;
	overflow: hidden;
	clip: rect(0 0 0 0);
	border: 0;
	word-wrap: normal !important; /* many screen reader and browser combinations announce broken words as they would appear visually */
}

Twenty Seventeen:

.screen-reader-text {
	clip: rect(1px, 1px, 1px, 1px);
	height: 1px;
	overflow: hidden;
	position: absolute !important;
	width: 1px;
	word-wrap: normal !important; /* Many screen reader and browser combinations announce broken words as they would appear visually. */
}

I'm not sure why themes should use a different class. Though WordPress uses an old syntax for the clip property, I think the best option would be establishing one single pattern to be used everywhere, in Core and Themes.

About the single points:

border
I'd say absolutely yes, and it's already used in core.

clip-path
Though support is still very limited, I'd say it's definitely something to take into consideration. However, I'm not sure we should introduce a property that is still "unofficial". Maybe we should do this when the CSS Masking Module Level 1 will be an official recommendation. Currently, it's a Candidate Recommendation.

Also, I'm not sure about the combined effect of clip and clip-path for browsers that currently support both properties. As far as I know, clip-path doesn't override clip: they're different properties. So what happens when a browser tries to apply both? Has this been tested?

single pixel width
In the WP accessibility team, we've already discussed the issue so clearly illustrated by J. Renée Beach and turned out there was already a solution for that.

What it's not mentioned in that post is that, by default, words don't break and spaces are not stripped out, even if you use clip with a single pixel size.

I see there was some investigation but seems they missed to identify the real reason why words were smushed together. On WordPress, that happened because some elements use word-wrap: break-word; which is inherited by children elements. I guess that's the same reason why it was happening on Facebook in the cases mentioned by J. Renée Beach: some parent element using word-wrap: break-word.

The solution introduced in core is word-wrap: normal !important; and it's there since May 2015
https://core.trac.wordpress.org/changeset/32509. Some months later was adopted by bundled themes too. This should definitely be added to the a11ythemepatterns.

I'd recommend to give the Trac ticket a read, for more details, see https://core.trac.wordpress.org/ticket/31962

So I'm not sure white-space: nowrap; (in addition to the word-wrap: normal !important; we should add in the a11ythemepatterns) would be useful at all, unless you've noticed and reproduced cases in WordPress or in the bundled themes where words get announced by screen readers as they were smushed together.

Lastly
I'd consider to add also:

margin: -1px;
padding: 0;

@afercia
Copy link
Contributor

afercia commented Mar 11, 2017

P.S.
I see there's one more occurrence of the screen-reader-text CSS class in dropdown-menus/vanilla-js/style.css that should be updated as well.

@ffoodd
Copy link
Contributor Author

ffoodd commented Mar 13, 2017

Very interesting answer, thanks!

I didn't know about the inherited word-wrap thing. That's a good point.
I proposed this based on a utility class for frameworks and tools, so I'll keep it as a preventing property. But maybe WordPress context is slightly different… It seems you already investigated a lot — so this is your call to keep white-space or not :)

clip-path is indeed not cross-browser for now.you'll notice I'm not using vendor prefixed version, as my point is to use it as progressive enhancement and try to be more future proof. The codepen I linked had been tested with a wide range of browser and ATs without noticing any trouble when combining clip and clip-path. However you're right: they're not the same at all.

That should probably be tested in WordPress itself.

I'll add padding and margin too, as I had it in mind but they aren't in my PR -_-

Thanks for reading, I'm still learning :)

Adding `margin` and `padding`.
@ffoodd
Copy link
Contributor Author

ffoodd commented Mar 13, 2017

As another source on the white-space / word-wrap topic, I just found the same issue in Drupal, with another useful discussion. They solved it the same way you evoked: word-wrap: normal !important;.

I don't know if there is any difference between those two declarations — but since WordPress already solved this using word-wrap, I guess I should stick to this solution.

Any thought?

@afercia
Copy link
Contributor

afercia commented Mar 13, 2017

Interesting discussion on the Drupal issue. I'd lean towards keeping it consistent with what Core does and use word-wrap: normal !important; 🙂

rianrietveld referenced this pull request in humanmade/hm-pattern-library Mar 31, 2017
@afercia
Copy link
Contributor

afercia commented Apr 24, 2017

Discussed in today's a11y meeting, decided to merge this and open a new issue to modernize the class, taking into account end of support for IE 8-9-10 announced for WordPress 4.8. See
https://make.wordpress.org/core/2017/04/23/target-browser-coverage/

we are officially ending support for Internet Explorer versions 8, 9, and 10, starting with WordPress 4.8

@afercia afercia merged commit b36ce6b into wpaccessibility:master Apr 24, 2017
@ffoodd ffoodd deleted the patch-1 branch April 25, 2017 07:14
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.

None yet

3 participants