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

Suppress focus outline for buttons when it shouldn't be visible in Chromium #32689

Merged
merged 3 commits into from Jan 10, 2021

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Jan 6, 2021

Follow-up to #32631

To see this in action easily, using Chrome, check one of the "Copy" buttons in the documentation. Currently, they end up with the two-tone outline (even though they shouldn't), but with this additional "hack" (more of a restating of what the browser should be doing), the outline only shows if you're setting focus with the keyboard, not mouse or touch tap

Preview: https://deploy-preview-32689--twbs-bootstrap.netlify.app/

@patrickhlauke patrickhlauke requested a review from mdo January 6, 2021 00:30
@patrickhlauke patrickhlauke requested a review from a team as a code owner January 6, 2021 00:30
@patrickhlauke patrickhlauke added this to Inbox in v5.0.0-beta2 via automation Jan 6, 2021
@patrickhlauke patrickhlauke added this to Inbox in v4.6.0 via automation Jan 6, 2021
@ffoodd
Copy link
Member

ffoodd commented Jan 6, 2021

I'm definetely in, that's a first step at using :focus-visible appropriately.

However regarding your comment on #32631, I think it means a few older WebKit browsers will still show focus outlines (based on Can I Use).

I personnaly don't care that much, but like to at least mention this.

v5.0.0-beta2 automation moved this from Inbox to Approved Jan 6, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Jan 7, 2021

Removed from backporting. @mdo your call

@XhmikosR XhmikosR removed this from Inbox in v4.6.0 Jan 7, 2021
@patrickhlauke
Copy link
Member Author

@mdo thoughts on backporting?

@mdo
Copy link
Member

mdo commented Jan 9, 2021

@patrickhlauke I'd say backport this and #32631 for sure.

@XhmikosR XhmikosR added this to Inbox in v4.6.0 via automation Jan 10, 2021
@XhmikosR
Copy link
Member

The only reason I wasn't sure about backporting is due to @ffoodd's comment above which v4 supports, so this will be inconsistent?

@patrickhlauke
Copy link
Member Author

The only reason I wasn't sure about backporting is due to @ffoodd's comment above which v4 supports, so this will be inconsistent?

this is more of a nice-to-have progressive enhancement, so personally think it's ok to use even if it's not supported by all browsers that are targeted by v4

@patrickhlauke patrickhlauke merged commit a2ae2c3 into main Jan 10, 2021
v5.0.0-beta2 automation moved this from Approved to Done Jan 10, 2021
@patrickhlauke patrickhlauke deleted the patrickhlauke-button-focus-fix branch January 10, 2021 10:42
@XhmikosR
Copy link
Member

@patrickhlauke feel free to push any backports to my v4-dev-xmr branch which is #32748. Should be the last batch of patches before 4.6.0, hopefully :)

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Jan 10, 2021

yup, on it today hopefully.

[edit: done]

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

4 participants