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

Use :focus-visible polyfill #30872

Closed
wants to merge 5 commits into from
Closed

Use :focus-visible polyfill #30872

wants to merge 5 commits into from

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented May 20, 2020

This is a very rough first try to include and use WICG's :focus-visible polyfill in Bootstrap. Let me add some background.

What

:focus-visible belong to CSS Selectors level 4 specification. As per the spec:

The :focus-visible pseudo-class applies while an element matches the :focus pseudo-class and the user agent determines via heuristics that the focus should be made evident on the element.

Thus it's a very helpful selector to massively enhance accessibility. I suggest you read ":focus-visible and backwards compatibility" by @patrickhlauke for more explanation and even more background.

Why

First of all: it's an easy way to safely drop the (sometimes) ugly default outlines when activating things using a mouse. This PR, as-is, fixes #27163 and may help with #29875.

For years, Bootstrap has been using box-shadow for focus states, which can lead to some confusion since there's also a $enable-shadows variable that may cause some focus style to not render appropriately. See #26802 for an example.

But what's more promising here is that it'd allow us to make focus styles stronger and more obvious.

It would allow us to increase contrasts on :focus, which had been requested a few times (#23329 and #29422).

And I'm quite sure there's probably a few cases with our components where focus could be better. :)

How

There, I need some help from @twbs/js-review :D

I added the dependency, but this definetly require some JS love to be included in the best possible way. For now, I roughly imported the file form the node_modules—which can't obviously stay this way.

On the CSS side:

  1. I added a global reset for default outlines: you can preview this in our docs navbar for example, which got the default outline until now.
  2. This is using the [data-focus-visible-added] attribute instead of the .focus-visible class since it's environment-proof.
  3. And we'll be able to enhance focus everywhere, but that's another story :) FYI in Boosted we chose to use 2px wide solid outlines, and a transition on outline-offset (which may vary, depending on the component). You can use keyboard to navigate through Boosted website to preview this.

Sidenote

@patrickhlauke mentionned in his post in 2018 the potential use of @supports—but it couldn't test for selector support at that time. Well, it can now and the support for, say, @supports selector(:focus-visible) is pretty much the same than support for :focus-visible.

So a future-proof, JS-free way to go might be using @supports—but it'd be much, much more verbose on the CSS side.

@ffoodd
Copy link
Member Author

ffoodd commented May 20, 2020

Bundlewatch diff : +767B for bootstrap.min.js, +3.18KB before minification.

I think it worths it :)

@XhmikosR
Copy link
Member

I'm still against adding this. It's extra dependency.

@ffoodd
Copy link
Member Author

ffoodd commented May 20, 2020

Indeed.

However it'll allow Bootstrap to make a (big) step further on the accessibility side. Maybe using the polyfill isn't the best option as of now, but I wanted to introduce at least a POC for this concern.

I mentionned an alternative already using only future-proof CSS (props to @patrickhlauke):

button:focus { /* some exciting button focus styles */ }

@supports selector(:focus-visible) {
    button:focus { /* undo all the above focused button styles */ }
    button:focus-visible { /* and then reapply the styles here instead */ }
}

but this would bloat our styles and wouldn't really work for now—except for Firefox users.

We could also implement our own polyfill for this: we already have a polyfill file, and we might not need every case covered by WICG's polyfill (it supports IE11, also supports Shadow DOM, etc.).

I'm not fluent enough on the JavaScript side to determine what can be done, but from both an accessibility and a CSS perspective, I think it's the safer way to go.

As I mentionned previously, this is only a POC and I'd like to get as much advices as possible to see what can be done :)

@PigeardSylvain
Copy link

Ok it is an extra dependency but it is usefull on all websites.

js/index.esm.js Outdated Show resolved Hide resolved
@patrickhlauke
Copy link
Member

patrickhlauke commented Sep 7, 2020

as this shows as closed rather than merged, a silly question...is this now in v5 or not?

just asking because i've long been a proponent of adding this (with the polyfill, with an eye towards future proofing), and now that Chrome shipped it ... it's probably a good time to do it. https://blog.chromium.org/2020/09/giving-users-and-developers-more.html

@ffoodd
Copy link
Member Author

ffoodd commented Sep 7, 2020

It's not in v5, this was the latest attempt but adding a polyfill seemed an extra effort that we couldn't afford ATM.

FWIW I merged it in Boosted v5 (and it was already in v4 since 4.4.0).

@patrickhlauke
Copy link
Member

an extra effort that we couldn't afford ATM

meanwhile we're adding utility classes for every possible CSS property and value... oh well

@ffoodd
Copy link
Member Author

ffoodd commented Sep 8, 2020

I agree, but I think that what was the main argument is to add a dependency: since we dropped jQuery, only Popper remains.

I suggested the polyfill and was the only one in favor of this ATM. Nevertheless, the PR could be restored pretty easily if you dare since as I said, works fine in Boosted currently.

@ffoodd
Copy link
Member Author

ffoodd commented Sep 8, 2020

@patrickhlauke Just to be sure, are you in favor of using the WICG polyfill, or "simply" the CSS enhancement based on @supports()? Maybe if I try a PR without the polyfill, the team would be less relunctant.

@patrickhlauke
Copy link
Member

could try the @supports route, though not sure how verbose that would get. at least it would future-proof things.

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.

Restyle close icon's focus style
4 participants