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 explicit suppression of focus outline #33029

Merged
merged 3 commits into from
Feb 11, 2021

Conversation

patrickhlauke
Copy link
Member

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.

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.
@patrickhlauke patrickhlauke requested a review from a team as a code owner February 9, 2021 18:50
@patrickhlauke patrickhlauke marked this pull request as draft February 9, 2021 18:50
@patrickhlauke
Copy link
Member Author

prompted by #29875

@patrickhlauke
Copy link
Member Author

Want to primarily see if removing this in reboot causes any of bootstrap's own components to suddenly start showing inappropriate focus outlines on things like modal dialog containers etc. It shouldn't, since I believe we explicitly suppress these cases where tabindex="-1" is used and focus is set to it. And if we do find that core BS components do suffer when this is removed, the problem should likely be fixed there at the component level, rather than with this heavy-handed-yet-redundant-if-you're-doing-it-right fix in reboot

@patrickhlauke patrickhlauke marked this pull request as ready for review February 9, 2021 20:03
@patrickhlauke
Copy link
Member Author

Doesn't seem to adversely affect any of our components, from testing. And, as the change in #28437 already essentially neutered this directive, I think it's safe to fully remove it.

@ffoodd
Copy link
Member

ffoodd commented Feb 10, 2021

Just had a look using Firefox—which IMHO is the most susceptible of messing focus styles— and didn't notice anything. Tested modals, collapses and skip links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants