Simplify popover=hint behaviours & related spec fixes#12345
Simplify popover=hint behaviours & related spec fixes#12345jakearchibald wants to merge 24 commits intomainfrom
Conversation
|
I'll sort out the wrapping after reviews & changes. |
|
Btw https://github.com/domfarolino/specfmt is quite useful for the wrapping. |
|
@lukewarlow haha your deleted comment did reveal a bug! It wasn't the |
|
Edit: I've broken this out into #12355 av1.mp4@mfreed7 sorry for another video, but I don't think I'd be able to describe it well. What do you see as the intended behaviour here? https://random-stuff.jakearchibald.com/bug-repros/popover-show-on-hide-warning/ - here's the demo page. Edit: I'm not convinced my spec totally solves this either, as hiding a popover hides child popovers, which would include the new ones added, and they'd be hidden with events. Maybe. |
|
@mfreed7 can you create a PR for your tests so far? |
|
@keithamus and @mfreed7, hopefully this is it in terms of the behaviour. Aside from editorial stuff, I've changed:
On that last point, I was pretty heavy-handed, and applied it to all popovers, not just auto/hint. Otherwise, it gets tricky if the popover type changes. Thank you for all the reviews so far. Up for one more? I'll take a look at the tests. |
Thanks again for taking this on!
This bit is not yet represented in the WPTs, FYI. I don't think I can get to it today, but maybe. Let me know if you edit the WPTs in the meantime. All of the rest should be represented (modulo my note below).
I think that's fine. Exceptions are better than confusing situations - they lead to the well lit path.
I'll look right now.
Thanks! Note that this set of changes landed in Chromium, but hasn't been put up as a WPT PR yet. I think our infra is having a problem. I've pinged them. |
I think the new changes look good, modulo the one I re-opened and commented on. Otherwise, great! I'm getting started on the new/changed WPTs corresponding to the auto-downgrades-to-hint and "loop" prevention stuff. |
|
Complete aside: it's cool and it's also a problem that you've grabbed PR #12345. My LLM wrote that into a comment, and I just naturally assumed it hallucinated the number. |
Yes! It was really satisfying to grab #12345. Even more-so because, when I noticed the original issue was #12304, I thought "ohhhh, #12345 will be soon! I wonder what that will be". |
mfreed7
left a comment
There was a problem hiding this comment.
LGTM, and I'm working on putting up the corresponding WPT changes.
Ok, I've implemented the latest things in Chromium (auto downgrades to hint, and all of the togglepopover reentrancy stuff) and adjusted WPTs accordingly. Due to the first WPT being stuck in manual review, the next two can't even be uploaded as PRs yet. So just to track the changes in one place, here are all three sets:
|
|
@mfreed7 a lovely bit of teamwork! I'm happy for this to land if you are? |
Totally agree - thanks for raising these issues, explaining them so clearly, and then working through the solutions. This feature got better/cleaner as a result. LGTM to land this. |
This patch updates the popover implementation to align with recent HTML spec changes, specifically regarding reentrancy protection and nested popover types. See the conversation in the spec PR: whatwg/html#12345 1. Strict Reentrancy Protection: Documents now maintain a `popover_showing` boolean and a `popover_hiding_nesting_count` integer to prevent concurrent popover toggles. If `showPopover()` is called while a popover is actively showing or hiding, a `DOMExceptionCode::kInvalidStateError` is immediately thrown. Stack-allocated `ScopedPopoverShowing` and `ScopedPopoverHiding` RAII helpers have been added to safely manage this state during `ShowPopoverInternal` and `HidePopoverInternal`. 2. Downgrading `popover=auto` to `popover=hint`: Previously, attempting to open an `auto` popover nested inside a `hint` popover threw an `InvalidStateError`. The spec now dictates that the `auto` popover should be permitted to open but with its effective type downgraded to `hint`. `ShowPopoverInternal` has been updated to omit the exception and coerce the incoming popover into the `hint_stack` so it appropriately assumes hint light-dismissal behaviors. Check invariants in `HideAllPopoversUntil` have been relaxed to handle this downgraded state safely. Tests in `external/wpt/html/semantics/popovers` have been updated to reflect the new expected behaviors. Bug: 499019927 Change-Id: Ibd6af063310585b9044de63a06e36ffd79d011d4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7787494 Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1620601}
Co-authored-by: Copilot <copilot@github.com>
|
@annevk looks like we're happy for this to land unless there's anything I'm missing? There have been a few changes since you last reviewed (sorry), in case you want to review again. (omg @domfarolino thanks so much for |
|
@jakearchibald OP appears incomplete. Note that after each edit (including checking a checkbox so best to not use the UI for that) you have to wait for the bot to update it as it will overwrite you otherwise. Also a bit unclear what the status of the tests is. |
This comment (#12345 (comment)) is still the state of things. I've left reply comments on the first PR, and the second two are still queued up in Chromium's infra waiting to be uploaded. As far as I'm concerned, yes, the new WPTs are all good. But I'm biased - I wrote them. Perhaps it'd be best to land all three of those PRs (which given the review hangups will likely take days) as-is, and then we can sort out any nits about the resulting state of tests? |
Commit message:
(See WHATWG Working Mode: Changes for more details.)
Fixes #12304.
Fixes #12346.
Fixes #12355.
Fixes #12356.
/form-elements.html ( diff )
/infrastructure.html ( diff )
/interactive-elements.html ( diff )
/popover.html ( diff )