refactor: fix forwarded light DOM slots mapping

[jmsjtu]

Details

Fixes #3843 and replaces #3854.

The main issue with #3854 is that it does not handle static fragments and does not work properly with reactive slot assignment (slot={reactiveValue}).

There will also need to be a separate PR to update the ACT compiler.

This PR fixes the issue of light DOM slots not mapping correctly when they are forwarded to another component.

Essentially, when a light DOM slot is passed as a child to another element it will return the children rather than create a slot element.

This can cause the slot mapping to be incorrect when the children have a slot attribute already.

For example:

<x-light-child>
    <div slot="namedSlot">Named slot content</div>
    <div>Unnamed slot content</div>
</x-light-child>

<template lwc:render-mode="light">
    <x-leaf>
        <slot name="namedSlot"></slot>
        <slot></slot>
    </x-leaf>
</template>

// x-light-child

<template>
    <slot></slot>
</template>

// x-leaf

In the above example, when the slot element is passed x-leaf, the children are passed directly (no slot element is created).

Since the div element has a slot="namedSlot" attribute, the slot mapping will attempt to map to a slot named "namedSlot" which doesn't exist and does not render.

This is especially an issue with native shadow DOM as in order to render the slot correctly, the slot attribute must be included on the element.

Note, this is only an issue when the root of the slotted content is a slot. In the above example, if the slot was wrapped in a div the content would render correctly.

ex:

<template lwc:render-mode="light">
    <x-leaf>
        <div>
            <slot></slot>
        </div>
    </x-leaf>
</template>
// x-light-child

This PR resolves the issue by replacing the slot attribute on the children with the correct slot attribute mapping.

In above example, the div slot attribute will be removed on the div so that it can render properly.

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ⚠️ Yes, it does include an observable change.

The top-level element of any content that is slotted into a light DOM <slot> element will have the slot attribute updated to match the slot attribute of the <slot> element.

For example,

<x-light-child>
    <div slot="namedSlot">Named slot content</div>
    <div>Unnamed slot content</div>
</x-light-child>

<template lwc:render-mode="light">
    <x-leaf>
        <slot name="namedSlot"></slot>
        <slot></slot>
    </x-leaf>
</template>

// x-light-child

<template>
    <slot></slot>
</template>

// x-leaf

Will render as follows when it is slotted into <x-leaf>:

<div slot>Named slot content</div>
<div>Unnamed slot content</div>

Note that the top <div> element's slot attribute value has been removed.

This is because the light DOM named slot does not contain a slot attribute, ie:

<slot name="namedSlot">, does not contain a slot attribute.

Since light DOM <slot> elements do not get rendered into the DOM and the content is directly appended to the host element, this is needed to correctly forward the slotted content to the correct named slot.

Additionally, updating the slot attribute of the slotted content is necessary in order to slot content correctly in native shadow.

GUS work item

W-14356488

[jmsjtu]

We need to move slot assignment outside the props in ElementData because static vnodes can also be reassigned during light DOM slot forwarding.

For example:

<x-slot-cmp>
    <div>This is a static element</div>
</x-slot-cmp>

<template>
    <slot slot="foo"></slot>
</template>
// x-slot-cmp

In the above example, the div, which is a static element needs to be forwarded to thefoo slot. This can't happen through the props because static vnodes don't update attributes from props.

[jmsjtu]

We'll need to add an ACT compiler change for this

[jmsjtu]

This might be an issue if we decide to use API versioning because of the mechanics of how ACT compiler + API versioning works.

Try publishing an alpha version of LWC with these and run the jest tests on core with this change.

[jmsjtu]

These tests asserts that forwarding slots between light and shadow DOM should produce the same results.

[jmsjtu]

These tests ensure that dynamically setting the slot attribute on a forwarded slot works properly between light and shadow DOM.

[nolanlawson]

Great work on this!! A real beast of a PR. 💪

Overall it looks good to me, but I would like to review in dev sync since this is a risky change touching a lot of the framework internals.

[nolanlawson]

TIL 😂

[nolanlawson]

Is there any way this could be a breaking change? E.g. are we treating slot={boolean} or slot={null} or slot={undefined} or any other non-string values any different from before?

[jmsjtu]

@nolanlawson I have a test in the @lwc/template-compiler to test the various combinations for values passed to the slot attribute.

Here's a stackblitz demo using LWC 5.1.0 to compare.

I compared the tmpl functions and it looks like the resulting values are the same, the slot value is always transformed into a string.

I created this section by extracting what we were doing with the slot value in computeAttrValue and it should be doing the same thing as before.

[salesforce-nucleus]

❌ An unexpected error occurred while attempting to start the test stage of workflow 191513. Please try to start the stage again, or reach out to #nucleus-talk for help.

[salesforce-nucleus]

❌ An unexpected error occurred while attempting to start the test stage of workflow 191534. Please try to start the stage again, or reach out to #nucleus-talk for help.

[salesforce-nucleus]

❌ An unexpected error occurred while attempting to start the test stage of workflow 191536. Please try to start the stage again, or reach out to #nucleus-talk for help.

[salesforce-nucleus]

❌ An unexpected error occurred while attempting to start the test stage of workflow 191537. Please try to start the stage again, or reach out to #nucleus-talk for help.

[nolanlawson]

LGTM, just a minor note about hoisting a variable, and also of course we need to update the ACT Compiler.

If there are significant breaking changes, we should also consider API versioning. This will be determined by testing downstreams.

[nolanlawson]

LGTM! Worst comes to worst, we can revert and then add it to the API versioning changes for LWC v6.0.0.

[jmsjtu]

@nolanlawson I ran through all the nucleus downstream and found the following:

1. There were only a few test failures because of snapshot tests
2. There was one test failure but it's because of a difference in the LWC engine vs compiler version.
3. There is one test failure because of a reliance on a query selector element.querySelector('div[slot=taxIncludedLabel]').

I mainly have a question about #3 above, do you think it's worth adding API versioning for this? They could always just update the selector themselves.

I also ran through the core jest tests and didn't find any snapshots that needed to be updated.

[nolanlawson]

@jmsjtu The slot issue is the most worrisome to me. How much trouble would it be to do this through API versioning?