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

Slots in a flat tree #308

Closed
rniwa opened this issue Sep 1, 2015 · 57 comments
Closed

Slots in a flat tree #308

rniwa opened this issue Sep 1, 2015 · 57 comments

Comments

@rniwa
Copy link
Collaborator

rniwa commented Sep 1, 2015

Composition Algorithm defined in section 4.1 "unwraps" slots by re-distributing distributed nodes of a distributed slot by invoking Distributed Nodes Algorithm in section 3.3.

However, I don't think that should happen. In the world where re-distribution is explicit, there is no point in stripping out slot elements. It's better to treat a slot element like any other element and re-distribute it on its entirely. This will make the distribution/composition algorithm much simpler and allows slot element to be styled properly.

@hober @othermaciej @annevk @travisleithead

@sorvell
Copy link

sorvell commented Sep 2, 2015

Where would the distributed nodes be projected?

It would be problematic if they are projected inside the <slot>. In that case one couldn't, for example, project elements into an element that has display: flex.

<x-b>
  SR
    <div style="display: flex;"><slot></slot></div>
  <div>flex item?</div>

@hayatoito
Copy link
Contributor

Thanks for the feedback.

I'd like to treat a slot, as well as an insertion point, as DocumentFragment, as ShadowRoot actually is. That has been in my mental model for a long time. By considering both as DocumentFragment, everything became clear to me.

I guess someone doesn't like a slot element being left in the document composed tree, given a shadow root is not left in the document composed tree.
IMO, a slot, and an old insertion point as well as a shadow root, should not be on any more than needed to support the distribution and the composition mechanism.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 2, 2015

It would be problematic if they are projected inside the <slot>. In that case one couldn't, for example, project elements into an element that has display: flex.

 <x-b>
  SR
    <div style="display: flex;"><slot></slot></div>
  <div>flex item?</div>

I see. That's a pretty good argument for keeping the currently spec'ed behavior although it feels like we're working around the limitation of CSS.

If we had a new display type like display: ignore which replaces a node with its children for the purpose of box generations, we wouldn't need to do this, right?

One thing I don't like about the current model is the discrepancy between the composed tree as exposed to CSS and the tree event dispatching takes place. Namely, the event path will include the slot element that is a deep ancestor of the target.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 2, 2015

I'd like to treat a slot, as well as an insertion point, as DocumentFragment, as ShadowRoot actually is. That has been in my mental model for a long time. By considering both as DocumentFragment, everything became clear to me.

I don't understand what you mean by this. A slot element is an element so it certainly can't be a document fragment.

Moreover, as things stand, the concept of "slot" isn't well defined in the spec. It's a "defined location" according to the spec but there are no definitions of "defined"-ness or what a "location" is. It could be a (node, offset) pair like a boundary point or it could be a node. Whatever it is, it needs to be precisely defined. See issues #306 and #307.

@hayatoito
Copy link
Contributor

Thanks. I think we can close this issue. Please feel free to re-open this if required.
Let me address some comments as separate issues, like #306.

One more note I can add here is that we might want to support the following use case in the future:

 <table>
  SR
    <slot name="thead"></slot>
    <slot name="tr"></slot>
    ...

  <thead slot="thead">...</thead>
  <tr slot="tr">...</tr>
  <tr slot="tr">...</tr>
  ...

In this case, I guess no one is pleasant that <slot> would appear between <table> and <thead> in the composed tree.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 2, 2015

I don't think we want to do this just for the styling use cases mentioned here. I think we just need to set display: contents as the default sheet sheet on slot elements as follows and that should address both use cases mentioned here.

slot { display: contents; }

To put it another way, the composed tree is only useful to CSS box generation and the current distribution algorithm is trying to avoid box generation for slot element manually. And I'm saying that it's not necessary or desirable since box generation is inherently a CSS concern and should be dealt as such.

We can preserve the behavior of not generating boxes (so that tables, flex boxes, ec... work) without having to do it manually in the algorithm by simply relying on a CSS feature such as display: contents or a new concept in CSS if one is needed.

One huge benefit of this approach is that the author can then override display property, e.g. to show borders around slot elements, if they wanted to, and removes one more magic from shadow DOM.

@hayatoito
Copy link
Contributor

One huge benefit of this approach is that the author can then override display property, e.g. to show borders around slot elements, if they wanted to, and removes one more magic from shadow DOM.

FYI, we can get this benefit by wrapping a slot with an arbitrary element, like:

Before:

<shadow-host>
  <shadow-root>
    <slot name="xxx">
  <div slot="xxx">...</div>
  <div slot="xxx">...</div>

After:

<shadow-host>
  <shadow-root>
    <div class="wrapper">
      <slot name="xxx">
  <div slot="xxx">...</div>
  <div slot="xxx">...</div>

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 3, 2015

Right, but that's not really my point. The benefit of making this behavior reliant on CSS is that we'll decouple shadow DOM's distribution algorithm from the matter of box generation so that authors can figure it as they wish.

@dglazkov
Copy link
Contributor

dglazkov commented Sep 3, 2015

I think we should try this. We've attempted this before, and failed for a reason I can't remember. I would love to explain <slot> as just a UA style that's slot { display:contents; }.

@hayatoito
Copy link
Contributor

I see. I think one good news is this is an implementation detail because users can't see the difference unless users try to style <slot> elements, right? Both approaches should have the same result, I think, because we won't expose an API for composed tree traversal.

@annevk
Copy link
Collaborator

annevk commented Sep 3, 2015

Isn't deepPath using the composed tree?

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 3, 2015

@annevk : I think deepPath would contain the slot elements we distributed nodes through.

@hayatoito : Mostly, yes. One big difference is that using UA stylesheet would mean that the author can override it with some other display type generate boxes on slot elements so it is observable to an extent.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 17, 2015

Now there are three benefits to take this approach:

  1. It reduces the platform magic involved in shadow DOM into smaller primitives that are more familiar to Web developers.
  2. The concept of composed tree can be merged with the event path since they would become identical.
  3. The semantics of assignedSlot and getDistributedNodes() become same. That is, node.assignedNode === slot if and only if slot.getDistributedNodes().includes(node).

@dglazkov
Copy link
Contributor

I just remembered one other thing about unwrapping/flattening nodes. In Shadow DOM v0, we had this nice property that the tree-structural pseudo-classes operated on the composed tree (example here:https://gist.github.com/dglazkov/e8402f9ab18ad7ef2e7d). I guess the real question is how display:contents interacts with tree-structural pseudo-classes?

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 21, 2015

Since ::slotted can unwrap slots, I don't think this would be an issue.

@hayatoito
Copy link
Contributor

I'm thinking of adding the following changes to the spec:

A blocking issue might be how ::contents should behave in this new world. I'll file an issue for that.
We'll use a new name, like ::slotted, for ::contents.

@hayatoito
Copy link
Contributor

I forgot to mention that UA should have a UA style, slot { display: contents} in the previous comment.
I'm assuming that slots don't generate any boxes by default with this UA style.

I expect that Layout Tree doesn't change at all by default even if we add slots to the document composed tree.

Can I assume that {display: contents} is well supported in all UAs?

@dglazkov
Copy link
Contributor

@esprehn pointed out to me that I may know understand all of the implications of using display: contents fully. For example, do elements that have that applied still match styles? Should slot { color: red } result in assigned elements inheriting red? Will <slot contenteditable> do anything? Looking at the spec, I am not sure. Adding @tabatkins, because he probably does.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 29, 2015

For example, do elements that have that applied still match styles? Should slot { color: red } result in assigned elements inheriting red?

It should because box generation is separate from style inheritance

Will <slot contenteditable> do anything?

Contenteditable is a DOM concept. It won't be affected by display types.

@hayatoito
Copy link
Contributor

It should because box generation is separate from style inheritance

Hmm. That's not what I expected for display: contents. I assumed that slot {color: red} does nothing unless users overwrites UA's slot {display: contents} with something like slot {display: block}.

Let me take another look later.

@tabatkins
Copy link

Elements with display:contents are still elements; they still receive styles, participate in inheritance, etc.

All that display:contents does is prevent them from generating a box, and "lift" the boxes of their children into their own context (so they can participate in the grandparent's layout, etc.)

I assumed that slot {color: red} does nothing unless users overwrites UA's slot {display: contents} with something like slot {display: block}.

It "does nothing" visible, to the slot element itself, but the slot element still gets its color property set, and that will inherit as normal. (Tho, unless slots have changed significantly from how <content> worked, there's no inheritance from slot->slotted content.)

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 30, 2015

The section 2.3 in the current shadow DOM says:

In resolving CSS inheritance, an element must inherit from the parent node in the composed tree, if applicable.

I think this statement needs to be removed/revised since a slot element is the parent node of its assigned nodes in the composed tree.

@hober
Copy link

hober commented Feb 12, 2016

Ideally slot elements should only be as magical as they need to be, and no more magical. I much prefer a world in which they all show up in the composed tree & the UA stylesheet sets display:contents on them to a world in which the engine magically hides them from style resolution.

@annevk
Copy link
Collaborator

annevk commented Mar 11, 2016

Marking this v1 since it seems important to agree on this.

@hayatoito
Copy link
Contributor

How can we agree on this issue? I would like to move this issue forward.

There are still technical constraints to accept this proposal:

Given that, I would like to move this issue to v2.
As I explained in #331 (comment), we can support the use case to style distributed nodes in more expressive way than using slots as a hook.

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 24, 2016

No solution for http://github.com/w3c/webcomponents/issues/308#issuecomment-136903096. We do not have any concrete proposal to address this issue.

The concrete proposal is to use the default style of display: contents on every slot element. This solves the problem of slotted elements being treated is they're direct child nodes of its parent.

@hayatoito
Copy link
Contributor

Yeah, I know {display: contents} is the hope, however, a lot of HTML elements's definition are not ready for {display: content}, I think.

From https://drafts.csswg.org/css-display-3/#box-generation

contents currently only has an effect on box generation and layout. Other things that care about the document tree are unaffected, like counter scopes. Is this what we want?

e.g.

<table>
  <div style="display: contents">
      <tr>...</tr>
  </div>
</table>

https://html.spec.whatwg.org/#the-tr-element

Most of the definition of HTML elements use the parent directly in their definition and do not take care of the parent element's style. We have to update HTML Standard so that it will find the effective parent, skipping elements with {display: contents}.

And we might have to update the parser too?

Is WebKit ready for this? Unfortunately, Blink is not ready at all for {display: contents}.

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 24, 2016

How else are we going to implement slot elements? One way or another, browser engines need to support semantics similar to display: contents. We don't need to consider what happens with other elements with display: contents since we're talking about slot elements here.

In fact, it's fine for initial implementations of slot elements to not fully respect display: contents-ness. For the purpose of writing specifications, however, we should assume display: contents exist. If Blink has issues with display: contents, then you should bring that up on www-style instead.

@hayatoito
Copy link
Contributor

Thanks. I tend to agree, hoping web developers would not complain about Blink about this.
However, before making the final decision, let me consult with other guys.

@hayatoito hayatoito changed the title Composition Algorithm shouldn't unwrap slots Slots in a flat tree Mar 24, 2016
@hayatoito
Copy link
Contributor

My biggest concerns are:

  1. How can we trust the feature of {display: contents}? Can we be 100% sure this will be successful? It looks there is no other spec which bets on its success, AFAIK. Only Shadow DOM looks the early adapter.
  2. Suppose that we support "a slot in a flat tree" in the spec, and developers start to style a slot element, that's un-reversible change.
    Later, when we find "{display: contents}" is not a thing, it's too late. We can not change the behavior without breaking the Web site.

Thus, I have to be careful to add a slot in a flat tree. If we support it, that means that Shadow DOM will fail if {display: contents} fails.

Is there any significant benefit of adding a slot in a flat tree, which is worth having such a great risk?
Note that we can support "slots in a flat tree" anytime later when we can feel "{display: contents}" is widely accepted.

I am not opposing to "slots in a flat tree", but I am afraid that it's too risky today, and today is not the day.

@tabatkins, WDTY? Do you have a good feeling that "{display: contents}" will become successful??

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 28, 2016

In general, it's a good idea to minimize the number of new "magics" we introduce to the Web. Being able to describe/de-sugar slot element's behavior in terms of display: contents would be "explaining" the platform and "building on top of primitives". Introducing a shadow-DOM specific CSS box generation mechanism is anti-pattern in this regard (against the Extensible Web Manifest).

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 28, 2016

On more concrete examples, I already hit an interoperability problem with Chrome whereby which it doesn't respect display: none on slot elements whereas WebKit did. Chrome's behavior of ignoring display:none was rather surprising for me as an author. In addition, having to use :slotted(*) because styles on slot elements would be ignored has been really annoying.

@tabatkins
Copy link

display: contents is useful on the CSS side for a number of things; Shadow DOM is only one of them.

I can't say whether it will succeed or not, as that depends on browser vendors implementing it. That said, I think Firefox has done so.

@hayatoito
Copy link
Contributor

@tabatkins, thanks.

Can I confirm one more thing? Does the following HTML work as expected (from the view of the spec)?

<div style="display: flex;">
  <div style="display: contents;">
    <div>flex item 1</div>
    <div>flex item 2</div>   
    ....
  </div>
</div>

This is the raised concern from @sorvell in #308 (comment).

@tabatkins
Copy link

Yes, it works as expected, per spec. I think this works in Firefox right now if you want to test it.

@hayatoito
Copy link
Contributor

@tabatkins , thanks. I appreciate that.

Then, it looks reasonable to support "add slots in a flat tree" with UA-style "slot { display:contents; }" in the spec. I'll update the spec.

Since Blink has not supported "display: contents" yet and its engine requires a huge rewrite to support "a slots in a flat tree", Blink's initial implementation of v1 will not fully respect it.

If someone has a concern about this decision, please let us know that.

@esprehn
Copy link

esprehn commented Apr 1, 2016

Looking at WebKit I think right now slotted elements inherit from the parent of the <slot>, and webkit only respects display: none. That is doing slot { display: block; } has no effect.

I'm very hesitant to ship with matching behavior, it doesn't seem like that's how display: contents was supposed to work. display: contents also has implications, for example are counter increments considered when declared on the slot? What about quoting level? Does slot::before and slot::after work? does display: contents disable ::before? (I hope so). What about slot::first-line ?

I'd like to work out the details before we commit to making display: contents be a thing, and slot having a dependency on that technology. The current system is easy to explain, is just gone by the time styles are computed. There's no subtle rules for what does and does not apply.

@tabatkins
Copy link

Looking at WebKit I think right now slotted elements inherit from the parent of the , and webkit only respects display: none. That is doing slot { display: block; } has no effect.

I'm very hesitant to ship with matching behavior, it doesn't seem like that's how display: contents was supposed to work.

Correct, that is a broken implementation. Firefox has, afaik, a real implementation of display:contents.

display: contents also has implications, for example are counter increments considered when declared on the slot? What about quoting level? Does slot::before and slot::after work? does display: contents disable ::before? (I hope so). What about slot::first-line ?

All defined by https://drafts.csswg.org/css-display/#valdef-display-contents.

Counters work on the element tree; they're not disturbed by box-tree manipulation. (If they were, then the 'order' property would mess with them too.) Quotes aren't very specific (they're defined by language lingering from CSS2, when we were much less explicit about the element/box/fragment divisions), but they should work the same. Pseudo-elements still generate as normal.

@rniwa
Copy link
Collaborator Author

rniwa commented Apr 5, 2016

Telecom agreement: We'd spec this as display: contents. While there are unknowns with respect to its behavior, and there is disagreements within Google regarding this feature, it seems okay to use this given WebKit and Gecko are implementing this feature in near future.

ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=149701

Reviewed by Antti Koivisto.

Source/WebCore:

Following discussions on Github discussions [1] [2], we're adding the default rule of `display: contents`
on slot elements and making slot elements render its children when there are no assigned nodes [3].

Make these changes by attaching renderers on direct-children of slot elements when there are no assigned
nodes during render tree construction. Note `display: contents` will be aded in webkit.org/b/149439.

[1] WICG/webcomponents#317
[2] WICG/webcomponents#308
[3] WICG/webcomponents#308 (comment)

Tests: fast/shadow-dom/css-scoping-shadow-slot-fallback.html
       fast/shadow-dom/shadow-layout-after-slot-fallback-changes.html

* style/StyleResolveTree.cpp:
(WebCore::Style::attachSlotAssignees):
(WebCore::Style::detachSlotAssignees):
(WebCore::Style::resolveSlotAssignees):

LayoutTests:

Added tests for fallback contents in slot elements. One of them could be safely submitted to CSS WG,
and the other one is a style recalc test.

* fast/shadow-dom/css-scoping-shadow-slot-fallback-expected.html: Added.
* fast/shadow-dom/css-scoping-shadow-slot-fallback.html: Added.
* fast/shadow-dom/shadow-layout-after-slot-fallback-changes-expected.html: Added.
* fast/shadow-dom/shadow-layout-after-slot-fallback-changes.html: Added.


Canonical link: https://commits.webkit.org/167842@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@190430 268f45cc-cd09-0410-ab3c-d52691b4dbfc
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

8 participants