-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Editorial: change "show popover" to call "check popover validity" first #9439
Conversation
This is what implementations (at least Gecko and Chromium) do and it's analogous to what happens in "hide popover". Fixes whatwg#9429.
…how" Simplifies understanding the spec.
@@ -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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)).
Otherwise, the rendered page would contain a space character after the `.`.
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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). |
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. |
@nt1m @josepharhar just FYI, one might want to change this in WebKit and Chromium too. |
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
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
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 )