-
Notifications
You must be signed in to change notification settings - Fork 124
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
don't enable polyfill if browser supports ":focus-visible" #259
base: main
Are you sure you want to change the base?
Conversation
@c-bandy - Can you join the WICG to appease the IPR bots? |
@@ -297,7 +314,7 @@ if (typeof window !== 'undefined' && typeof document !== 'undefined') { | |||
window.dispatchEvent(event); | |||
} | |||
|
|||
if (typeof document !== 'undefined') { | |||
if (typeof document !== 'undefined' && !supportsNativeFocusVisible()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes #237.
I have no idea if it is correct to skip firing the "ready" event if :focus-visible is supported. I think this would do what most users expect? Maybe a maintainer can pitch in on this.