Add imperative access to popover invoker, and connect popover/invoker to implicit anchor element #10728

mfreed7 · 2024-10-28T23:46:29Z

This includes two related changes:

The showPopover() and togglePopover() methods now include an options bag that allows setting the popover invoker implicitly. Previously this was only possible via the declarative popovertarget attribute.
Popover invokers (set declaratively or imperatively) now create an implicit anchor element reference for that popover.

This new behavior was resolved in the WHATWG/CSSWG/OpenUI joint task force meeting on June 26, 2024.

Closes #10442
Closes #10675

At least two implementers are interested (and none opposed):
- Chromium
- This behavior was resolved in the WHATWG/CSSWG/OpenUI joint task force meeting on June 26, 2024.
Tests are written and can be reviewed and commented upon at:
- https://wpt.fyi/results/html/semantics/popovers/imperative-invokers.tentative.html
- https://wpt.fyi/results/css/css-anchor-position/popover-implicit-anchor.tentative.html
Implementation bugs are filed:
- Chromium: https://crbug.com/364669918
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1928128
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=282337
MDN issue is filed: …
- Improvements to popover API and connection to anchor positioning mdn/mdn#598
The top of this comment includes a clear commit message to use.

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

/dom.html ( diff )
/infrastructure.html ( diff )
/popover.html ( diff )
/references.html ( diff )

domenic

Normative behavior looks good. A variety of editorial tweaks.

source

mfreed7

Thanks for the quick review! I think I got everything, LMK if I missed something.

source

domenic

Editorially LGTM (with one more nit). Let us know when all the boxes are checked.

source

mfreed7 · 2024-10-30T16:49:51Z

Editorially LGTM (with one more nit). Let us know when all the boxes are checked.

Thanks! Done. It would be good to get the attention of @annevk and @smaug----, since the implementer support checkbox relies on our joint resolution at the form controls task force. (And I therefore didn't want to clutter up the standards positions requests for this.)

source

This includes two related changes: 1. The `showPopover()` and `togglePopover()` methods now include an options bag that allows setting the popover invoker. 2. Popover invokers (declaratively or imperatively set) now create an implicit anchor reference for that popover. This new behavior was resolved in the [WHATWG/CSSWG/OpenUI joint task force meeting on June 26, 2024](whatwg#9144 (comment)). Closes whatwg#10442 Closes whatwg#10675

annevk · 2024-11-15T07:59:16Z

@mfreed7 OP doesn't include a link to a PR for renaming the tests away from .tentative. Have the tests also been updated to test source instead of invoker?

mfreed7 · 2024-11-15T17:26:19Z

@mfreed7 OP doesn't include a link to a PR for renaming the tests away from .tentative.

https://chromium-review.googlesource.com/c/chromium/src/+/6025803

There seems to be a chicken and egg problem with this. I've had folks complain about renaming .tentative too early (before the spec PR lands) and too late (after the spec PR lands).

Have the tests also been updated to test source instead of invoker?

web-platform-tests/wpt#49189

annevk · 2024-11-18T08:48:32Z

There seems to be a chicken and egg problem with this.

Ideally you create a PR against web-platform-tests that the specification editors can merge. That's how we have done this thus far.

The spec PR landed: whatwg/html#10728 so this removes .tentative from the WPTs. Bug: 364669918 Change-Id: I35e50fff8d94ff52e212844814a4e597c35d78b4

The spec PR landed: whatwg/html#10728 so this removes .tentative from the WPTs. Bug: 364669918 Change-Id: I35e50fff8d94ff52e212844814a4e597c35d78b4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6025803 Commit-Queue: Di Zhang <[email protected]> Reviewed-by: Di Zhang <[email protected]> Auto-Submit: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384325}

mfreed7 · 2024-11-18T17:22:11Z

There seems to be a chicken and egg problem with this.

Ideally you create a PR against web-platform-tests that the specification editors can merge. That's how we have done this thus far.

Yeah, ok. The Chromium process makes that a bit harder, since we're accustomed to doing the review for WPT stuff within our code review system, and it then gets pushed automatically. I don't know if there's an easy way to tell it not to merge the WPT PR when the Chromium PR gets merged. If you know of one, please let me know!

annevk · 2024-11-18T17:27:22Z

You don't have to use the Chromium process to create a WPT PR. You can just create a PR directly against the repo.

mfreed7 · 2024-11-18T17:36:48Z

You don't have to use the Chromium process to create a WPT PR. You can just create a PR directly against the repo.

Yeah. It's just a lot more work. 😄

annevk · 2024-11-18T17:41:24Z

That seems strange. It really shouldn't be.

mfreed7 · 2024-11-18T19:48:38Z

That seems strange. It really shouldn't be.

Only due to familiarity. I spend 99% of my time writing C++ code within Chromium's tooling, so venturing outside is painful. It's ok - I'll try to put up the rename PRs like this in the future.

…PTs, a=testonly Automatic update from web-platform-tests Remove .tentative from popover invoker WPTs The spec PR landed: whatwg/html#10728 so this removes .tentative from the WPTs. Bug: 364669918 Change-Id: I35e50fff8d94ff52e212844814a4e597c35d78b4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6025803 Commit-Queue: Di Zhang <[email protected]> Reviewed-by: Di Zhang <[email protected]> Auto-Submit: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384325} -- wpt-commits: 6572d5b33f7e95cb1f857ed76d2cdd7939ef47ad wpt-pr: 49225

…PTs, a=testonly Automatic update from web-platform-tests Remove .tentative from popover invoker WPTs The spec PR landed: whatwg/html#10728 so this removes .tentative from the WPTs. Bug: 364669918 Change-Id: I35e50fff8d94ff52e212844814a4e597c35d78b4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6025803 Commit-Queue: Di Zhang <dizhanggchromium.org> Reviewed-by: Di Zhang <dizhanggchromium.org> Auto-Submit: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1384325} -- wpt-commits: 6572d5b33f7e95cb1f857ed76d2cdd7939ef47ad wpt-pr: 49225 UltraBlame original commit: 73e41baa89bb5804a6fb8a776bfe270b6204e7db

…PTs, a=testonly Automatic update from web-platform-tests Remove .tentative from popover invoker WPTs The spec PR landed: whatwg/html#10728 so this removes .tentative from the WPTs. Bug: 364669918 Change-Id: I35e50fff8d94ff52e212844814a4e597c35d78b4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6025803 Commit-Queue: Di Zhang <[email protected]> Reviewed-by: Di Zhang <[email protected]> Auto-Submit: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1384325} -- wpt-commits: 6572d5b33f7e95cb1f857ed76d2cdd7939ef47ad wpt-pr: 49225

…PTs, a=testonly Automatic update from web-platform-tests Remove .tentative from popover invoker WPTs The spec PR landed: whatwg/html#10728 so this removes .tentative from the WPTs. Bug: 364669918 Change-Id: I35e50fff8d94ff52e212844814a4e597c35d78b4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6025803 Commit-Queue: Di Zhang <dizhanggchromium.org> Reviewed-by: Di Zhang <dizhanggchromium.org> Auto-Submit: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1384325} -- wpt-commits: 6572d5b33f7e95cb1f857ed76d2cdd7939ef47ad wpt-pr: 49225 UltraBlame original commit: 5092cf8b763c8b49bd138427b54cb868786bab4d

smaug---- · 2024-12-11T12:17:00Z

That seems strange. It really shouldn't be.

Only due to familiarity. I spend 99% of my time writing C++ code within Chromium's tooling, so venturing outside is painful. It's ok - I'll try to put up the rename PRs like this in the future.

FWIW, similar here. Writing wpts is quite a bit easier on mozilla-central vs dealing with wpt repository explicitly.

This was referenced Oct 29, 2024

Add an imperative way to set popover invoker relationships #10442

Closed

The declarative popover invoker feature needs to automatically establish an implicit anchor element #10675

Closed

domenic added addition/proposal New features or enhancements topic: popover The popover attribute and friends labels Oct 29, 2024

domenic reviewed Oct 29, 2024

View reviewed changes

mfreed7 commented Oct 29, 2024

View reviewed changes

domenic approved these changes Oct 30, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

mfreed7 mentioned this pull request Oct 30, 2024

Improvements to popover API and connection to anchor positioning mdn/mdn#598

Open

1 task

annevk reviewed Nov 8, 2024

View reviewed changes

source Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

mfreed7 added 7 commits November 15, 2024 13:33

Slight cleanup

a4ea186

Address comments

11a3209

Remove todo

e35d6aa

invoker to source

a4eac71

Handle null options

68bd6c3

Missed one

5602678

domenic force-pushed the popover_anchor branch from 691b3fb to 5602678 Compare November 15, 2024 04:33

Set -> Let, wrapping, remove CSS URL version

660f130

domenic approved these changes Nov 15, 2024

View reviewed changes

domenic merged commit b4e1fb1 into whatwg:main Nov 15, 2024
2 checks passed

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

Remove .tentative from popover invoker WPTs web-platform-tests/wpt#49225

Merged

chrisdavidmills mentioned this pull request Dec 9, 2024

Add imperative invoker updates to popover API mdn/content#37133

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add imperative access to popover invoker, and connect popover/invoker to implicit anchor element #10728

Add imperative access to popover invoker, and connect popover/invoker to implicit anchor element #10728

mfreed7 commented Oct 28, 2024 •

edited by pr-preview bot

Loading

domenic left a comment

mfreed7 left a comment

domenic left a comment

mfreed7 commented Oct 30, 2024

annevk commented Nov 15, 2024

mfreed7 commented Nov 15, 2024

annevk commented Nov 18, 2024

mfreed7 commented Nov 18, 2024

annevk commented Nov 18, 2024

mfreed7 commented Nov 18, 2024

annevk commented Nov 18, 2024

mfreed7 commented Nov 18, 2024

smaug---- commented Dec 11, 2024

Add imperative access to popover invoker, and connect popover/invoker to implicit anchor element #10728

Add imperative access to popover invoker, and connect popover/invoker to implicit anchor element #10728

Conversation

mfreed7 commented Oct 28, 2024 • edited by pr-preview bot Loading

domenic left a comment

Choose a reason for hiding this comment

mfreed7 left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

mfreed7 commented Oct 30, 2024

annevk commented Nov 15, 2024

mfreed7 commented Nov 15, 2024

annevk commented Nov 18, 2024

mfreed7 commented Nov 18, 2024

annevk commented Nov 18, 2024

mfreed7 commented Nov 18, 2024

annevk commented Nov 18, 2024

mfreed7 commented Nov 18, 2024

smaug---- commented Dec 11, 2024

mfreed7 commented Oct 28, 2024 •

edited by pr-preview bot

Loading