Fix corner case invoker issue with popover nested inside invoker #10770

mfreed7 · 2024-11-16T01:54:47Z

In this situation:

<button popovertarget=foo>Activate
  <div popover id=foo>Clicking me shouldn't close me</div>
</button>

clicking the button properly activates the popover, however, clicking on the popover itself after that should not close the popover. It currently does, because the popover click bubbles to the <button> and activates the invoker, which toggles the popover closed.

This patch fixes that case by checking that the invoke event wasn't on the popover itself.

At least two implementers are interested (and none opposed):
- Chromium
- WebKit
Tests are written and can be reviewed and commented upon at:
- Remove .tentative from popover-nested-in-button test web-platform-tests/wpt#49241
Implementation bugs are filed:
- Chromium: https://crbug.com/379241451
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1932700
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=283494
MDN issue is filed:
- I don't believe this corner cases needs MDN documentation.
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

/form-elements.html ( diff )
/input.html ( diff )
/popover.html ( diff )

In this situation: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This patch fixes that case.

source

mfreed7

Thanks for the review!

source

In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c

In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: David Baron <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384498}

Spec PR: whatwg/html#10770

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: David Baron <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232

domenic

LGTM; will let @annevk review again if he's interested, and then we can merge when the template is filled out.

source

mfreed7 · 2024-11-21T17:29:13Z

LGTM; will let @annevk review again if he's interested, and then we can merge when the template is filled out.

Thanks! I filled out the rest of the template.

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: David Baron <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232

Spec PR: whatwg/html#10770

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <masonfchromium.org> Commit-Queue: Mason Freed <masonfchromium.org> Reviewed-by: David Baron <dbaronchromium.org> Cr-Commit-Position: refs/heads/main{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232 UltraBlame original commit: 1bc5ed8ce26d83713b264aae65068e3c8de79546

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: David Baron <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232

…-button test, a=testonly Automatic update from web-platform-tests Remove .tentative from popover-nested-in-button test Spec PR: whatwg/html#10770 -- wpt-commits: d8bd5c4ede4a4548950435d135e85ed52468778a wpt-pr: 49241

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: David Baron <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232

…-button test, a=testonly Automatic update from web-platform-tests Remove .tentative from popover-nested-in-button test Spec PR: whatwg/html#10770 -- wpt-commits: d8bd5c4ede4a4548950435d135e85ed52468778a wpt-pr: 49241

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <masonfchromium.org> Commit-Queue: Mason Freed <masonfchromium.org> Reviewed-by: David Baron <dbaronchromium.org> Cr-Commit-Position: refs/heads/main{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232 UltraBlame original commit: 9ef5f6532921cb390f6bb6a414f9d7a989e859d3

…-button test, a=testonly Automatic update from web-platform-tests Remove .tentative from popover-nested-in-button test Spec PR: whatwg/html#10770 -- wpt-commits: d8bd5c4ede4a4548950435d135e85ed52468778a wpt-pr: 49241 UltraBlame original commit: 8882e5746d1dc531c6d8054452b50f9ff4e5ca9a

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <masonfchromium.org> Commit-Queue: Mason Freed <masonfchromium.org> Reviewed-by: David Baron <dbaronchromium.org> Cr-Commit-Position: refs/heads/main{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232 UltraBlame original commit: 9ef5f6532921cb390f6bb6a414f9d7a989e859d3

…-button test, a=testonly Automatic update from web-platform-tests Remove .tentative from popover-nested-in-button test Spec PR: whatwg/html#10770 -- wpt-commits: d8bd5c4ede4a4548950435d135e85ed52468778a wpt-pr: 49241 UltraBlame original commit: 8882e5746d1dc531c6d8054452b50f9ff4e5ca9a

…ained within invokers, a=testonly Automatic update from web-platform-tests Implement new behavior for popovers contained within invokers In this case: ``` <button popovertarget=foo>Activate <div popover id=foo>Clicking me shouldn't close me</div> </button> ``` clicking the button properly activates the popover, however, clicking on the popover itself after that should **not** close the popover. It currently does because the popover click bubbles to the `<button>` and activates the invoker, which toggles the popover closed. This CL changes that behavior so that clicks on the popover in the case above no longer re-invoke the popover. Spec PR: whatwg/html#10770 Bug: 379241451 Change-Id: Iab67127c46a97a081a7818bfd917864729bf8b5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6026982 Auto-Submit: Mason Freed <masonfchromium.org> Commit-Queue: Mason Freed <masonfchromium.org> Reviewed-by: David Baron <dbaronchromium.org> Cr-Commit-Position: refs/heads/main{#1384498} -- wpt-commits: 50f7c0548260cdc2f11bface1816a283f9314de8 wpt-pr: 49232 UltraBlame original commit: 9ef5f6532921cb390f6bb6a414f9d7a989e859d3

…-button test, a=testonly Automatic update from web-platform-tests Remove .tentative from popover-nested-in-button test Spec PR: whatwg/html#10770 -- wpt-commits: d8bd5c4ede4a4548950435d135e85ed52468778a wpt-pr: 49241 UltraBlame original commit: 8882e5746d1dc531c6d8054452b50f9ff4e5ca9a

https://bugs.webkit.org/show_bug.cgi?id=283494 Reviewed by NOBODY (OOPS!). This patch matches the new spec behaviour for when a popover is inside its invoker. See whatwg/html#10770 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-nested-in-button-expected.txt: * Source/WebCore/html/HTMLButtonElement.cpp: (WebCore::HTMLButtonElement::defaultEventHandler): * Source/WebCore/html/HTMLFormControlElement.cpp: (WebCore::HTMLFormControlElement::handlePopoverTargetAction const): * Source/WebCore/html/HTMLFormControlElement.h: * Source/WebCore/html/HTMLInputElement.cpp: (WebCore::HTMLInputElement::defaultEventHandler):

https://bugs.webkit.org/show_bug.cgi?id=283494 Reviewed by Tim Nguyen. This patch matches the new spec behaviour for when a popover is inside its invoker. See whatwg/html#10770 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-nested-in-button-expected.txt: * Source/WebCore/html/HTMLButtonElement.cpp: (WebCore::HTMLButtonElement::defaultEventHandler): * Source/WebCore/html/HTMLFormControlElement.cpp: (WebCore::HTMLFormControlElement::handlePopoverTargetAction const): * Source/WebCore/html/HTMLFormControlElement.h: * Source/WebCore/html/HTMLInputElement.cpp: (WebCore::HTMLInputElement::defaultEventHandler): Canonical link: https://commits.webkit.org/287522@main

mfreed7 added 2 commits November 15, 2024 17:50

Fix nesting problem

81b5b7f

annevk reviewed Nov 18, 2024

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

Address comments

3f5cffe

mfreed7 commented Nov 18, 2024

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

chromium-wpt-export-bot mentioned this pull request Nov 18, 2024

Implement new behavior for popovers contained within invokers web-platform-tests/wpt#49232

Merged

mfreed7 added a commit to mfreed7/wpt that referenced this pull request Nov 18, 2024

Remove .tentative from popover-nested-in-button test

fa894a3

Spec PR: whatwg/html#10770

mfreed7 mentioned this pull request Nov 18, 2024

Remove .tentative from popover-nested-in-button test web-platform-tests/wpt#49241

Merged

domenic added normative change topic: popover The popover attribute and friends labels Nov 21, 2024

domenic approved these changes Nov 21, 2024

View reviewed changes

annevk reviewed Nov 21, 2024

View reviewed changes

source Show resolved Hide resolved

EventTarget->Node

4e75996

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 22, 2024

Remove .tentative from popover-nested-in-button test

d8bd5c4

Spec PR: whatwg/html#10770

annevk merged commit dac8ec1 into whatwg:main Nov 22, 2024
2 checks passed

mfreed7 mentioned this pull request Dec 3, 2024

[css-ui] select:hover and select:active styles w3c/csswg-drafts#11185

Open

lukewarlow mentioned this pull request Dec 7, 2024

Implement new popover inside invoker behaviour WebKit/WebKit#37592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix corner case invoker issue with popover nested inside invoker #10770

Fix corner case invoker issue with popover nested inside invoker #10770

mfreed7 commented Nov 16, 2024 •

edited by annevk

Loading

mfreed7 left a comment

domenic left a comment

mfreed7 commented Nov 21, 2024

Fix corner case invoker issue with popover nested inside invoker #10770

Fix corner case invoker issue with popover nested inside invoker #10770

Conversation

mfreed7 commented Nov 16, 2024 • edited by annevk Loading

mfreed7 left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

mfreed7 commented Nov 21, 2024

mfreed7 commented Nov 16, 2024 •

edited by annevk

Loading