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

Should popover use the flat tree #8970

Open
annevk opened this issue Mar 2, 2023 · 14 comments
Open

Should popover use the flat tree #8970

annevk opened this issue Mar 2, 2023 · 14 comments
Labels
topic: popover The popover attribute and friends topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@annevk
Copy link
Member

annevk commented Mar 2, 2023

This ends up removing slot elements for instance, is that really what we want here? Typically we use flat tree for CSS exclusively.

cc @whatwg/components @mfreed7 @josepharhar

@annevk annevk added topic: shadow Relates to shadow trees (as defined in DOM) topic: popover The popover attribute and friends labels Mar 2, 2023
@josepharhar
Copy link
Contributor

I'm missing some context. Is the popover spec using the flat tree and it shouldn't? Or vice versa? Where is the relevant spec text?

@annevk
Copy link
Member Author

annevk commented Mar 2, 2023

It's using the flat tree. However, in DOM land we typically use https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-ancestor and friends as we need to cater to elements that are not in the flat tree as well.

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 2, 2023

I went back and forth on this. The reason I went with flat tree was that "nesting" is kind of a visual concept, and the flat tree seemed slightly more natural. Take this as an example:

<div>
  <template shadowrootmode=open>
    <div popover id=p1>Popover 1
      <slot></slot>
    </div>
  </template>
  <div popover id=p2>Popover 2</div>
</div>

In this case, p2 seems fairly "nested" inside p1. E.g. it can't show until p1 shows (because p1 will otherwise be display:none). So it seemed natural that in this arrangement, the rules for nested popovers should apply.

I don't have strong opinions.

@annevk
Copy link
Member Author

annevk commented Mar 3, 2023

I think you are correct that should work that way and "shadow-including" concepts wouldn't cut it. You'd need to walk the tree in the way events do.

The problem with the "flat tree" is that certain nodes are just not in the flat tree so I think some behavior becomes undefined. That's not a problem for rendering, but can be for node tree relationships. E.g., if in your example the slot element had children and those were targeted somehow...

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 3, 2023

I think you are correct that should work that way and "shadow-including" concepts wouldn't cut it. You'd need to walk the tree in the way events do.

Ok, thanks.

The problem with the "flat tree" is that certain nodes are just not in the flat tree so I think some behavior becomes undefined. That's not a problem for rendering, but can be for node tree relationships. E.g., if in your example the slot element had children and those were targeted somehow...

That's a good point. I wonder, though, if there are real issues with nodes not in the flat tree? Again, the concepts for light dismiss and one-at-a-time behavior is all about visible (or about-to-be-visible) popovers, which are necessarily in the flat tree. Something like a popover inside unused <slot> fallback content shouldn't play a role. If the slot doesn't have assigned nodes, then the fallback content is in use, but then it's in the flat tree.

@annevk
Copy link
Member Author

annevk commented Mar 3, 2023

The problem is that if a node is not in the flat tree at all, things like

Set currentNode to currentNode's parent in the flat tree.

are undefined behavior. Maybe we could fix that by having better definitions for flat-tree walking. Hmm.

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 6, 2023

Set currentNode to currentNode's parent in the flat tree.

Ahh, yep that would need an update. Conceptually the fix is easy, but it sounds like you think the flat tree machinery isn't there? I.e. something like this would fix it:

If currentNode is in the flat tree, then set currentNode to currentNode's parent in the flat tree. Otherwise set currentNode to null.

Indeed that's how the implementation works already, for Chromium.

@annevk
Copy link
Member Author

annevk commented Mar 6, 2023

Maybe that's good enough for now. I'd prefer "parent" is actually defined and links to a definition that handles things like this.

@josepharhar
Copy link
Contributor

I also used the flat tree in these non-popover places which should be considered:

Both of these are also implemented using the flat tree in chromium. It is probably safe to change if that's what we want to do.

@mfreed7
Copy link
Contributor

mfreed7 commented Jul 13, 2023

Can this issue be closed?

@annevk
Copy link
Member Author

annevk commented Aug 28, 2023

I still think https://drafts.csswg.org/css-scoping/#flattening is the wrong construct for us to be using when we have a node tree. We should instead have operations that operate on nodes and return the relevant "flat tree" parent, child, etc.

@josepharhar
Copy link
Contributor

We should instead have operations that operate on nodes and return the relevant "flat tree" parent, child, etc.

This sounds like something that is defined or should be defined in the DOM spec...?

@tabatkins
Copy link
Contributor

Either DOM or the CSS Scoping spec, yeah. Unsure which would be more effective; probably DOM, since the level of detail here is necessary for DOM operations but not necessarily for CSS stuff.

josepharhar added a commit to josepharhar/dom that referenced this issue Sep 4, 2023
This can be used in the HTML spec as suggested in this issue:
whatwg/html#8970
@josepharhar
Copy link
Contributor

Ok, I'm going to try adding a flat tree parent definition here: whatwg/dom#1223
This could be used to replace all of the CSS flat tree references in the popover section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: popover The popover attribute and friends topic: shadow Relates to shadow trees (as defined in DOM)
Development

No branches or pull requests

4 participants