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

Make custom elements w/ shadowRoot { display: contents } by default #723

Closed
ndugger opened this issue Jan 6, 2018 · 18 comments
Closed

Make custom elements w/ shadowRoot { display: contents } by default #723

ndugger opened this issue Jan 6, 2018 · 18 comments

Comments

@ndugger
Copy link

ndugger commented Jan 6, 2018

This idea is definitely jumping the gun a little bit, but...

As I've been working on a project that uses a react-like library that uses native web components, I've found that when I'm using them with flexbox, often times I just want the host custom element to simply act as a shell for the contents without having its own box model so that it doesn't hijack the flexbox context.

I did some searching today, and apparently display: contents does exactly this; it makes sense for this to be default behaviour, because if a custom element has a shadowRoot, you generally expect the contents to define the makeup of the element.

However, at the moment, in both Chrome and Firefox, display: contents is usable only behind a flag, and Edge probably doesn't even have it on their radar.

Example:

<flex-container>
    <custom-element>
        <flex-item/>
        <flex-item/>
    </custom-element>
</flex-container>

In this example, you would have to reapply display: flex to the custom-element in order for the flex-items to actually be flex-items. However, if custom-element's default was display: contents, the flexbox context would pass through it, allowing the flex-items to actually be flex-items automatically.

Make sense? Thoughts? I saw one closed issue that suggested them to be display: block by default, but this makes more sense, in my opinion.

@domenic
Copy link
Collaborator

domenic commented Jan 6, 2018

What would worry me about this is that this isn't how any built-in element behaves. I don't think custom elements should be gratuitously different from built-ins.

display: contents is fairly surprising behavior; causing flex items to no longer be collected via direct children, but indirectly as you've shown, is not at all something that should be default, I think.

@ndugger
Copy link
Author

ndugger commented Jan 6, 2018

Upon further thinking, there's 2 things that might make or break this idea.

1. It does not make sense if you're writing your custom elements in HTML

I can see what you're saying about how custom elements should not be gratuitously different from built-in elements. If you saw that HTML, you would expect it to behave the way that it currently does.

2. It really makes sense if you're creating components and their children all in JS

From a react-like point of view, a component is just a set of predefined behaviour for a set of elements. The actual component has no affect on the box-model--only its contents do.

While I agree that custom elements should behave like any other elements, my proposal is to affect custom elements that have a shadowRoot. Shadow DOM already behaves differently enough from the regular DOM, with encapsulated styles, among other things, and I think that the expectation that the contents of the shadowRoot define the makeup of the element is enough to justify this change.


It's a double edged sword, in my opinion. Do you still manually write your HTML, or are you using a JS library that uses some sort of component architecture to define the makeup of your DOM?

Maybe you don't see a huge difference, but I do.

@domenic
Copy link
Collaborator

domenic commented Jan 6, 2018

I don't think we should create asymmetries between those situations, even if other frameworks also use the word "component$ in such an asymmetrical way.

ndugger pushed a commit to ndugger/cortex that referenced this issue Jan 6, 2018
@ndugger
Copy link
Author

ndugger commented Jan 8, 2018

I agree with all of your points, and this could be accomplished on the library level, but I still hold that it makes sense in the context of defining your components in JS. I'd love to hear what anyone else has to say on the matter as well before closing.

@rniwa
Copy link
Collaborator

rniwa commented Jan 8, 2018

Right now, there is no way to create a shadow root automatically via customElements.define.

We certainly can't change the behavior of attachShadow's behavior on custom elements at this point since we've been shipping shadow DOM for two years and custom elements for one year.

So I think this is a consideration for the future when we add a mechanism to automatically attach a shadow root when upgrading/instantiating a custom element.

@js-choi
Copy link

js-choi commented Jan 9, 2018

Have there been prior discussions about standardizing a way to automatically attach shadow roots to custom elements during their upgrade/instantiation?

@rniwa
Copy link
Collaborator

rniwa commented Jan 9, 2018

See #135 and related issues.

@trusktr
Copy link
Contributor

trusktr commented Jan 16, 2018

It's plenty of easy to just write a class that automatically creates a shadow root for your instances. Making it default is not necessary; I've custom elements that don't need a shadow root, they only render themselves differently and distributing outer children to inner roots is not part of that custom rendering.

A good example in the wild is A-Frame. Those elements don't use Shadow DOM at all, they are only a means to specify WebGL scenes (and so are my elements).

But anyhow, display: contents will help solve issues like for example components that want to render two <tr> elements and that otherwise interfere with the browser's legacy table rendering without display: contents (or, at least, I hope this new style lets us fix this problem).

React added "Fragments" to solve this problem, while Vue removed multi-root components which unfortunately introduces the problem into Vue.

@effulgentsia
Copy link

effulgentsia commented Jan 17, 2018

I just want the host custom element to simply act as a shell for the contents without having its own box model

@ndugger: Is the "I" in that sentence the custom element author, or the custom element consumer, who should decide whether the custom element should simply act as a shell without its own box model? My first hunch is that it should be a characteristic of the custom element itself, meaning decided by the element author, not the element consumer. In which case, can't that be done via the :host CSS selector (e.g., :host {display: contents})? What would you see as the reason to making it the default behavior rather than that single-line opt-in?

@ndugger
Copy link
Author

ndugger commented Jan 18, 2018

@effulgentsia I agree with you. I was just finding it a little bit annoying to apply that to a lot of my components and saw this as an opportunity to improve upon that, but I can solve for it on the library level as well, so this is mostly moot.

However, I think an alternative, like React's Fragments (see trusktr's comment), is worth exploring in the future.

@trusktr
Copy link
Contributor

trusktr commented Jan 18, 2018

finding it a little bit annoying to apply that to a lot of my components... I can solve for it on the library level as well

Yep! F.e. we can make it re-usable as a class factory mixin:

function DisplayContents(BaseClass) {
  BaseClass = BaseClass || HTMLElement
  
  return class DisplayContents extends BaseClass {
    constructor () {
      super()
      // apply the style somehow
      this.style.display = 'contents'
    }
  }
}

Then, if there's an existing custom element class,

class CubeLayout extends HTMLElement { ... }

it can be applied to the class:

class CubeLayout extends DisplayContents(HTMLElement) { ... }

The styling might be applied in a different way, but at least we can abstract it into a single place.

an alternative, like React's Fragments (see trusktr's comment), is worth exploring in the future.

Hmmm. The thing that makes Fragments possible in React is that in React there is a whole separate tree from the DOM, and React's internal tree holds a reference to an actual fragment in this internal render tree. Then based on this internal tree it knows how to create a new tree (the DOM) where the new tree does not contain the Fragment in the structure.

But Custom Elements, on the other hand, operate on a single tree (the DOM), so there isn't this opportunity to map one tree structure to another tree structure (that we've thought of yet at least).

So far at least, display:contents with the current single-tree mechanic of the DOM seems like the best way to get elements out of the way in terms of how the DOM renders and functions. Still though, the container elements will still be in the DOM as actual references in the tree.

There's the "composed tree" concept that the HTML engine maps to from the regular DOM, based on the composition of shadow roots, but outside of the HTML engine we don't really have access to that tree, we can only infer it from looking at open shadow roots (or if they are closed, monkey patching attachShadow to keep track of all shadow roots created in the application).

I'm sort of going off on a tangent now, but elements that are display:contents are essentially gone from the composed tree, if we factor this styling in. However these elements are still observed in parent and children properties of the elements that are next to the display:contents elements in the tree. We could traverse the composed tree factoring in shadow roots and elements that are contents, then come up with out own tree, and from this, we could perhaps do cool things like render to WebGL (think of something like A-Frame which renders to WebGL, but A-Frame is currently not considering the ShadowDOM composed tree, and therefore it completely breaks if you try to use ShadowDOM with A-Frame).

I've been thinking about this for a while, and have done some initial work over at infamous which can render to WebGL as well (not trying to advertise here, just like to share what I've been thinking around web components). I've put in place some features that allow it to track the composed tree, and soon this will map to the Three.js rendering so that making ShadowDOM-based Custom Elements will compose well together. It will give component authors the ability to compose and nest elements inside of each other to make these components highly-reusable. Imagine, for example, layouts designed for 3D space, that work based on distributing their content into slots. This is currently not possible with A-Frame (though you can just use React/Angular/Vue for that, but then you're sacrificing portability of you components by limiting them to a single framework).

@annevk
Copy link
Collaborator

annevk commented Feb 17, 2018

So I think this is a consideration for the future when we add a mechanism to automatically attach a shadow root when upgrading/instantiating a custom element.

I think even then we shouldn't couple this as such side effects are often unexpected and would need to be configurable. At best we could consider some kind of toggle for this, but at that point it's probably better as a library feature or on top of adding default styles (whenever we added that).

@rniwa
Copy link
Collaborator

rniwa commented Feb 20, 2018

We would be totally opposed to making this backwards incompatible change. We would not be opposed to adding an optional feature to add this property but perhaps this is a special case of the issue #468. Is there anything we need to do if we had an API to do #468?

@TakayoshiKochi
Copy link
Member

I agree this can be solved by #468 - the discussion is long to follow, basically with it you can give a stylesheet to a custom element, without attaching any shadow on it.

@annevk
Copy link
Collaborator

annevk commented Feb 20, 2018

Okay, let's close this in favor of that. If there's still desire for a specific thing around display:contents please file a new issue with a proposal that's backwards compatible with what's already shipped. Thanks all!

@Javarome
Copy link

Late to the party but I'm concerned by the fact that having display: contents by default would remove the web component contents from the accessibility tree, including slots (which are in light dom) roots on some browsers.

@justinfagnani
Copy link
Contributor

@Javarome don't worry. It's not possible to change the default. This issue is closed.

@trusktr
Copy link
Contributor

trusktr commented Jun 27, 2024

No description provided.

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

10 participants