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

Make togglePopover return a boolean #9393

Merged
merged 4 commits into from
Jun 20, 2023
Merged

Make togglePopover return a boolean #9393

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc2f307
Make togglePopover return a boolean
josepharhar Jun 6, 2023
96b0c28
split extra returns into extra steps
josepharhar Jun 7, 2023
a01d4a6
return the visibility state instead of force parameter
josepharhar Jun 7, 2023
1ad2969
semicolon
josepharhar Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11576,7 +11576,7 @@ interface <dfn interface>HTMLElement</dfn> : <span>Element</span> {
// The popover API
undefined <span data-x="dom-showPopover">showPopover</span>();
undefined <span data-x="dom-hidePopover">hidePopover</span>();
undefined <span data-x="dom-togglePopover">togglePopover</span>(optional boolean force);
boolean <span data-x="dom-togglePopover">togglePopover</span>(optional boolean force);
[<span>CEReactions</span>] attribute DOMString? <span data-x="dom-popover">popover</span>;
};

Expand Down Expand Up @@ -82424,7 +82424,8 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
data-x="dom-togglePopover">togglePopover</span>()</code></dt>

<dd>If the popover <var>element</var> is not showing, then this method shows it. Otherwise, this
method hides it.</dd>
method hides it. This method returns true if the popover is open after calling it, otherwise
false.</dd>
</dl>

<p>The <dfn method for="HTMLElement"><code data-x="dom-showPopover">showPopover()</code></dfn>
Expand Down Expand Up @@ -82665,10 +82666,14 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
<li><p>If <span>this</span>'s <span>popover visibility state</span> is <span
data-x="popover-showing-state">showing</span>, and <var>force</var> is not present or false, then
run the <span>hide popover algorithm</span> given <span>this</span>, true, true, and
true.</p></li>
true, then return false.</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.

Can we not end up in a situation where hide returns early without changing the state? E.g., if hide all popovers made some changes.

I think we have to return something at the end based on the current state, instead. This might need test coverage too as @rwlbuis's WebKit implementation didn't catch this. (Assuming I'm correct.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I made it return whether the popover is actually open instead of just returning true or false or force, and I updated the WPT


<li><p>Otherwise, if <var>force</var> is not present or true, then run <span>show popover</span>
given <span>this</span> true, and null.</p></li>
given <span>this</span> true, and null, then return true.</p></li>
josepharhar marked this conversation as resolved.
Show resolved Hide resolved

<li><p><span>Assert</span>: <var>force</var> is present.</p></li>

josepharhar marked this conversation as resolved.
Show resolved Hide resolved
<li><p>Return <var>force</var>.</p></li>
</ol>

<p>To <dfn data-x="hide-all-popovers-until">hide all popovers until</dfn>, given an <span
Expand Down