Skip to content
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

Set popover invoker immediately after changing popover's visibility state to "showing" #9481

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Set popover invoker immediately after changing popover's visibility state to "showing" #9481

merged 2 commits into from
Jul 20, 2023

Conversation

mbrodesser-Igalia
Copy link
Member

@mbrodesser-Igalia mbrodesser-Igalia commented Jul 3, 2023 •
edited by pr-preview bot
Loading

@mbrodesser-Igalia mbrodesser-Igalia added the topic: popover The popover attribute and friends label Jul 3, 2023
@mbrodesser-Igalia
Copy link
Member Author

Test-coverage: https://github.com/web-platform-tests/wpt/blob/b87174e39fd3a502910430752d7f9fce233b1b35/html/semantics/popovers/popover-invoker-reset.html violated an assertion before, now it doesn't anymore, see #9383 (comment).

josepharhar
josepharhar approved these changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@josepharhar josepharhar left a comment

Choose a reason for hiding this comment

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

This matches what I am implementing in chromium. Thanks for the PR!

@josepharhar
Copy link
Contributor

Corresponding change is being merged in chromium: https://chromium-review.googlesource.com/c/chromium/src/+/4657399

@mbrodesser-Igalia
Copy link
Member Author

CC @nt1m @rwlbuis

@annevk
Copy link
Member

annevk commented Jul 10, 2023

This seems like a normative change which normally has a PR template. How can be sure implementations will align with this change? I guess there's already a test which contradicts the current specification?

@josepharhar
Copy link
Contributor

I guess there's already a test which contradicts the current specification?

Yes, the current assertion in the spec about the invoker not being set will be hit. Presumably implementations have not added that assertion yet, because if they did, they would be hitting it without this PR

@mbrodesser-Igalia
Copy link
Member Author

This seems like a normative change which normally has a PR template. How can be sure implementations will align with this change? I guess there's already a test which contradicts the current specification?

@annevk: Yes, there's a test which currently violates an assertion, see #9383 (comment).

@annevk annevk merged commit bf60fc7 into whatwg:main Jul 20, 2023
@YusukeHirao YusukeHirao mentioned this pull request Sep 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josepharhar josepharhar josepharhar approved these changes

@annevk annevk annevk 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.

Should "show popover" assert the element's popover invoker is null?
3 participants
@mbrodesser-Igalia @josepharhar @annevk