Correct popover invokers #8993

annevk · 2023-03-08T00:26:49Z

They need to look within the current tree, not document. There's also no need for an HTMLCollection.

Tests: ?

At least two implementers are interested (and none opposed):
- WebKit
- Chromium (I think Chromium already does this, but please correct me if wrong)
Tests are written and can be reviewed and commented upon at:
- Collect popover invokers on root instead of document web-platform-tests/wpt#38981
Implementation bugs are filed:
- Chromium: N/A
- Gecko: N/A
- WebKit: N/A
MDN issue is filed: N/A

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

They need to look within the current tree, not document. There's also no need for an HTMLCollection. Tests: ? Fixes #8989.

mfreed7

LGTM. The existing spec is how Chromium implements this, but the result will be the same after this PR, and it's easier to understand.

annevk · 2023-03-09T00:02:57Z

@mfreed7 this is a normative change. Root vs document is not the same when shadow trees are involved. Do we have tests for this?

mfreed7 · 2023-03-13T23:47:54Z

@mfreed7 this is a normative change. Root vs document is not the same when shadow trees are involved. Do we have tests for this?

Ahh! Yes I missed that nuance, thanks for checking. So that is a difference from Chromium's implementation, in the case that nested sets of popovers are all inside a shadow root:

<div>
  <template shadowrootmode=open>
    <button popovertarget=p1>Click</button>
    <div popover id=p1>Popover 1
      <button popovertarget=p2>Click</button>
    </div>
    <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div>
  </template>
</div>

There isn't a test for this case, since it's currently broken in Chromium. But I agree that the new behavior is better, I think. Are there cases that this will break, compared to the old behavior? I didn't think of any off hand, but I also feel like I could be missing one.

I'm happy to add a WPT for this, once we agree this is the right behavior.

annevk · 2023-03-14T11:47:01Z

I don't think so. Before this aspect of the specification wouldn't work inside a shadow tree. Now it does. Crossing the boundary still doesn't work, but that seems good. (This gives it parity with forms.)

Please add a WPT and then this can be merged.

See this HTML spec PR: whatwg/html#8993 It points out that it would be better to look within `root` rather than `document` for invokers. Otherwise this use case will be broken: <div> <template shadowrootmode=open> <button popovertarget=p1>Click</button> <div popover id=p1>Popover 1 <button popovertarget=p2>Click</button> </div> <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div> </template> </div> I.e. nested popovers entirely within a shadow tree. This CL fixes that behavior and adds a test. Bug: 1307772 Change-Id: I28521ec1008d43994ca738c5673da3b704d7ba9c

mfreed7 · 2023-03-14T15:01:44Z

I don't think so. Before this aspect of the specification wouldn't work inside a shadow tree. Now it does. Crossing the boundary still doesn't work, but that seems good. (This gives it parity with forms.)

Ok, sounds good, thanks.

Please add a WPT and then this can be merged.

web-platform-tests/wpt#38981

See this HTML spec PR: whatwg/html#8993 It points out that it would be better to look within `root` rather than `document` for invokers. Otherwise this use case will be broken: <div> <template shadowrootmode=open> <button popovertarget=p1>Click</button> <div popover id=p1>Popover 1 <button popovertarget=p2>Click</button> </div> <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div> </template> </div> I.e. nested popovers entirely within a shadow tree. This CL fixes that behavior and adds a test. Bug: 1307772 Change-Id: I28521ec1008d43994ca738c5673da3b704d7ba9c

See this HTML spec PR: whatwg/html#8993 It points out that it would be better to look within `root` rather than `document` for invokers. Otherwise this use case will be broken: <div> <template shadowrootmode=open> <button popovertarget=p1>Click</button> <div popover id=p1>Popover 1 <button popovertarget=p2>Click</button> </div> <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div> </template> </div> I.e. nested popovers entirely within a shadow tree. This CL fixes that behavior and adds a test. Bug: 1307772 Change-Id: I28521ec1008d43994ca738c5673da3b704d7ba9c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335444 Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1118671}

See this HTML spec PR: whatwg/html#8993 It points out that it would be better to look within `root` rather than `document` for invokers. Otherwise this use case will be broken: <div> <template shadowrootmode=open> <button popovertarget=p1>Click</button> <div popover id=p1>Popover 1 <button popovertarget=p2>Click</button> </div> <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div> </template> </div> I.e. nested popovers entirely within a shadow tree. This CL fixes that behavior and adds a test. Bug: 1307772 Change-Id: I28521ec1008d43994ca738c5673da3b704d7ba9c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335444 Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1118671}

… of document, a=testonly Automatic update from web-platform-tests Collect popover invokers on root instead of document See this HTML spec PR: whatwg/html#8993 It points out that it would be better to look within `root` rather than `document` for invokers. Otherwise this use case will be broken: <div> <template shadowrootmode=open> <button popovertarget=p1>Click</button> <div popover id=p1>Popover 1 <button popovertarget=p2>Click</button> </div> <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div> </template> </div> I.e. nested popovers entirely within a shadow tree. This CL fixes that behavior and adds a test. Bug: 1307772 Change-Id: I28521ec1008d43994ca738c5673da3b704d7ba9c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335444 Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1118671} -- wpt-commits: 887ed601ff82d1cee8a358559e2c963528e6520b wpt-pr: 38981

See this HTML spec PR: whatwg/html#8993 It points out that it would be better to look within `root` rather than `document` for invokers. Otherwise this use case will be broken: <div> <template shadowrootmode=open> <button popovertarget=p1>Click</button> <div popover id=p1>Popover 1 <button popovertarget=p2>Click</button> </div> <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div> </template> </div> I.e. nested popovers entirely within a shadow tree. This CL fixes that behavior and adds a test. Bug: 1307772 Change-Id: I28521ec1008d43994ca738c5673da3b704d7ba9c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335444 Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1118671}

Correct popover invokers

d7b8795

They need to look within the current tree, not document. There's also no need for an HTMLCollection. Tests: ? Fixes #8989.

annevk added needs tests Moving the issue forward requires someone to write tests topic: popover The popover attribute and friends labels Mar 8, 2023

annevk requested review from domenic, josepharhar and mfreed7 March 8, 2023 00:26

domenic approved these changes Mar 8, 2023

View reviewed changes

mfreed7 approved these changes Mar 8, 2023

View reviewed changes

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

Collect popover invokers on root instead of document web-platform-tests/wpt#38981

Merged

annevk merged commit dc81ba3 into main Mar 14, 2023

annevk deleted the annevk/popover-invokers branch March 14, 2023 15:26

annevk removed the needs tests Moving the issue forward requires someone to write tests label Mar 14, 2023

YusukeHirao mentioned this pull request Sep 18, 2023

Update Popover API markuplint/Markuplint#1152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct popover invokers #8993

Correct popover invokers #8993

annevk commented Mar 8, 2023 •

edited

mfreed7 left a comment

annevk commented Mar 9, 2023

mfreed7 commented Mar 13, 2023

annevk commented Mar 14, 2023

mfreed7 commented Mar 14, 2023

Correct popover invokers #8993

Correct popover invokers #8993

Conversation

annevk commented Mar 8, 2023 • edited

mfreed7 left a comment

Choose a reason for hiding this comment

annevk commented Mar 9, 2023

mfreed7 commented Mar 13, 2023

annevk commented Mar 14, 2023

mfreed7 commented Mar 14, 2023

annevk commented Mar 8, 2023 •

edited