-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Have document.hasFocus() and blur/focus events reveal system-level focus #6172
Conversation
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.
Wow, thank you so much for tackling this! This has long been a known problem with the spec, and I'm so glad you dove in to figure out how to fix it.
My biggest issue at this point is that I don't understand some of the cases where you're adding null checks. Clarifying those would be ideal. Perhaps even with notes or examples in the spec.
@domenic thanks for the review! Hoping to get to the remaining items in the next day or two. |
One thing we noticed is that |
More generally, @a4sriniv noted several interesting things in WICG/portals#257 (comment), which I'll quote here:
We don't need to capture all of this behavior in a single spec update, but let's try to make sure that every change is in the right direction. (Thus my worry about |
@domenic I think so, because it looks like ...and new chain is empty then. The only place we appear to clear it is in the Focus fixup rule, which doesn't apply here. |
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.
OK, I pushed some minor tweaks to line wrapping and wording, plus added an assert in one place where it wasn't immediately obvious why we were OK without a null check. I'm happy with this now!
We should try to have some tests before merging, if at all possible. Your demo page seems like it could be converted to an automated web platform test; are you up for doing that?
Thanks, I can take a stab at it. |
I'm confident enough about the test work in web-platform-tests/wpt#26845 that I'll go ahead and merge this spec change. We'll work together to ensure the tests land ASAP, but I think there's no more need to hold up the spec PR. Thanks again for the great work here on this longstanding problem. |
🙌🏼 |
Fixes #5049. ...to match implementations.
/interaction.html ( diff )
/semantics-other.html ( diff )