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 sr-only and not-sr-only utilities #964

Merged
merged 1 commit into from
Jul 10, 2019
Merged

Conversation

adamwathan
Copy link
Member

@adamwathan adamwathan commented Jun 10, 2019

This PR adds a new accessibility core plugin that includes sr-only and not-sr-only utilities.

Their implementation is as follows:

.sr-only {
  position: absolute;
  width: 1px;
  height: 1px;
  padding: 0;
  margin: -1px;
  overflow: hidden;
  clip: rect(0, 0, 0, 0);
  white-space: nowrap;
  border-width: 0;
}

.not-sr-only {
  position: static;
  width: auto;
  height: auto;
  padding: 0;
  margin: 0;
  overflow: visible;
  clip: auto;
  white-space: normal;
}

sr-only is based on Bootstrap's implementation, with one notable difference being that we use border-width: 0 to remove the border instead of border: 0. This is to prevent blowing out our default Preflight border styles, so that if you need to apply not-sr-only to an item, the border utilities will start working as expected.

not-sr-only simply resets all of the properties set by sr-only. This class is useful if you want to make something visible at larger breakpoints but keep it visually hidden at smaller ones:

<a href="#">
  <svg><!-- ... --></svg>
  <span class="sr-only sm:not-sr-only">Settings</span>
</a>

It can also be used with the focus variant modifier to make a visually hidden element appear when focused:

<a href="#" class="sr-only focus:not-sr-only">
  Skip to content
</a>

If anyone has any feedback or suggested changes to this implementation let me know!

This blog post suggests that this implementation might actually be better than Bootstrap's:

.sr-only {
    border: 0;
    clip: rect(0 0 0 0);
    height: auto; /* new - was 1px */
    margin: 0; /* new - was -1px */
    overflow: hidden;
    padding: 0;
    position: absolute;
    width: 1px;
    white-space: nowrap;
}

...but since it hasn't been adopted yet by Bootstrap or HTML5 Boilerplate I'm a little hesitant to use it, since there might be issues that haven't been uncovered by mass usage.

@PatrickJoannisse
Copy link

I'm not usually a "if it's not broken, don't fix it" kind of guy...but when you're talking about web accessibility and standards that's the answer I would give you. I'd stick to the implementation that's used by governments and companies with a lot of money on the line: Bootstrap's.

I'm still not sure what the blog post is trying to fix or improve. A smaller class in CSS? What do you want in that CSS class? To be reliable and tested or to be a couple lines shorter?

@justgage
Copy link

This is awesome, great to see that tailwind is making this easier for people. For what it's worth maybe firing up a few screen readers is the best way to test the classes. Seem like they would work though 😁 (not an expert)

@aaronbnb
Copy link

aaronbnb commented Jun 11, 2019

You don't need a tabindex on the skip link. It's an anchor tag with an href attribute. The element is already focusable.

@OwenMelbz
Copy link
Contributor

the UK govt has a really decent team working on accessibility have their blog has so much on it. If you search stuff like GDS + Topic, normally something good e.g. https://accessibility.blog.gov.uk/2017/02/08/advice-for-creating-content-that-works-well-with-screen-readers/ loads of content and explanations on YouTube.

https://youtu.be/JHaLzm-FGsc This is great to help understand the complexity of forms if you really want to make really accessible interfaces

@florianbouvot
Copy link
Contributor

@adamwathan HTML5 Boilerplate switch to sr-only naming and fix clip : https://github.com/h5bp/main.css/blob/master/dist/_helpers.css#L14

@sambeevors
Copy link

This is great, the first thing I do when I start a project with Tailwind is take .sr-only from Bootstrap. Welcome addition 👍🏻

@opdavies
Copy link

I've been using the https://github.com/webdna/tailwindcss-visuallyhidden plugin, but I think that it would be a great addition to Tailwind core.

@garygreen
Copy link

Does anyone know why tools like this are needed when we have aria accessibility guidelines/attributes? I wonder if there are some shortcomings in using aria which is why Bootstrap include this utility out the box?

@opdavies
Copy link

I think that ARIA and roles like aria-hidden are hiding the content from assistive technologies like screen readers (e.g. if an image is just presentational, so it doesn't need to be read), whereas this is visually hiding elements on the page but doing so in a way that they can still be read by screen readers like display: none would do.

@tlgreg
Copy link

tlgreg commented Jun 11, 2019

@garygreen
True that aria-label (and with aria-labelledby) you can have text to only show up for screen readers, but unfortunately ARIA support in assistive technologies with a combination of potentially old browsers is dodgy at best.

Powermapper did a test recently with a few, common browser + screen reader combinations:
https://www.powermapper.com/tests/screen-readers/aria/
aria-label can fail in some instances even with screen readers that otherwise have nice support for it.

JAWS, NVDA and VoiceOver are the most commonly used screen readers and JAWS + IE is at the top in usage.
WebAIM stats from 2017: https://webaim.org/projects/screenreadersurvey7/

Another thing for css driven sr-only, you might want to hide something that's not purely just text. For example, a heading, that a screen reader can quickly jump to, but otherwise in your UI, you might not want anything there (common in app-ui). Or interactive elements just for screen readers, that makes it easier/possible for them to use a certain feature of the site, that would be otherwise impossible without mouse/sight.

@jeremyfrank
Copy link

This is awesome as a core plugin!!! Thank you for this work.

Just a heads up that VoiceOver has a nasty bug where it inverts the order of screen reader text and reads things backwards, which I've tracked down to the presence of margin: -1px via the references below. I have an open pull request to fix this for the tailwindcss-visuallyhidden plugin. Can you test this particular scenario? webdna/tailwindcss-visuallyhidden#6

For those interested, here are the references where others have hit the same issue, and @NickColley has done extensive testing with different assistive technologies.
h5bp/main.css#12
https://gist.github.com/nickcolley/19b80ed24d0364cfd3afd3b1b49c4014

@adamwathan
Copy link
Member Author

Just a heads up that VoiceOver has a nasty bug where it inverts the order of screen reader text and reads things backwards

Yeah I've seen this, and the proposed solution to fixing this is the one I mentioned in the original comment of this thread:

image

I don't have enough experience with this stuff to test it properly though, so I'm hesitant to deviate from Bootstrap. If this ends up being the better solution I'm sure they will update their class and we can follow suit.

@jeremyfrank
Copy link

Gotcha. I didn't realize that updated snippet from the blog post referenced was also dealing with the inverted reading. Gonna read that now. 👍

@adamwathan
Copy link
Member Author

I should've just linked to the GitHub thread, the blog post doesn't mention it until the end, heh.

@jeremyfrank
Copy link

Interesting, I dug into bootstrap's version a bit and it looks like the v5 dist file has an updated sr-only class. I know v5 hasn't been officially released yet, but good to know they're giving attention to it.

https://github.com/twbs/bootstrap/blob/master/dist/css/bootstrap.css#L7139-L7157

.sr-only {
  position: absolute;
  width: 1px;
  height: 1px;
  padding: 0;
  overflow: hidden;
  clip: rect(0, 0, 0, 0);
  white-space: nowrap;
  border: 0;
}

.sr-only-focusable:active, .sr-only-focusable:focus {
  position: static;
  width: auto;
  height: auto;
  overflow: visible;
  clip: auto;
  white-space: normal;
}

@adamwathan
Copy link
Member Author

adamwathan commented Jun 11, 2019

It's changed a bit even since then, our implementation is based on this recent PR:

twbs/bootstrap#28720

Only real difference is they changed how they are handling the focusable side of things which we will always do differently just due to the nature of Tailwind, but the implementation of sr-only is unchanged as far as I can tell from what you posted.

@MartijnCuppens
Copy link
Contributor

We (Bootstrap) changed the implementation of the .sr-only and .sr-only-focusable, the latest implementation also generates less css:

.sr-only,
.sr-only-focusable:not(:focus) {
  position: absolute !important;
  width: 1px !important;
  height: 1px !important;
  padding: 0 !important;
  margin: -1px !important;
  overflow: hidden !important;
  clip: rect(0, 0, 0, 0) !important;
  white-space: nowrap !important;
  border: 0 !important;
}

We don't have the ability to generate responsive classes like tailwind has though, might be something to look into later

The reason why we used the margin: -1px because of this weird bug: twbs/bootstrap#25686 (comment).

The latest implementation is available via this url: https://twbs-bootstrap.netlify.com/docs/4.3/helpers/screen-readers/

@adamwathan
Copy link
Member Author

The reason why we used the margin: -1px because of this weird bug: twbs/bootstrap#25686 (comment).

Truly the weirdest bug I've ever seen. Do you know when it shows up in practical situations? The margin: -1px thing causes things to be read in the wrong order by VoiceOver on macOS so have to decide which consequence is the lesser evil I guess.

@MartijnCuppens
Copy link
Contributor

The issue I linked to is the only scenario I know of. margin: 0 0 -1px; also seems to fix the issue, but I'm not sure what the effect is for the VoiceOver on mac (not sure how that works tbh).

@adamwathan
Copy link
Member Author

Going to merge this as-is despite the VoiceOver issue because it's good enough for Bootstrap and it's better than offering nothing. We can refine this in the future if a more bullet-proof solution ever surfaces.

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.