Skip to content

Remove old/unnecessary reboot bug fix - #32631

Merged
XhmikosR merged 4 commits into
mainfrom
patrickhlauke-remove-cargocult-forced-focus
Jan 5, 2021
Merged

XhmikosR merged 4 commits into
mainfrom
patrickhlauke-remove-cargocult-forced-focus

Conversation

@patrickhlauke

@patrickhlauke patrickhlauke commented Dec 27, 2020 •

Copy link
Copy Markdown
Member

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 backport-to-v4 Legacy: v4 backports css Sass sources or compiled CSS v5 v5, the v5-dev branch labels Dec 27, 2020
@patrickhlauke

Copy link
Copy Markdown
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.

@patrickhlauke

Copy link
Copy Markdown
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
Copy Markdown
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

XhmikosR commented Jan 5, 2021

Copy link
Copy Markdown
Member

Not sure if this should be backported, though.

@XhmikosR
XhmikosR merged commit eb45005 into main Jan 5, 2021
@XhmikosR
XhmikosR deleted the patrickhlauke-remove-cargocult-forced-focus branch January 5, 2021 20:20
@patrickhlauke

Copy link
Copy Markdown
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
Copy Markdown
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?)

@XhmikosR

XhmikosR commented Jan 7, 2021

Copy link
Copy Markdown
Member

@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 pushed a commit that referenced this pull request Jan 11, 2021
XhmikosR pushed a commit that referenced this pull request Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-v4 Legacy: v4 backports css Sass sources or compiled CSS v5 v5, the v5-dev branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants