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.

Sign up for GitHub

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

don't enable polyfill if browser supports ":focus-visible" #259

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

don't enable polyfill if browser supports ":focus-visible" #259

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbb2f0e
check for a native :focus-visible implementation before trying to loa…
furudean Aug 26, 2021
627a283
oops
furudean Aug 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions src/focus-visible.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,10 +273,27 @@ function applyFocusVisiblePolyfill(scope) {
}
}

/**
* Returns true if :focus-visible is supported natively in the current
* environment, otherwise returns false.
*/
function supportsNativeFocusVisible() {
try {
document.querySelector(':focus-visible');
return true;
} catch (e) {
return false;
}
}

// It is important to wrap all references to global window and document in
// these checks to support server-side rendering use cases
// @see https://github.com/WICG/focus-visible/issues/199
if (typeof window !== 'undefined' && typeof document !== 'undefined') {
if (
typeof window !== 'undefined' &&
typeof document !== 'undefined' &&
!supportsNativeFocusVisible()
) {
// Make the polyfill helper globally available. This can be used as a signal
// to interested libraries that wish to coordinate with the polyfill for e.g.,
// applying the polyfill to a shadow root:
Expand All @@ -297,7 +314,7 @@ if (typeof window !== 'undefined' && typeof document !== 'undefined') {
window.dispatchEvent(event);
}

if (typeof document !== 'undefined') {
if (typeof document !== 'undefined' && !supportsNativeFocusVisible()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make this change we are literally requiring all users to double up their style declarations related to :focus-visible.

/* this won't work in browsers don't have native :focus-visible support */
.foo.focus-visible,
.foo:focus-visible {
  outline: 2px solid blue;
}

/* we need to do this instead */
.foo.focus-visible {
  outline: 2px solid blue;
}
.foo:focus-visible {
  outline: 2px solid blue;
}

Imagine if we have many focus visible styles for different elements and components, this will cause a huge pain.

Copy link
Author

@furudean furudean Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I naively went in and assumed most would want to author their code with :focus-visible instead of .focus-visible and then use something like csstools/postcss-focus-visible to process and duplicate the styles.

For the completely manual use case where you have to author your code with both, forcing those users to double up their styles can be disruptive. It would make :focus-visible way more seamless and usable for my use case.

I assume putting a config into this polyfill is out of the question? Maybe a different release which supports this behavior would be more appropriate if we can't work this into the same package.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to ping, do you have any remarks on the above? @Justineo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not talking about how we would author style code but as the feature detection is for runtime, we always need to ship the duplicated styles to our users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@furudean @Justineo
In this issue #244, @Westbrook propose to provide 2 files:

  • the current one without any change
  • a new one not applying automatically, to let the user set a condition.

If users keep the current polyfill, no change.
If users want to apply it conditionally, they can use the second one with warning about double CSS declaration.

This solution can conciliate both of use cases.

// Apply the polyfill to the global document, so that no JavaScript
// coordination is required to use the polyfill in the top-level document:
applyFocusVisiblePolyfill(document);
Expand Down