Skip to content

Conversation

@MartijnCuppens
Copy link
Member

Analysis of the .sr-only properties:

position: absolute: needed to prevent the element from taking up space in the document
width: 1px: prevents the element from becoming too wide and therefor creating scrollbars
height: 1px: prevents the element from becoming too tall and therefor creating scrollbars
padding: 0: padding could increase the width or height even when those are set to 1px
overflow: hidden: prevent the text inside from breaking out the box
clip: rect(0, 0, 0, 0): make the element invisible
white-space: nowrap: see #22154
border: 0: border could increase the width or height even when those are set to 1px

So yeah, we all need them.

We had this problem: #28691, which could be fixed by adding !important to all properties, but that would just make this problem bigger: https://codepen.io/MartijnCuppens/pen/Pgrpwe.

And than I remembered a technique I used before (#25806), just make use of the :not() selector. This would solve #28691 without introducing padding issues.

This would also not require the .sr-only class on elements with .sr-only-focusable. I've changed the docs a bit to make this clear.

I added @Jakobud as co author for the !important commit since he already fixed that in #28701.

Fixes #28691
Closes #28701

@ysds
Copy link
Contributor

ysds commented May 3, 2019

Using :not is nice approach.

I think that the :not(:active) come from the current code, but I doubt it's needed (:not(:focus) would be enough).

CC: @patrickhlauke

@XhmikosR XhmikosR force-pushed the master-mc-sr-only branch from 8fd87c4 to 2f7a7f5 Compare May 4, 2019 12:03
@XhmikosR
Copy link
Member

XhmikosR commented May 4, 2019

@MartijnCuppens: rebased. Please make any further changes if needed.

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually, i like it - and prefer the fact that you don't have to double up/combine the sr-only and sr-only-focusable classes anymore, but just pick one. Not had time to test, but trust that if it does what it says on the tin, the approach is sound (modulo that thing about :not(:active)

@MartijnCuppens
Copy link
Member Author

I've also added a notice in the migration guide. @patrickhlauke, could you have a look if everything is ok now?

@XhmikosR
Copy link
Member

Ping @patrickhlauke

@XhmikosR XhmikosR force-pushed the master-mc-sr-only branch from 7cff7d4 to 9b6c882 Compare May 19, 2019 12:41
Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With exception of the tiny change request, everything else looks good to me and can be merged after change is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.sr-only should use !important

5 participants