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

flatten: true in assignedNodes doesn't make much sense when a slot doesn't have display: contents #493

Closed
rniwa opened this issue May 2, 2016 · 25 comments

Comments

@rniwa
Copy link
Collaborator

rniwa commented May 2, 2016

When a slot element assigned to another slot has display value other than contents, it doesn't make much sense to unwrap it in assignedNodes since those elements would definitely show up in the rendering.

@rniwa
Copy link
Collaborator Author

rniwa commented May 2, 2016

In addition, when a slot is wrapped another element, e.g. span, it continues to function as a slot. As an example, consider the following tree:

div --------- shadow-root
 + slot A      + slot C
 + span
    + slot B

slot C has two assigned nodes: slot A and span. But span also contains slot B. It's not clear as to why slot A needs a special treatment of being unwrapped just because it happens to be a direct child of the shadow host.

Or perhaps the importance is that it's a direct child of a slot?

@rniwa
Copy link
Collaborator Author

rniwa commented May 2, 2016

@annevk
Copy link
Collaborator

annevk commented May 2, 2016

Yeah, a child of a slot becomes semi-assigned and therefore unwrapped if it's a slot. I'm also not sure we should make this API dependent on layout. There's a difference between what's rendered and what is semantically assigned.

@hayatoito
Copy link
Contributor

hayatoito commented May 9, 2016

When a slot element assigned to another slot has display value other than contents, it doesn't make much sense to unwrap it in assignedNodes since those elements would definitely show up in the rendering.

The API was designed in the era: "slot is not included in the flat tree", to meet the following demand:

  • Developers wanted to know the effective children ("child nodes in the flat tree"), because their interest is effective child nodes, in practice. That's the most important point.

In v0, we had InsertionPoint.getDistributedNodes(), which is basically equivalent to the current assignedNodes({flatten: true} of v1, which tell us the effective child nodes.
The reason why slotchange events is for distributed nodes, rather than for assigned nodes, is the same. They wanted to know the change of effective children.

@hayatoito
Copy link
Contributor

hayatoito commented May 9, 2016

Or perhaps the importance is that it's a direct child of a slot?

A slot B does not appear in assignedNode of slot C. Thus, it can be a descendant, but it never be an effective direct child.

We do not have to detect any change in any node in the subtree. That's too burden for UAs. What we can do at most for developers here is to tell them the (change of) direct children (in the flat tree).

@rniwa
Copy link
Collaborator Author

rniwa commented May 9, 2016

Here's a problem. If the span had display: contents, then slot B would be an effective child of slot C.

@annevk
Copy link
Collaborator

annevk commented May 9, 2016

Just layout-wise. We don't let that effect what the actual children are of an element. Whenever we have operations on some element's children, it's always about the node tree, not node tree + some CSS.

@rniwa
Copy link
Collaborator Author

rniwa commented May 9, 2016

The thing is that the idea of caring about assigned nodes of an assigned slot is about caring about display: contentsness of the slot so we're already caring about the CSS boxes here.

@annevk
Copy link
Collaborator

annevk commented May 9, 2016

No, you only care about display:contents when you generate the flattened tree. For the DOM API that should not matter one bit. We don't let firstChild depend on whether any child has display:none.

@rniwa
Copy link
Collaborator Author

rniwa commented May 9, 2016

No, you only care about display:contents when you generate the flattened tree. For the DOM API that should not matter one bit. We don't let firstChild depend on whether any child has display:none.

In that case, we shouldn't have flatten: true option at all.

Think about why we need flatten: true in the first place. That's because nodes assigned to slot A, which is in turn assigned to slot C appear as though they're direct children of slot C. However, this is a consequence of the fact slot A has display: contents. If it didn't, then there is nothing special about slot A. All assigned nodes of slot A would generate CSS boxes under slot A's CSS box, and they don't appear as direct children of slot C at all.

@annevk
Copy link
Collaborator

annevk commented May 10, 2016

@rniwa and I had some discussion on IRC (#whatwg) and concluded that maybe composed:true would be better here too. The reasoning is that flattening would be the layout tree, which might also disregard display:none boxes and such. That way "composed" is for any node tree operations.

@hayatoito
Copy link
Contributor

hayatoito commented May 10, 2016

I'm fine with the naming of composed: true.

My major concern here is how we should treat display: contents. The existence of display: contents made the situation complex. That violates our original assumption. However, it's not good idea to let DOM APIs depend on the value of "display: xxx".

Given that, the following might be acceptable:

  • We unwrap only slots. A slot is already so special in a shadow dom world. That covers most use cases.
  • We do not unwrap other elements even if they are "display: contents".

Does this make sense?

@annevk
Copy link
Collaborator

annevk commented May 10, 2016

Indeed, for the DOM only non-layout aspects such as slots matter. For flattened trees display matters too.

@hayatoito
Copy link
Contributor

hayatoito commented May 10, 2016

Okay. Then, the conclusion here is:

  • We do not change any semantics.
  • We rename flatten: xxx -> composed: xxx.

However, I am feeling that composed is getting used in a lot of meanings. Maybe we should use more explicit name here. How about using unwrap? Does it sound bad?

@annevk
Copy link
Collaborator

annevk commented May 11, 2016

@rniwa @smaug----, thoughts? Not sure I care strongly either way.

@rniwa
Copy link
Collaborator Author

rniwa commented May 12, 2016

The issue wasn't that the variable name is bad. It was that the semantics doesn't make much sense in the world where a slot doesn't get "unwrapped" when its used style isn't display: contents.

@annevk
Copy link
Collaborator

annevk commented May 13, 2016

Again, you are conflating layout and the DOM I think. The first child of an html element is not the body element just because the head element has display:none. Same kind of logic should apply here.

@rniwa
Copy link
Collaborator Author

rniwa commented May 13, 2016

Again, you are conflating layout and the DOM I think. The first child of an html element is not the body element just because the head element has display:none. Same kind of logic should apply here.

Well, my point is that the very concept of having flatten: true or composed: true is conflating layout and DOM. That is, the idea of having to unwrap slots is conflating layout and DOM, not the other way around.

Why else would one want to have flatten: true or composed: true?

@annevk
Copy link
Collaborator

annevk commented May 13, 2016

It seems to me a way to get all elements that are logically assigned to a slot, without having to do recursion and fallback yourself for any assigned slots. There's no guarantees as to whether anything that is assigned will also be displayed. Some shadow-including ancestor might very well have display:none.

@annevk
Copy link
Collaborator

annevk commented May 13, 2016

Furthermore, this API might be used in a tree that's not even connected with a browsing context or viewport.

@rniwa
Copy link
Collaborator Author

rniwa commented May 13, 2016

It seems to me a way to get all elements that are logically assigned to a slot, without having to do recursion and fallback yourself for any assigned slots

A slot assigned to another slot is a logically assigned element. Where did you get this whole idea of slots not being inside another slot?

If a slot is assigned to another slot, it is IN the slot. I don't understand the basis for unwrapping/flattening any slots. Why on the earth do you want to do such a thing in pure DOM API?

@annevk
Copy link
Collaborator

annevk commented May 13, 2016

Where did you get this whole idea of slots not being inside another slot?

I didn't say that.

I don't understand the basis for unwrapping/flattening any slots. Why on the earth do you want to do such a thing in pure DOM API?

E.g., when you write your own layout engine.

@rniwa
Copy link
Collaborator Author

rniwa commented May 13, 2016

I don't understand the basis for unwrapping/flattening any slots. Why on the earth do you want to do such a thing in pure DOM API?

E.g., when you write your own layout engine.

If you're writing your own layout engine in JS, you might as well as traverse through nodes. Avoiding the work of having to traverse through slots' assigned nodes should be of the least concern.

@hayatoito
Copy link
Contributor

Can we make a decision here?

My opinion has not changed since #493 (comment). I prefer unwrap though.

@hayatoito
Copy link
Contributor

Let's close this issue in status quo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants