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.

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

Editorial: change "show popover" to call "check popover validity" first #9439

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

mbrodesser-Igalia
Copy link
Member

@mbrodesser-Igalia mbrodesser-Igalia commented Jun 19, 2023

This is what implementations (at least Gecko and Chromium) do and it's analogous to what happens in "hide popover".

Fixes #9429.


/popover.html ( diff )

This is what implementations (at least Gecko and Chromium) do and it's analogous to
what happens in "hide popover".

Fixes whatwg#9429.
@mbrodesser-Igalia mbrodesser-Igalia changed the title nested show not initialized Change "show popover" to call "check popover validity" first Jun 19, 2023
@mbrodesser-Igalia mbrodesser-Igalia added the topic: popover The popover attribute and friends label Jun 19, 2023
@@ -82457,12 +82457,23 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
element</span> or null <var>invoker</var>:</p>

<ol>
<li><p>If the result of running <span>check popover validity</span> given <var>element</var>,
false, <var>throwExceptions</var>, and null is false, then return.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

This change means that "popover showing or hiding" is no longer set to false. Is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's what implementations (Gecko and Chromium at least) do anyway.

Be ware that "check popover validity" might need to be adapted as part of whatwg/dom#1185 (see whatwg/dom#1185 (comment) and whatwg/dom#1185 (comment)).

source Show resolved Hide resolved
Otherwise, the rendered page would contain a space character after the
`.`.
@annevk annevk requested review from nt1m and rwlbuis June 19, 2023 12:24
@nt1m nt1m requested review from josepharhar and mfreed7 June 24, 2023 05:31
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.

Looks OK to me

Copy link

@rwlbuis rwlbuis left a comment

Choose a reason for hiding this comment

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

LGTM.

@mbrodesser-Igalia
Copy link
Member Author

For the record: calling "check popover validity" first is not observable by tests, because https://html.spec.whatwg.org/#popover-invoker is only read when popover's visibility state is showing, or when indirectly read from https://html.spec.whatwg.org/#show-popover via https://html.spec.whatwg.org/#topmost-popover-ancestor which happens after "check popover validity" (with and without the patch).

@annevk
Copy link
Member

annevk commented Jun 29, 2023

So to clarify, this is an editorial change and I should reflect that in the eventual commit message, right?

@mbrodesser-Igalia
Copy link
Member Author

mbrodesser-Igalia commented Jun 29, 2023

So to clarify, this is an editorial change and I should reflect that in the eventual commit message, right?

It's a syntactical correction, the semantics didn't change. In that sense, yes.

@annevk annevk merged commit 66e162f into whatwg:main Jun 29, 2023
@annevk annevk changed the title Change "show popover" to call "check popover validity" first Editorial: change "show popover" to call "check popover validity" first Jun 29, 2023
@mbrodesser-Igalia
Copy link
Member Author

@nt1m @josepharhar just FYI, one might want to change this in WebKit and Chromium too.

webkit-commit-queue pushed a commit to rwlbuis/WebKit that referenced this pull request Jul 3, 2023
https://bugs.webkit.org/show_bug.cgi?id=258800

Reviewed by Tim Nguyen.

Adjust to spec change:
whatwg/html#9439

There should be no behavior change that can be tested.

* Source/WebCore/html/HTMLElement.cpp:
(WebCore::HTMLElement::showPopover):

Canonical link: https://commits.webkit.org/265734@main
mnutt pushed a commit to movableink/webkit that referenced this pull request Jul 18, 2023
https://bugs.webkit.org/show_bug.cgi?id=258800

Reviewed by Tim Nguyen.

Adjust to spec change:
whatwg/html#9439

There should be no behavior change that can be tested.

* Source/WebCore/html/HTMLElement.cpp:
(WebCore::HTMLElement::showPopover):

Canonical link: https://commits.webkit.org/265734@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: popover The popover attribute and friends
Development

Successfully merging this pull request may close these issues.

nestedShow is not initialized in https://html.spec.whatwg.org/#show-popover
4 participants