Skip to content

Add "check and possibly close popover stack" algorithm #9048

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

Closed
josepharhar wants to merge 5 commits into from
Closed

Add "check and possibly close popover stack" algorithm #9048

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
44ca874
Add "check and possibly close popover stack" algorithm
josepharhar Jan 18, 2023
c683d72
add popovertarget change check
josepharhar Mar 18, 2023
df13f28
fix attr-popovertarget
josepharhar Mar 20, 2023
e1edb5d
use hide all popovers
josepharhar Mar 20, 2023
7e2eb2a
add note
josepharhar Mar 21, 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
40 changes: 40 additions & 0 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1828,6 +1828,10 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li><p>If <var>removedNode</var>'s <code data-x="attr-popover">popover</code> attribute is not in
the <span data-x="attr-popover-none-state">no popover state</span>, then run the <span>hide
popover algorithm</span> given <var>removedNode</var>, false, false, and false.</p></li>

<li><p>If <var>removedNode</var> is a <span data-x="concept-button">button</span>, then run
<span>check and possibly close popover stack</span> given <var>removedNode</var>'s <span>node
document</span>.</p></li>
</ol>

<p>A <dfn id="insert-an-element-into-a-document" data-x="node is inserted into a document"
Expand Down Expand Up @@ -47024,6 +47028,9 @@ interface <dfn interface>HTMLInputElement</dfn> : <span>HTMLElement</span> {
element's <span data-x="concept-textarea/input-cursor">text entry cursor position</span> to the
beginning of the text control, and <span data-x="set the selection direction">set its selection
direction</span> to "<code data-x="">none</code>".</p></li>

<li><p>Run <span>check and possibly close popover stack</span> given the element's <span>node
document</span>.</p></li>
Comment thread
josepharhar marked this conversation as resolved.
</ol>

</div>
Expand Down Expand Up @@ -81861,6 +81868,10 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
<ol>
<li><p>If <var>namespace</var> is not null, then return.</p></li>

<li><p>If <var>localName</var> is <code data-x="attr-fe-disabled">disabled</code> or <code
data-x="attr-fae-form">form</code>, then run <span>check and possibly close popover stack</span>
Copy link
Copy Markdown
Member

@nt1m nt1m Mar 18, 2023

Choose a reason for hiding this comment

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

If someone changes the popovertarget to a different element, should we run the algorithm? We're also effectively breaking the ancestor relationship in that case.

Though the algorithm below would probably no longer work, since the popovertarget attribute is used to get the topmost popover ancestor.

Copy link
Copy Markdown
Contributor Author

@josepharhar josepharhar Mar 18, 2023

Choose a reason for hiding this comment

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

I agree! I pushed a commit to add popovertarget here, and I started on chromium and WPT changes

Copy link
Copy Markdown
Contributor Author

@josepharhar josepharhar Mar 18, 2023

Choose a reason for hiding this comment

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

Though the algorithm below would probably no longer work, since the popovertarget attribute is used to get the topmost popover ancestor.

I don't understand, the rest of this algorithm only cares if the popover attribute is changed, right?

Copy link
Copy Markdown
Member

@nt1m nt1m Mar 18, 2023

Choose a reason for hiding this comment

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

I'm not sure if "check and possibly close popover stack" runs before or after the attribute change, but this bit relies on checking the invokers:

If the result of running topmost popover ancestor

So if the popovertarget changes, the tree will end up different depending on when this runs, but disabled/form are also similarly affected (since we use the popover target element algorithm).

Copy link
Copy Markdown
Contributor Author

@josepharhar josepharhar Mar 18, 2023

Choose a reason for hiding this comment

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

I'm not sure if "check and possibly close popover stack" runs before or after the attribute change

In chromium we are definitely doing it after the attribute has been changed. After reading https://dom.spec.whatwg.org/#concept-element-attributes-change it looks like these steps actually run before the attribute has been changed! Maybe we should create an alternative in the DOM spec which runs after the attribute is changed instead of before.

Is that the extent of your concern?

Copy link
Copy Markdown
Member

@nt1m nt1m Mar 20, 2023

Choose a reason for hiding this comment

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

Is that the extent of your concern?

Yep :)

Copy link
Copy Markdown
Member

@annevk annevk Mar 22, 2023

Choose a reason for hiding this comment

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

Hmm. Do engines have callbacks before and after or is the DOM Standard wrong here in doing it before?

Copy link
Copy Markdown
Contributor Author

@josepharhar josepharhar Mar 23, 2023

Choose a reason for hiding this comment

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

Based on my brief searching, it looks like in chromium we only look at attribute changes after they happen, not before they happen. I'd be in favor of just changing element attribute changes to run after instead of having two algorithms/definitions.

Copy link
Copy Markdown
Member

@annevk annevk Mar 24, 2023

Choose a reason for hiding this comment

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

Agreed.

Copy link
Copy Markdown
Contributor Author

@josepharhar josepharhar Mar 25, 2023

Choose a reason for hiding this comment

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

I created a PR here: whatwg/dom#1176

given <var>element</var>'s <span>node document</span>.</p></li>

Copy link
Copy Markdown
Member

@annevk annevk Apr 20, 2023

Choose a reason for hiding this comment

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

I don't think this is correct for form and possibly popovertarget. This algorithm runs whenever the value is set, even when it's set to the same value. Presumably when it's set to the same value we don't want to invalidate.

<li><p>If <var>localName</var> is not <code data-x="attr-popover">popover</code>, then
return.</p></li>

Expand Down Expand Up @@ -82411,6 +82422,35 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
<li><p>Return true.</p></li>
</ol>

<p>To <dfn>check and possibly close popover stack</dfn> for a <code>Document</code>
<var>document</var>:</p>

<ol>
<li><p>Let <var>stack</var> be <var>document</var>'s <span>auto popover list</span>.</p></li>

<li><p>Let <var>index</var> <var>stack</var>'s <span data-x="list size">size</span> - 1.</p></li>

<li>
<p><span>While</span> <var>index</var> is greater than 0:</p>

<ol>
<li>
<p>If the result of running <span>topmost popover ancestor</span> given
<var>stack</var>[<var>index</var>] is not <var>stack</var>[<var>index</var> - 1], then:</p>
Comment on lines +82438 to +82439
Copy link
Copy Markdown
Member

@nt1m nt1m Mar 18, 2023

Choose a reason for hiding this comment

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

I wonder if we can do something more simple, like pass the popoverTargetElement then run hide all popovers until popoverTargetElement if it's an auto popover.

I'm having a bit of trouble understanding why this is the right thing to do.

Copy link
Copy Markdown
Member

@nt1m nt1m Mar 18, 2023

Choose a reason for hiding this comment

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

@josepharhar Can you explain what this "check and possibly close popover stack" algorithm does in terms of user experience? Like which cases it will be no-op, which cases it will hide all popovers, this is mostly the part I have trouble wrapping my head around.

Copy link
Copy Markdown
Contributor Author

@josepharhar josepharhar Mar 18, 2023

Choose a reason for hiding this comment

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

Can you explain what this "check and possibly close popover stack" algorithm does in terms of user experience?

Check and possibly close popover stack recalculates the popover stack being built from the relationships of the popovers to each other in the DOM and from the popover target attributes and reconciles it with the currently open popovers. If they don't match up, it closes all popovers because something must have been modified which messed up the relationships.

Like which cases it will be no-op, which cases it will hide all popovers, this is mostly the part I have trouble wrapping my head around.

When setting disabled in this case, check and possibly close popover stack will be called but no popovers will be closed because outerpopover and innerpopover are associated by their parent-child relationship in the DOM.

<div id=outerpopover popover=auto>
  <div id=innerpopover popover=auto>hello world</div>
</div>
<button id=mybutton>button</button>
<script>
outerpopover.showPopover();
innerpopover.showPopover(); // both can be open since outerpopover is an ancestor
mybutton.disabled = true; // doesn't close any popovers
</script>

However, if we make the button the thing which creates the relationship between outerpopover and innerpopover and allows them to both be open at the same time and then take that away, it will close all popovers:

<div id=outerpopover popover=auto>
  <button id=mybutton popovertarget=innerpopover>toggle popover</button>
</div>
<div id=innerpopover popover=auto>hello world</div>
<script>
outerpopover.showPopover();
innerpopover.showPopover(); // both can be open thanks to mybutton
mybutton.removeAttribute('popovertarget'); // all popovers will be closed since we broke the connection
</script>

There are additional test cases in the WPT. Does this help?

Copy link
Copy Markdown
Member

@nt1m nt1m Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, thanks! seems like this is symmetric with the auto popovers hidden by the show popover algo as well, perhaps a note linking the two would be helpful.

Copy link
Copy Markdown
Contributor Author

@josepharhar josepharhar Mar 21, 2023

Choose a reason for hiding this comment

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

ok I added a note, how does it look?


<ol>
<li><p>Run <span data-x="hide-all-popovers-until">hide all popovers until</span> given
<var>document</var>, false, and false.</p></li>
Comment thread
josepharhar marked this conversation as resolved.
Outdated

<li><p>Return.</p></li>
</ol>
</li>

<li><p>Set <var>index</var> to <var>index</var> - 1.</p></li>
</ol>
</li>
</ol>

<h4>The popover target attributes</h4>

<p><span data-x="concept-button">Buttons</span> may have the following content attributes:</p>
Expand Down