-
Notifications
You must be signed in to change notification settings - Fork 123
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
Is DOM ready necessary? #193
Comments
I think that seems reasonable to me. @alice any objections? |
This would be a backwards incompatible change if anyone is hanging styles off |
yeah good point. I'm fine doing a major version bump for it. |
Seems reasonable, thanks! |
I came here with this exact question, so glad to see someone else had it too :) I had a different use case in mind, and @jakearchibald 's mention of the lazy-loading use case leads me to think that it may not be sufficient to apply the class synchronously. The approach proposed in the original issue only works assuming that you have some knowledge that the polyfill might be loaded at all, and control over when it is loaded. Feel free to skip to the bottom if you just want to read my proposal to address this. Polyfill indeterminismIn some cases, you may not know if the polyfill will be applied at all. For example, when vending a standalone, re-usable component, you can ask the content author using the component to provide a polyfill, but you have no direct control over whether they ultimately do or do not. If they provide the polyfill, it could be loaded at any time during the lifecycle of the component, and the component cannot typically rely on direct coordination with the content author to know when this will happen. Now to compound the issue a bit, let's talk about Shadow DOM: If it were a perfect world we could rely on So, it would seem that the best way to transparently support this polyfill in a standalone custom element with a shadow root is to detect the presence of the polyfill somehow in The problem is ultimately a variation of the lazy loading problem, but with more constraints: if you don't know whether or not the polyfill will be loaded, when do you attempt to detect the presence (or absence) of the polyfill? Potential solutionsSo, to circle back around to the beginning: @jakearchibald 's suggested approach would be a big improvement, and simplify the problem. A component author could say in documentation that loading the polyfill synchronously is a hard requirement of the component. However, this is clearly at odd's with @jakearchibald 's stated intention to lazily load the polyfill. An improved approach would be to also dispatch a globally observable event after |
@cdata can you provide a bit more info on what use case you're trying to solve? Are you trying to provide alternative focus styles if you detect that the focus-visible polyfill is not in play?
I'm not sure how I feel about that since this is polyfilling a CSS feature and when Circling back to the first point, if there was an |
@robdodson my problem, at its core, is that there does not seem to be a way to use the polyfill with just CSS from a shadow root-encapsulated stylesheet. Perhaps I'm wrong, in which case maybe you can help talk me down :) In case I'm not wrong: A workaround that I started to explore is detecting the presence of the polyfill from JS (described in my original comment; see also how this leads to more coordination problems). If we are forced to delve into JS-detection of the polyfill, then there is very little added risk of breaking code that depends on an event.
Nor will there be a globally-applied CSS class. This polyfill gives us the correct mental model for using |
Please forgive my psuedocode: class FocusVisibleElement extends HTMLElement {
decorateForFocusVisiblePolyfill() {
const hasFocusVisiblePolyfill =
document.documentElement.classList.has('js-focus-visible');
// When this attribute is present, shadow root styles can be applied with the selector
// :host([data-js-focus-visible]) :focus:not(.focus-visible)
this.toggleAttribute('data-js-focus-visible', hasFocusVisiblePolyfill);
}
connectedCallback() {
this.decorateForFocusVisiblePolyfill();
self.addEventListener('js-focus-visible-init',
() => this.decorateForFocusVisiblePolyfill());
}
} |
It just occurred to me that I might be screwed for a completely different reason: this polyfill does not seem to be aware of shadow roots at all.
Since https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo does not explicitly rule-out shadow roots, it seems like the polyfill should care about e.g., composed event path and "deep" focus. |
Sorry, to circle back around once again: it looks like this polyfill doesn't (but should) care about focus traversal in shadow roots. To the extent that it does care in the future, it's possible that my use case will dissolve, but it depends on the extent to which the polyfill can be used without JS-based coordination from a Shadow DOM-using component. If JS-based coordination is required, then I would say that some kind of "init" event is important. |
We spent some time exploring Shadow DOM support but it kind of ran aground because when focus moves around inside of a shadow root, the focus event doesn't bubble out. Someone suggested adding a mixin that web component authors could use which might be a better approach than our document level listener. Unfortunately I haven't had time to dive back into the shadow dom stuff but would love help. |
@robdodson IMO the mixin approach sounds like a reasonable middle ground. It's better than the unbounded problem of observing focus throughout the composed tree. Naively, I might take the following approach:
Number 3 could be easily packaged as a mixin for anyone who prefers to consume it that way. Relatedly: some kind of "init" event is probably needed for a comprehensive approach that covers scenarios where the polyfill is lazy loaded. WDYT of this approach? |
For scoping, I definitely think #196 could be used to scope the polyfill as described in #126 and #46. The caveat is that #196 relies on the way encapsulation works at shadow root boundaries. So, anyone who wants to use it in their components would have to also use a shadow root. Reasonable people can disagree about whether Shadow DOM should be a requirement for scoping the polyfill, but my personal opinion is that the platform primitive designed for this use case is the shadow root. I suspect that the effort and scope of the change required to make scoping work would increase substantially if we wanted to support scoping at arbitrary subtrees in the same document (without the benefit of Shadow DOM semantics). |
btw with I/O coming up this week (and impending post-I/O vacation) I wanted to mention that I don't think I'd be able to land these changes until after May 20, unless Alice wants to take the lead on landing them. |
That's fine. We don't have a critical deadline for these changes. Of
course, we would like to be able to recommend the `:focus-visible` polyfill
to our users as soon as possible, but it's not going to kill us to wait a
couple of weeks.
…On Fri, May 3, 2019 at 10:21 AM Rob Dodson ***@***.***> wrote:
btw with I/O coming up this week (and impending post-I/O vacation) I
wanted to mention that I don't think I'd be able to land these changes
until after May 20, unless Alice wants to take the lead on landing them.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#193 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB2TU2FVMJKRM55CJJEB63PTRYCHANCNFSM4HGTGWQQ>
.
|
What is involved in landing the changes? |
Signing off on the code review and getting the tests working. |
There is no pressure on my side. I have a bookmark to take a look at the tests in IE soon, but it probably won't happen until I/O is behind us. |
The only line that seems to depend on some sort of DOM readiness is:
Could that be:
…so this script could be loaded lazily or async?
The text was updated successfully, but these errors were encountered: