Skip to content
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.

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

[Popup] The definition of "ancestral element" might have some issues #463

Closed
mfreed7 opened this issue Mar 19, 2021 · 2 comments
Closed
Assignees

Comments

@mfreed7
Copy link

mfreed7 commented Mar 19, 2021

In implementing the updated proposal which includes the "popup" attribute, I've come across some issues with the way "ancestral elements" are defined and used. For reference, here's that definition:

Only one “top-level” popup may be displayed at a time. When a popup is shown and placed on the stack, it will remove all popups from the stack until it encounters an “ancestral” popup, or the list is empty. In this way, the user agent will ensure that only one popup and its child popups are ever displayed at a time—even across uncoordinated code.

The following would be considered an “ancestral” popup:

  • A popup ancestor of the new popup’s invoking element (based on the popup attribute)
  • A popup ancestor of the new popup’s anchoring element (based on the anchor attribute)
  • A popup ancestor of the new popup itself

My issues/questions:

  1. First, "ancestral popup" (vs. "ancestral element") seems to help at least my own understanding just a bit.
  2. Next, in just trying to wrap my head around the usage of nested popups, I am struggling to see what type of DOM hierarchy will be "typical" for nested popups. As the explainer has it now, the new popup can have an ancestor: a) directly, b) through its single anchor element, or c) through any of its invoking elements. I can definitely understand a). I can sort of see b), if you have some element that has a popup attached, and that element also contains another sub-popup. I'm struggling to see the use case for c). I'm not saying there isn't one, I'd just like people to chime in with good examples here. Because...
  3. Implementation of c) from above is a bit tricky. Essentially, when a new dialog is shown, or when light-dismiss is happening, there's a parent-tree-walk that needs to happen, walking up from either the new dialog or the element that was clicked to start light-dismiss. In this tree walk, we can directly look for parent popups (a). We can easily look for anchor elements (b), because those point backwards from popups, and we only need to check the already-open popups in the popup stack. But (c) is tough. There can be many invoking elements, and the id-refs can easily change through subtle DOM tree mutations (e.g. re-ordering a tree containing repeated ids). This isn't to say this part isn't implementable, but it certainly feels more error prone and tricky to get a performant implementation.

Basically, given #3, I'm really curious to hear use cases for #2. If there aren't any really good ones, I'd lobby to remove c) entirely.

@mfreed7 mfreed7 added the Popup label Mar 19, 2021
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 24, 2021
With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2021
With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2021
With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}
pull bot pushed a commit to luojiguicai/chromium that referenced this issue Mar 29, 2021
With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 2, 2021
…n up ancestor walk, a=testonly

Automatic update from web-platform-tests
Add "popup" invoking attribute, and clean up ancestor walk

With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}

--

wpt-commits: bdd414e6a1e9b4ac92cbe7f0d0dd26743ebdf674
wpt-pr: 28222
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 2, 2021
…n up ancestor walk, a=testonly

Automatic update from web-platform-tests
Add "popup" invoking attribute, and clean up ancestor walk

With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}

--

wpt-commits: bdd414e6a1e9b4ac92cbe7f0d0dd26743ebdf674
wpt-pr: 28222
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 7, 2021
…n up ancestor walk, a=testonly

Automatic update from web-platform-tests
Add "popup" invoking attribute, and clean up ancestor walk

With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}

--

wpt-commits: bdd414e6a1e9b4ac92cbe7f0d0dd26743ebdf674
wpt-pr: 28222
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 8, 2021
…n up ancestor walk, a=testonly

Automatic update from web-platform-tests
Add "popup" invoking attribute, and clean up ancestor walk

With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}

--

wpt-commits: bdd414e6a1e9b4ac92cbe7f0d0dd26743ebdf674
wpt-pr: 28222
@melanierichards
Copy link
Contributor

When we discussed this a couple weeks ago, we were able to simplify "c) through any of its invoking elements" in that we only care about the element that actually invoked the given popup, not "all elements whose popup attribute point to this given popup". We could keep track of this on the back edge. So I think we could open an issue in Open UI just to ensure that's documented in the spec.

IIRC the other thing we wanted to do here was to confirm in Open UI with web developers that it's actually necessary to maintain this sense of ancestry through the popup and anchor attributes. Are there cases where (even with popup providing top-layer without clipping) they would have popups that behave like descendants of the shown popup but couldn't be marked up as such?

@mfreed7 anything else I'm missing that's unresolved?

@travisleithead
Copy link
Contributor

Given the elapsed time, and lack of response on the concluding question to @mfreed7, I'm going to assume that it's been resolved or overtaken by events. If not, please re-file in the OpenUI CG's issue list: https://github.com/openui/open-ui/issues. Thanks!

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
With this CL, the "popup" attribute can be used on <button> elements
only, to invoke a popup. Additional elements (e.g. <input type=button>,
other input types, etc.) will be added in subsequent CLs. In addition,
this CL adds storage of all invoking elements on each <popup>, so that
invokers can be checked in the ancestral popup walk. No code has been
added yet to populate this vector, since it is still up in the air [1]
whether this should be part of the behavior, and it is a bit
complicated to maintain this linkage.

[1] MicrosoftEdge/MSEdgeExplainers#463

Bug: 1168738
Change-Id: I62af6e7655ad67540082b5a796f8390e4a890f72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2780636
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Ionel Popescu <iopopesc@microsoft.com>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867244}
GitOrigin-RevId: 6ac96e09f70ae2d3bf03abb1221bd8e564a5cdc0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants