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

josepharhar · 2023-03-17T19:38:48Z

The "auto popover list" is constructed by attributes set on buttons. When those buttons are modified, it can break connections in the list. This patch adds checks to spots where buttons can be modified in order to fix up the list by closing all popovers when a connection has been broken.

Fixes #8991

At least two implementers are interested (and none opposed):
- Chrome
- …
Tests are written and can be reviewed and commented upon at:
- https://wpt.fyi/results/html/semantics/popovers/popover-target-element-disabled.html
Implementation bugs are filed:
- Chromium: already implemented
- Gecko: …
- WebKit: …

/infrastructure.html ( diff )
/input.html ( diff )
/popover.html ( diff )

The "auto popover list" is constructed by attributes set on buttons. When those buttons are modified, it can break connections in the list. This patch adds checks to spots where buttons can be modified in order to fix up the list by closing all popovers when a connection has been broken.

nt1m

Thanks! I have a few questions.

nt1m · 2023-03-18T01:27:55Z

source

+   <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>


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.

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

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?

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).

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?

Is that the extent of your concern?

Yep :)

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

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.

I created a PR here: whatwg/dom#1176

source

nt1m · 2023-03-18T01:50:41Z

source

+      <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>


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.

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

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?

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.

ok I added a note, how does it look?

source

If the popovertarget attribute is changed on an element in a way which breaks the connection keeping multiple popovers open, then this patch will close all open popovers. This was pointed out here: whatwg/html#9048 (comment) Bug: 1307772, 1408546 Change-Id: I0a42260e97b7ef3fde01f416ef090b50994b8c5d

If the popovertarget attribute is changed on an element in a way which breaks the connection keeping multiple popovers open, then this patch will close all open popovers. This was pointed out here: whatwg/html#9048 (comment) Bug: 1307772, 1408546 Change-Id: I0a42260e97b7ef3fde01f416ef090b50994b8c5d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4354171 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120013}

If the popovertarget attribute is changed on an element in a way which breaks the connection keeping multiple popovers open, then this patch will close all open popovers. This was pointed out here: whatwg/html#9048 (comment) Bug: 1307772, 1408546 Change-Id: I0a42260e97b7ef3fde01f416ef090b50994b8c5d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4354171 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120013}

nt1m · 2023-03-21T23:31:19Z

Looks good to me, will leave it to @annevk for a final look, and for the before attribute change vs after attribute change issue.

nt1m · 2023-03-24T00:27:07Z

WebKit implementation bug: https://bugs.webkit.org/show_bug.cgi?id=254382

At least in chromium, synchronous internal listeners for attribute changes which are run as "attribute change steps" are run after the attribute is actually changed, not before. This affects popover attribute related algorithms in HTML: whatwg/html#9048 (comment)

If the popovertarget attribute is changed on an element in a way which breaks the connection keeping multiple popovers open, then this patch will close all open popovers. This was pointed out here: whatwg/html#9048 (comment) Bug: 1307772, 1408546 Change-Id: I0a42260e97b7ef3fde01f416ef090b50994b8c5d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4354171 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120013}

https://bugs.webkit.org/show_bug.cgi?id=254382 Reviewed by Tim Nguyen. Implement "check and possibly close popover stack" algorithm as specified here: whatwg/html#9048 This patch also imports the latest version of popover-target-element-disabled.html. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-target-element-disabled-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-target-element-disabled.html: * Source/WebCore/html/HTMLElement.cpp: (WebCore::HTMLElement::checkAndPossiblyClosePopoverStackInternal): * Source/WebCore/html/HTMLElement.h: (WebCore::HTMLElement::checkAndPossiblyClosePopoverStack): * Source/WebCore/html/HTMLFormControlElement.cpp: (WebCore::HTMLFormControlElement::removedFromAncestor): (WebCore::HTMLFormControlElement::parseAttribute): (WebCore::HTMLFormControlElement::disabledStateChanged): (WebCore::HTMLFormControlElement::didChangeForm): * Source/WebCore/html/HTMLFormControlElement.h: * Source/WebCore/html/HTMLInputElement.cpp: (WebCore::HTMLInputElement::updateType): Canonical link: https://commits.webkit.org/262440@main

If the popovertarget attribute is changed on an element in a way which breaks the connection keeping multiple popovers open, then this patch will close all open popovers. This was pointed out here: whatwg/html#9048 (comment) Bug: 1307772, 1408546 Change-Id: I0a42260e97b7ef3fde01f416ef090b50994b8c5d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4354171 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120013}

…ttribute changes, a=testonly Automatic update from web-platform-tests Close popover stack when popovertarget attribute changes If the popovertarget attribute is changed on an element in a way which breaks the connection keeping multiple popovers open, then this patch will close all open popovers. This was pointed out here: whatwg/html#9048 (comment) Bug: 1307772, 1408546 Change-Id: I0a42260e97b7ef3fde01f416ef090b50994b8c5d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4354171 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1120013} -- wpt-commits: 699803ffbe61479119ee4dd8622635b6b0ef715e wpt-pr: 39083

Fixes whatwg#9160 If this patch is merged, then whatwg#9048 is obsolete and can be closed without merging. Rationale (copied from [masons patch](https://chromium-review.googlesource.com/c/chromium/src/+/4429412)): Instead of using just the DOM structure to establish the popover hierarchy, the user's behavior should matter. For example, if one popover contains a popover invoker pointing to another popover, it should matter whether that invoker is *actually used* to open the second popover. An example: - Component 1 is a third party widget, which uses popover - Component 2 is another third party widget, also using popover - A page wants to use both components separately, from separate invoking buttons. - Component 1 also wants to be able to use Component 2, via a button within Component 1. In this example, the page should be able to still independently use these components. So a user clicking the page's button for Component 2 is expected to close Component 1 if it's open, because that's a direct invocation of Component 2. However, if the user clicks the button within Component 1 to get Component 2, it is natural to leave Component 1 open because this is a nested call. Important note: this often happens to be the behavior even before this change, since the user clicking on the page-level Component 2 invoking button represents a light dismiss signal for Component 1, so it closes either way. But this patch simplifies the implementation, removing the need to track all invokers on the page, and also removing the need to continuously check whether invoker relationships have changed.

josepharhar · 2023-04-14T23:16:55Z

If #9171 gets merged, then it will make this PR obsolete and I will close this

annevk · 2023-04-20T14:41:44Z

source

+   <li><p>If <var>localName</var> is <code data-x="attr-fe-disabled">disabled</code>, <code
+   data-x="attr-fae-form">form</code>, or <code data-x="attr-popovertarget">popovertarget</code>,
+   then run <span>check and possibly close popover stack</span> given <var>element</var>'s
+   <span>node document</span>.</p></li>


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.

In all implementations, internal listeners for attribute changes which are run as "attribute change steps" are run after the attribute is actually changed, not before. This affects popover attribute related algorithms in HTML: whatwg/html#9048 (comment).

josepharhar mentioned this pull request Mar 17, 2023

WPT tests for unspecified PopoverInvokerElement removing steps #8991

Closed

nt1m reviewed Mar 18, 2023

View reviewed changes

josepharhar added 3 commits March 18, 2023 11:42

add popovertarget change check

c683d72

fix attr-popovertarget

df13f28

use hide all popovers

e1edb5d

chromium-wpt-export-bot mentioned this pull request Mar 20, 2023

Close popover stack when popovertarget attribute changes web-platform-tests/wpt#39083

Merged

add note

7e2eb2a

nt1m added the topic: popover The popover attribute and friends label Mar 22, 2023

josepharhar mentioned this pull request Mar 25, 2023

Handle attribute changes after changing attribute whatwg/dom#1176

Merged

3 tasks

rwlbuis mentioned this pull request Mar 28, 2023

[popover] Implement "check and possibly close popover stack" algorithm WebKit/WebKit#11996

Merged

josepharhar mentioned this pull request Apr 14, 2023

Only use the used invoker to establish popover hierarchy #9171

Merged

4 tasks

annevk reviewed Apr 20, 2023

View reviewed changes

annevk closed this in cf9625f May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

josepharhar commented Mar 17, 2023 •

edited by pr-preview bot

Loading

nt1m left a comment •

edited

Loading

nt1m Mar 18, 2023

josepharhar Mar 18, 2023

josepharhar Mar 18, 2023

nt1m Mar 18, 2023

josepharhar Mar 18, 2023

nt1m Mar 20, 2023

annevk Mar 22, 2023

josepharhar Mar 23, 2023

annevk Mar 24, 2023

josepharhar Mar 25, 2023

nt1m Mar 18, 2023

nt1m Mar 18, 2023

josepharhar Mar 18, 2023

nt1m Mar 20, 2023 •

edited

Loading

josepharhar Mar 21, 2023

nt1m commented Mar 21, 2023 •

edited

Loading

nt1m commented Mar 24, 2023

josepharhar commented Apr 14, 2023

annevk Apr 20, 2023

		<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>

		<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>

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

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

Conversation

josepharhar commented Mar 17, 2023 • edited by pr-preview bot Loading

nt1m left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nt1m Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nt1m commented Mar 21, 2023 • edited Loading

nt1m commented Mar 24, 2023

josepharhar commented Apr 14, 2023

Choose a reason for hiding this comment

josepharhar commented Mar 17, 2023 •

edited by pr-preview bot

Loading

nt1m left a comment •

edited

Loading

nt1m Mar 20, 2023 •

edited

Loading

nt1m commented Mar 21, 2023 •

edited

Loading