Navigation Menu

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 old/unnecessary reboot bug fix #32631

Merged
merged 4 commits into from Jan 5, 2021

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Dec 27, 2020

From initial testing, this bug ("transparent button background results in a loss of the default button focus styles") doesn't seem to manifest itself anywhere in Bootstrap (since we don't just set transparent background anywhere on buttons, and when we do set explicit button styles in the more specific stylings, we already do create a custom :focus style anyway)

Incidentally, this forced definition of :focus is biting us in the rear because it essentially circumvents the browser logic that only applies focus outline based on the browser's heuristic (i.e. not applying the button outline when clicked with a mouse). this forces the focus style to always show even after a mouse click, which is causing is hassle that we then need to work around - see #32630

From initial testing, this bug doesn't seem to manifest itself anywhere in Bootstrap (since we don't just set transparent background anywhere on buttons, and when we do set explicit button styles in the more specific stylings, we already do create a custom `:focus` style anyway)
@patrickhlauke patrickhlauke requested a review from a team as a code owner December 27, 2020 22:06
@patrickhlauke patrickhlauke added this to Inbox in v4.6.0 via automation Dec 27, 2020
@patrickhlauke patrickhlauke added this to Inbox in v5.0.0-beta2 via automation Dec 27, 2020
@patrickhlauke
Copy link
Member Author

Testing with Firefox (and IE, though of course v5 isn't aimed at it, but the principle is the same for v4.x which does include it) on https://deploy-preview-32631--twbs-bootstrap.netlify.app/docs/5.0/content/reboot/#forms and https://deploy-preview-32631--twbs-bootstrap.netlify.app/docs/5.0/components/buttons/#outline-buttons and various other places, I see no issues with "transparent button background results in a loss of the default button focus styles" per the comment. It may well be that we've been cargo-culting this fix unnecessarily.

v5.0.0-beta2 automation moved this from Inbox to Approved Dec 28, 2020
@patrickhlauke
Copy link
Member Author

just want to wait/check with others like @mdo who may know of the original reason that patch was added in the first place... (or i could go on an archeological expedition to find the commit where this was first added)

@septatrix
Copy link
Contributor

In the future it should be possible to use :focus-visible to only show focus styles when the browser determines they are necessary however support is currently not perfect

@XhmikosR
Copy link
Member

XhmikosR commented Jan 5, 2021

Not sure if this should be backported, though.

@XhmikosR XhmikosR merged commit eb45005 into main Jan 5, 2021
v5.0.0-beta2 automation moved this from Approved to Done Jan 5, 2021
@XhmikosR XhmikosR deleted the patrickhlauke-remove-cargocult-forced-focus branch January 5, 2021 20:20
@patrickhlauke
Copy link
Member Author

i'd say it's worth backporting as it shouldn't actually affect anything in v4. and other PRs that i think are worth backporting as well (e.g. #32630 (review)) rely on this PR to be backported if they're also to be backported

@patrickhlauke
Copy link
Member Author

seems that while removing this fixes the unsightly behaviour in firefox (for when buttons are used as tab riders), something still trips up chromium making it show its default outline even on mouse press. this may need a further fix. adding

button:focus:not(:focus-visible) {
  outline: 0;
}

to reboot seems to work (and as it seems to only still be a problem in chromium, and recent chromium is one of the few browsers that supports :focus-visible, this seems like a fair enough addition...thoughts?)

patrickhlauke added a commit that referenced this pull request Jan 6, 2021
@XhmikosR XhmikosR moved this from Inbox to Needs manual backport in v4.6.0 Jan 7, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Jan 7, 2021

@mdo @MartijnCuppens please confirm if this can safely be backported or not.

patrickhlauke added a commit that referenced this pull request Jan 10, 2021
…romium (#32689)

Follow-up to #32631

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
patrickhlauke added a commit that referenced this pull request Jan 10, 2021
@XhmikosR XhmikosR moved this from Needs manual backport to Cherry-picked/Manually backported in v4.6.0 Jan 11, 2021
XhmikosR pushed a commit that referenced this pull request Jan 11, 2021
XhmikosR pushed a commit that referenced this pull request Jan 13, 2021
XhmikosR pushed a commit that referenced this pull request Jan 13, 2021
@XhmikosR XhmikosR moved this from Cherry-picked/Manually backported to Shipped in v4.6.0 Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.6.0
Shipped
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants