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

Enable creating widgets in shadow DOM #517

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krassowski
Copy link
Member

Fixes #434.

Introduces a distinction between attachmentNode and a content node.

Adds new methods:

  • adoptStyleSheet()
  • removeAdoptedStyleSheet()

@krassowski krassowski added the enhancement New feature or request label Jan 14, 2023
@krassowski krassowski added this to the Lumino 2 milestone Jan 14, 2023
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general feeling is that it makes Widget even more complicated. And it will create confusion as the style sheet methods are not always available.

As nothing prevent a developer to set the widget node as a shadowRoot. I would prefer developer to go that path and knows explicitly that a particular Widget contains a shadowRoot.
To clarify, I would prefer developer to do:

const node = document.createElement('div');
const root = node.attachmentNode.attachShadow({mode: 'open'});
// populate `root` with the widget view
// ...
const w = new Widget({node});

// deal with local style sheet if needed
// ...

This is similar to wrapping a custom elements (= webcomponents).

const node = document.createElement('my-element');
const w = new Widget({node});

my-element will contain a shadow DOM by definition.

@krassowski
Copy link
Member Author

There is an appeal of simplicity in what you propose, but there would be inconsistencies too, for example, Widget.addClass() and friends would effectively be a no-op because of the shadow DOM boundary, whereas the proposed code allows to inject the shadow root without changing the contract.

Also, we would end up duplicating the code for managing adopted styles. The alternative could be to use the composition approach that you suggest in a new subclass say ShadowDOMWidget:

class ShadowDOMWidget extends Widget {
   constructor(options: {node: Node}) {
      const wrapper = document.createElement('div');
      const root = wrapper.attachShadow({mode: 'open'});
      super({...options, node: wrapper});
      this._root = root;
   }
  adoptStyleSheet(sheet) {}
  removeAdoptedStyleSheet(sheet) {}
}

but then we cannot just make shadow DOM optional the same way as we do with hiddenMode (unless in the very leaf elements).

@fcollonval
Copy link
Member

Playing around with that example: https://mdn.github.io/web-components-examples/edit-word/

I added a class to the person-details element that has a shadow dom. Adding classes is valid and you can then use it to style the children of the element as I did here by adding a text underline.

image

The all point of the shadow dom regarding styling is to add visibility of what can or cannot be styled. So I don't understand why breaking this by hooking style sheets.

Note: it is possible to make element part of a shadow tree stylable from outside stylesheet using ::part pseudo class. See https://developer.mozilla.org/en-US/docs/Web/CSS/::part#examples

@krassowski
Copy link
Member Author

I added a class to the person-details element that has a shadow dom. Adding classes is valid and you can then use it to style the children of the element as I did here by adding a text underline.

I don't believe this is the case. In your example .person-age is not in shadow DOM, it is in the main DOM. It is only working because it gets inserted into a slot. Using slots like that defies the point of introducing the shadow DOM for the benefit of reducing stylesheet calculation cost.

The all point of the shadow dom regarding styling is to add visibility of what can or cannot be styled.

I don't believe this to be the case. There is also a reason for why adoptedStyleSheets was adeded. What you say is true about web components in general, but this PR is not meant to bring all three technologies of web components, but only to bring the stylesheet isolation. I should have probably made this more clear in the PR description (though it was described in #434).

So I don't understand why breaking this by hooking style sheets.

What I set out to achieve is making stylesheets for specific components independent. For example, re-rendering menu styles should not need to match against 1000s of notebook-related styles rules.

@krassowski
Copy link
Member Author

but this PR is not meant to bring all three technologies of web components

but I am happy to consider whether we should go all in (if it simplifies things and enables other things thanks to slots, etc), but that would likely be Lumino 3 given the timelines.

@fcollonval
Copy link
Member

Reporting it some notes of the performance meeting:

Discussion around web components adoption in the future in lumino/lab ecosystem:

  • in future maybe a new alternative implementation of Widget which would be web-component based
  • places where react is used poorly (too many roots), like toolbar buttons
  • icons may be a good target for gradual adoption, may give a boost
    • svg <use>

Mike also reported that the measured gains so far were not as impressive as with other performance improvements he did previously.

So I would prefer adding an new ShadowDOMWidget - maybe tagging it as experimental. And if a test case shows a important performance boost, we could use it at very specific places.

@krassowski
Copy link
Member Author

In that case I am removing this PR from Lumino 2.0 milestone as I believe we can add ShadowDOMWidget in 2.1. I will switch this one to draft for time being.

@krassowski krassowski marked this pull request as draft January 22, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shadow DOM support for widgets
2 participants