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

Remove indiscriminate outline suppression for tabindex="-1" elements #28437

Merged
merged 2 commits into from
Mar 12, 2019

Conversation

patrickhlauke
Copy link
Member

instead, only apply outline suppression if the browser wouldn't normally apply the focus outline, using the (currently experimental) :focus-visible pseudo-class

Closes #28425

instead, only apply outline suppression if the browser wouldn't normally apply the focus outline, using the (currently experimental) `:focus-visible` pseudo-class
@MartijnCuppens
Copy link
Member

This will add the focus ring back everywhere since this property is not yet supported (https://caniuse.com/#search=focus-visible). Don't know if that's an issue?

@patrickhlauke
Copy link
Member Author

This will add the focus ring back everywhere

everywhere there's a tabindex="-1" and the element is then focused programmatically - either because, say, it's an in-page anchor, or because it was invoked via focus() JS method. unless i missed anything, only our modal dialogs (which have an explicit outline: 0 already) fall into the latter category.

@patrickhlauke
Copy link
Member Author

but yes, the :focus-visible part is speculative/future-compatible (as per the comment in the scss)

Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

Aight, fine by me

@XhmikosR XhmikosR merged commit 3e05d01 into master Mar 12, 2019
@XhmikosR XhmikosR deleted the patrickhlauke-focusvisible-programmatic branch March 12, 2019 13:16
XhmikosR pushed a commit that referenced this pull request Mar 12, 2019
…28437)

instead, only apply outline suppression if the browser wouldn't normally apply the focus outline, using the (currently experimental) `:focus-visible` pseudo-class
@mdo mdo mentioned this pull request Jul 22, 2019
patrickhlauke added a commit that referenced this pull request Feb 9, 2021
It's unclear what the reason for first introducing the original hack here (for `[tabindex="-1"]:focus {...}`) was. Seems something that may have been useful/necessary in SuitCSS, but don't think BS ever relied on this. #18330
It's since been modified to only apply when the browser wouldn't apply a visible outline anyway based on its own heuristics (the `:not(:focus-visible)` part) #28437

But now, thinking this through more...in browsers that do support this pseudo-selector, what this is essentially saying is redundant: don't apply outline in cases where a `tabindex="-1"` element receives focus but the browser wouldn't normally apply focus outline". at best, this is unnecessary. at worst, this actually overrides things an author may explicitly be trying to do with adding `:focus { outline: ... }` explicitly.
mdo pushed a commit that referenced this pull request Feb 11, 2021
It's unclear what the reason for first introducing the original hack here (for `[tabindex="-1"]:focus {...}`) was. Seems something that may have been useful/necessary in SuitCSS, but don't think BS ever relied on this. #18330
It's since been modified to only apply when the browser wouldn't apply a visible outline anyway based on its own heuristics (the `:not(:focus-visible)` part) #28437

But now, thinking this through more...in browsers that do support this pseudo-selector, what this is essentially saying is redundant: don't apply outline in cases where a `tabindex="-1"` element receives focus but the browser wouldn't normally apply focus outline". at best, this is unnecessary. at worst, this actually overrides things an author may explicitly be trying to do with adding `:focus { outline: ... }` explicitly.
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.

3 participants