Skip to content

Assertion in "hide all popovers until" will fail when changing a showing "hint" popover to "auto" #10996

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

Jump to bottom
Closed
Gingeh opened this issue Feb 5, 2025 · 4 comments · Fixed by #11197
Closed

Assertion in "hide all popovers until" will fail when changing a showing "hint" popover to "auto" #10996

Gingeh opened this issue Feb 5, 2025 · 4 comments · Fixed by #11197
Assignees
@josepharhar @mfreed7
Labels
topic: popover The popover attribute and friends

Comments

@Gingeh
Copy link

Gingeh commented Feb 5, 2025

What is the issue with the HTML Standard?

Here's the problematic scenario:

<div popover="hint"></div>
<script>
const popover = document.querySelector("div");
popover.showPopover();
popover.setAttribute("popover","auto");
</script>

When the popover is shown, its opened in popover mode is set to "hint".
Upon changing the popover attribute:

  1. The popover attribute change steps say to run the hide popover algorithm.
  2. The hide popover steps eventually say If element's popover attribute is in the auto state or the hint state, then: Run hide all popovers until given element (step 7.1).
  3. The hide all popovers until steps eventually say If document's showing hint popover list contains endpoint: Assert: endpoint's popover attribute is in the hint state (step 6.1)
  4. That assertion fails because the popover's opened in popover mode is still "hint" (so it is in the document's showing hint popover list), but its popover attribute has been changed to "auto".
@Gingeh
Copy link
Author

Gingeh commented Feb 6, 2025

After thinking about this, I think the hide popover algorithm should be using the popover's opened in popover mode instead of the current popover state.

@domenic domenic added the agenda+ To be discussed at a triage meeting label Feb 6, 2025
@mfreed7
Copy link
Contributor

mfreed7 commented Feb 7, 2025

After thinking about this, I think the hide popover algorithm should be using the popover's opened in popover mode instead of the current popover state.

Yep, I think this is the proper fix. Chromium indeed doesn't look at the attribute value, but instead the popover mode.

@past past mentioned this issue Feb 7, 2025
Closed
@past past removed the agenda+ To be discussed at a triage meeting label Feb 7, 2025
@Gingeh Gingeh mentioned this issue Feb 11, 2025
Merged
@annevk annevk added the topic: popover The popover attribute and friends label Feb 21, 2025
@Gingeh
Copy link
Author

Gingeh commented Mar 18, 2025 •
edited
Loading

I'd love to submit a fix for this (and my other popover issues) but I can't contribute without publishing my full legal name, which I don't feel comfortable doing. (I believe it would also be legally dubious to give a patch to someone to submit on my behalf)
So I would really appreciate it if someone else could contribute these (fairly simple) fixes instead.

josepharhar added a commit to josepharhar/html that referenced this issue Apr 8, 2025
@josepharhar
Fix assertion in hide all popovers until
31fa377 
This PR prevents an assertion from being hit in hide all popovers until
by changing the conditions in which hide all popovers until gets called
by the hide popover algorithm to use the opened in popover mode instead
of looking at the current value of the popover attribute.

Fixes whatwg#10996
@josepharhar josepharhar mentioned this issue Apr 8, 2025
Merged
6 tasks
@josepharhar
Copy link
Contributor

Thanks for figuring this out! Here's a PR: #11197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@josepharhar josepharhar

@mfreed7 mfreed7

Labels
topic: popover The popover attribute and friends
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix assertion in hide all popovers until josepharhar/html
6 participants
@past @domenic @annevk @josepharhar @mfreed7 @Gingeh