Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Move popover invoker reset later #11304

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
Merged
merged 1 commit into from
May 19, 2025
Merged

Move popover invoker reset later #11304

merged 1 commit into from
May 19, 2025

Conversation

mfreed7
Copy link
Contributor

@mfreed7 mfreed7 commented May 13, 2025 •
edited by pr-preview bot
Loading

Per #11246 (comment), this moves the reset of the popover invoker later in the "hide a popover" algorithm.

(See WHATWG Working Mode: Changes for more details.)

/popover.html ( diff )

@mfreed7
Move popover invoker reset later
5741a55 
Per whatwg#11246 (comment), this moves the reset of the popover invoker later in the "hide a popover" algorithm.
@mfreed7 mfreed7 mentioned this pull request May 13, 2025
Closed
aarongable pushed a commit to chromium/chromium that referenced this pull request May 13, 2025
@mfreed7
Delay clearing the popover invoker until after beforetoggle
9b98a81 
Since the `beforetoggle` event can be cancelled, the invoker
shouldn't be changed until after that event.

See the spec discussion here:

  whatwg/html#11246 (comment).

And the spec PR here:

  whatwg/html#11304

See my comment here, but I think this isn't observable/testable:

  whatwg/html#11246 (comment)

Bug: 364669918
Change-Id: Ia6cbd7b2839ec9b742e2883a49f45c2544260ce6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6520560
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1459712}
keithamus
keithamus approved these changes May 14, 2025
View reviewed changes
Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@domenic domenic added the topic: popover The popover attribute and friends label May 15, 2025
domenic
domenic approved these changes May 15, 2025
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's give this a day or two to see if people figure out how to test it, or if it's just editorial.

@nt1m
Copy link
Member

nt1m commented May 15, 2025

This PR made me realize that

Set element's implicit anchor element to null.

is currently ran only if fireEvents is true. That seems like an oversight and it should be done even when we don't fire events? e.g. I think you'd want it in the same place we reset the popover invoker in this PR. I don't really see a reason to only reset the implicit anchor element when events are fired.

@mfreed7
Copy link
Contributor Author

mfreed7 commented May 15, 2025 •
edited
Loading

is currently ran only if fireEvents is true. That seems like an oversight and it should be done even when we don't fire events? e.g. I think you'd want it in the same place we reset the popover invoker in this PR. I don't really see a reason to only reset the implicit anchor element when events are fired.

I 100% agree - see my comment here.

At least in Chromium, that's not actually where it gets cleared, for good reasons. I think the spec should change to match.

@domenic domenic merged commit 579fdba into whatwg:main May 19, 2025
2 checks passed
@domenic
Copy link
Member

domenic commented May 19, 2025

I've merged this now, and closed #11246 as part of doing so. Please track any other suggestions in new open bugs!

Gingeh added a commit to Gingeh/ladybird that referenced this pull request May 22, 2025
@Gingeh
LibWeb: Move popover invoker reset later
c6b2aeb 
See whatwg/html#11304
AtkinsSJ pushed a commit to LadybirdBrowser/ladybird that referenced this pull request May 22, 2025
@Gingeh @AtkinsSJ
LibWeb: Move popover invoker reset later
fbc56f7 
See whatwg/html#11304
gotlougit pushed a commit to gotlougit/ladybird that referenced this pull request May 29, 2025
@Gingeh @gotlougit
LibWeb: Move popover invoker reset later
0866c86 
See whatwg/html#11304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithamus keithamus keithamus approved these changes

@domenic domenic domenic approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@mfreed7 @nt1m @domenic @keithamus
Morty Proxy This is a proxified and sanitized view of the page, visit original site.