Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Light DOM Support #44
RFC: Light DOM Support #44
Changes from 7 commits
cb55a94
72fe402
a306839
9bf37b0
bad70a8
6b6572e
f791aec
990bc6a
dea55cf
0a87dea
a9b1c55
ea27928
3a62c00
fbcab6b
b2df06f
36a77b8
8313490
340f4b9
a00f719
653b680
846f6b9
74f6da6
458cc0f
619721a
798268f
a3f6d71
c8a76f0
e735d3e
3460e3a
baa79bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Dean said in DRB today, UIP has the same (near-term future) requirements around theming and branding for LEX and LWR apps as Communities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe the LEX requirements are the same as what Communities is requesting because the number of actors in the system is very different. Let's discuss offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hsterlingsfdc Could you make sure to bring the LWC team into the loop when this will be discussed/designed? I would like to better understand the LEX and LWR requirements in this regard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Image content: Remove "Today" and "Where we want to be" label. The RFC should not present points of view but rather preset facts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list of invariants and constraints is missing. From the live discussion on this RFC today it became obvious that there are many you're working with.
There are many others that need to get captured here. This'll bring visibility to the rationale driving this design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still fuzzy about:
what I see in the PR is that the content of the slot is flatten, so the slot element itself never gets rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned about exposing light DOM via
MacroElement
. I can break my concerns into two groups: 1) deviation from other WC frameworks, and 2) potential class proliferation.Deviation from other WC frameworks
Here is how other frameworks handle light DOM:
Stencil
Fast
Lit
So we have 2 frameworks opting for decorators, and another opting for an overridable method. LWC would be the only WC library using separate classes (unless I missed one).
Potential class proliferation
In the future,
we may want to support closed shadow roots(edit: my mistake, didn't realize we already did 🤦 ). Would that be another class?There is also a proposal for "open stylable roots" (WICG/webcomponents#909). Would this be another one?
In that proposal, there is even some talk of
attachShadow()
eventually looking like this:If the options bag for
attachShadow
continues to grow like this, then we could end up with a combinatorial explosion of classes to handle every possible case.My personal preference would be for decorators or mixins, but mostly I'm just concerned about choosing classes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original reason we chose a different class was to emphasize that these are different kinds of components because the programming model is different. I'm not sure if we still feel that is the case.
I'm curious how the createElement API will change which currently takes the mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tedconn Ah my mistake, I didn't even know we supported closed shadow roots. 🙂 It seems that
createElement
would be the right place to put the options bag (if it becomes a big bag!).So it looks like my concerns about class proliferation might not apply, since we only plan on having
MacroElement
for this one case (light DOM). But then why not have light DOM as an option increateElement
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need an API on the component, so component authors can toggle, not on the engine. Something like this would be nicer though, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed in the past 2 options, using a static field or using a brand new base class. Until the decorator proposal goes back to stage 3, I don't think it is safe to introduce a brand new decorator.
The main advantage of introducing a brand new class over static fields is around component inheritance. You can shadow (by inadvertence or intentionally) a base class static field. It means that you can turn a Shadow DOM component into a Light DOM one (and vice-versa) by extending from it. Such override can't be done when inheriting from a different base class.
In the following example, let say that we introduce a new
shadowDOM
static boolean to indicate whether the component rendered in the Light DOM or in the Shadow DOM. It is something we should discourage.Now that you speak about class proliferation, I am more favorable to adding a signal via a static field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion with the team, we landed on the static field rather than
MacroElement
, correct? AFAICR, we concluded:Is this right? @pmdartus @abdulsattar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of Stencil, Fast, and Lit, only Stencil opted to make slots "work" with light DOM.
Fast says:
Lit is also against slots in light DOM:
Based on discussion in this thread, it seems like Stencil's choice may have resulted in significant performance overhead, which is understandable given that a lot of behavior you get "for free" with slots in shadow DOM now has to be reimplemented for light DOM.
Before committing to this, I'd be interested to see some more details about how we plan to handle slots in light DOM:
I'd also be interested to see some benchmarks, to understand if the issues Stencil ran into were just problems with Stencil's implementation, or if they're inherent to trying to make slots "work" in light DOM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another interesting point of comparison: Aurelia has slots, but they explicitly mark it as
<au-slot>
, not<slot>
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With synthetic shadow DOM, we implemented a system for style scoping similar to this (using attribute selectors). However, with shadow DOM, it was kind of implied that eventually we would migrate to native shadow, so the synthetic system wasn't designed to last forever. With this system, though, we are committing to support a new styling system indefinitely, including whatever implementation details we land on, which will impact compatibility moving forward. For instance:
It seems to me that the safest option is to not provide any kind of style scoping for light DOM elements. I.e. "if you want style scoping, use shadow DOM." This avoids making any commitments toward one particular styling system in light DOM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the goal is to land on the most natural ergonomics for the developer, including developers already versed in LWC.
Styles in LWC are scoped today, but for end users this is not a natural consequence of the shadow DOM encapsulation, rather it just makes sense since the authoring model is per-component. This is why Vue scopes styles, not because it aligns with the Shadow DOM model. For us that is just a bonus.
Not scoping styles would mean that without any extra code on our side, styles would leak into shadow DOM children, which would not match the native shadow DOM behavior when we turn off synthetic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tedconn
It is due to
synthetic-shadow
, sure, but the long-term goal is to migrate to native shadow DOM. So then it would be a natural consequence. 🙂I'm not saying scoped styles aren't good for developer ergonomics; I cede that point. 🙂 My point is just that "which scoped styles?" is an important question, especially since we have to indefinitely maintain whatever we choose today.
I'm not sure I follow this. Today, in a purely non-LWC, non-synthetic world, I can have a light DOM-using component with a shadow DOM-using component as its child, and the only styles that leak in are inheritable styles like
font-family
andcolor
, which is per the spec. It should work this way both in native shadow DOM and in our synthetic shadow DOM.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be wrong but my thinking was that If styles aren't scoped, in synthetic shadow DOM, those styles will leak in, because there's no other mechanism to prevent that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure, so I just ran a quick check. It looks like if you add the style
body { font-family: monospace; }
to a Salesforce page, it will indeed leak in to synthetic shadow DOM components. (This is the same as what would happen with a light DOM-using component wrapping a shadow DOM-using component.) So it appears we are following the spec correctly on that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tedconn You can prevent inherited styles to be applied by resetting all the properties on the host element. It works in both native shadow and synthetic shadow.
We discussed CSS scoping at length and we reached the conclusion that it something we would like to preserve with Light DOM for developer ergonomic reasons, as @tedconn mentioned.
We considered the following approach to style scoping:
component content scoping: Styles are applied to the content the component renders and aren't applied to children components. This approach is similar to the way styles are scoped today in synthetic shadow. It also behaves the same as Vue, Svelte, and Angular. An HTML attribute is added to all the CSS selectors, the same attribute is also added to all the elements rendered by the component.
component subtree scoping: Styles are applied to the content the component renders and also to the content rendered in the children's components. This is a similar approach to Aura styling. All the CSS selectors are prefixed with a CSS attribute selector and the same attribute is applied to the root element to scope the subtree.
We decided to go with the component subtree scoping approach for the following reasons:
querySelector
works in the light DOM. It applies to the entire subtree and not only the current element.>>>
in Vue and Angular or:global
in Svelte).The main downside for me of the component subtree scoping approach is that you can run into a style clashing issue where the parent component can unintentionally override children's component styles because the parent selector has a higher specificity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense. Two comments:
First, I'm not sure that preferring Aura's styling model to Vue/Svelte's styling model is the best choice from the perspective of developer ergonomics. Aura is well-known inside Salesforce, but not so much outside. (Also if I'm not mistaken, most React CSS-in-JS libraries follow something more like the Vue/Svelte model.) Personally I would find it a bit surprising that parent light components can affect child light components' styles.
Second, for this CSS:
I find this to be a bit scary from a performance perspective. Imagine something like:
Now you're potentially the telling the browser to check the ancestor chain for every
div
to see if its attributes match. For class selectors, there is already a Bloom filter to handle this, but I'm not sure if browsers have gotten around to optimizing attribute selectors in the same way. I may have to whip up a benchmark to test this. 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to know. We can survey developers internally to see which approach they find the most intuitive.
I don't think we can use classes because component developers can set classes via the template (from outside) or in the component via
this.classList
(from within). We would need to special-case this scoping class and ensure that it is always present even when the author manipulates the component classes. I am not sure if we can do this in an efficient way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I reference the root element in a light dom component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
LightningElement
, all the properties and methods on thethis
value interacts with the host element (this.getAttribute
,this.querySelector
,this.classList
). Andthis.template
allows to interact with the shadow root. However, LWC never exposes the host element itself to the component instance. You can get access to it, viathis.template.host
but it is more a hack than anything else.The same model is preserved with
MacroElement
. It is possible to interact with the host element using methods and properties on thethis
value. But there is currently no workaround (likethis.template.host
) to access the host element from aMacroElement
component instance.If accessing the host element is necessary, I would vote for making it available to both
LitghtningElement
andMacroElement
in a similar fashion. For example via ahost
getter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an example of how light DOM components can be disabled? Will there be a compiler flag to accomplish this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
way more detail necessary here. Does this mean that a component was authored in the LightDom but app requested it in the shadow dom for ex?