fix(runtime): textContent for scoped components with slots

[rwaskiewicz]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • https://github.com/ionic-team/stencil-site/pull/767/files
• Build (npm run build) was run locally and any changes were pushed
• Unit tests (npm test) were run locally and passed
• E2E Tests (npm run test.karma.prod) were run locally and passed
• Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

GitHub Issue Number: #2801

The issue above points to a bug when using Stencil w/Maquette. The underlying cause is Stencil does not adhere to "the correct thing to do" when textContent is set, which Maquette is doing under the hood in the reproduction case when creating this node:

            h("calcite-label", {}, ["I am a text node and won't get properly relocated"]),

What is the new behavior?

patch textContent for scoped components that use slots. because there is
no shadow dom, calling the setter/getter on textContent would
otherwise act on the bare HTML elements. use the stencil metadata to
relocate the text to the proper location

this functionality is hidden behind a feature flag. users may enable it
by setting scopedSlotTextContentFix to true in the extras section
of their Stencil configuration file. This field defaults to false
(keeping the same behavior that exists today) while this flag is
evaluated.

Does this introduce a breaking change?

• Yes
• No

Testing

1. Pull down the reproduction in Slotted text node in scoped component is not relocated in the DOM properly when rendered with Maquette #2801, found at https://github.com/eriklharper/stencil-maquette-bug
2. Follow the reproduction steps in the issue to confirm the issue
3. Pull down this branch, and build Stencil npm ci && npm run build.prod && npm pack
4. In the reproduction, install the tarball that you generated in step 3 npm i <PATH_TO_TARBALL>
5. Update the stencil.config.ts file as follows:

+  extras: {
+    scopedSlotTextContentFix: true,
+  }

6. npm start, observe the text being slotted correctly:
   

Other information

this does not bring this functionality over to the custom elements
build. this will be done in a future story (added to the team refinement sheet)

the second commit contains some JSDoc I created as I combed through the code. they are not strictly related/necessary for the fix, but something that were tangentially related enough for me to feel it was worth including here. I can put them in a separate PR if y'all feel it's not a good time/place to add them

[johnjenkins]

Hey!
I included this fix in this PR #2938 when you get to it. Perhaps you may get a chance to see if it fixes your requirements
Just don’t wanna cause too much overlap :)

[rwaskiewicz]

Eagle eye! I did have your PR in mind when I was working on this 🙂 I definitely feel better equipped to start looking at that one in particular when I get the time after doing a piece of it myself. I'm pushing this subset of work out now to offer up a fix for an enterprise customer, but that doesn't mean this can't be usurped by the implementation in #2938. Just don't want you thinking we're ignoring it completely 😉

[johnjenkins]

Ha! Very wise.
I’m very well aware that that PR is massive and unwieldy and I should definitely have split it - apologies for that.
I had particular, regular snags from the community in mind at the time and so lumped them all together if they felt similar

[rwaskiewicz]

No worries - honestly after spending some time in the scoped-slot domain that PR's all the more impressive!

[splitinfinities]

this does not bring this functionality over to the custom elements
build. this will be done in a future story (added to the team refinement sheet)

Can you share a bit about this? Does some render code differ between output targets due to - I assume - Lazy Loading's extensive coupling? Can you see a path toward unifying that behavior?

[johnjenkins]

None of the optional patches apply to this output. I looked into this before and there didn’t appear to be any reason 🤷🏻‍♂️. These monkey patches can apply without issue to that output (in my PR I did that without obvious problems).

My best guesses are it was forgotten about(?) or there was a thought that it wouldn’t be a priority with the custom elements build seeing as the main / initial usecase was consumption by bundler meaning users can apply their own patches?

it is interesting to me that there hasn’t been any issues raised about it that I could find

[rwaskiewicz]

Can you share a bit about this? Does some render code differ between output targets due to - I assume - Lazy Loading's extensive coupling? Can you see a path toward unifying that behavior?

I think @johnjenkins is correct there (or, I came to the same conclusions laid out here).

The patch as defined in dom-extras.ts is agnostic of the output target that's being used. In this PR, we're only applying it to lazy loaded components.

ATM, serving the custom elements bundle will result in the text node NOT being placed in the correct place. So we should absolutely backport/unify this fix - I added this to the user story refinement sheet. I didn't implement it here to keep this PR as simple as possible (and to get this fix out to the customer sooner). My thought here was that once we tackled some of the testing infrastructure work we have planned for this quarter, we'd be in a great place to backport this with tests 🙂

[splitinfinities]

Worked great from my local tests!

[ltm]

Why would this need to access the next sibling? Using cmp-label as an example, I assume slotNode and slotNode.nextSibling are the following:

<Host>
  <label>
    <slot /> <!-- slotNode -->
    <div>Non-slotted text</div> <!-- slotNode.nextSibling -->
  </label>
</Host>

[johnjenkins]

it's because when not using shadow: true stencil tries to recreate the behaviour of slots using empty text nodes to preserve the slot position. Thus the next nodes after the slot text node, should be some slotted content.

[rwaskiewicz]

@ltm not exactly. Let's say we have a layout that looks something like:

<Host>
  <label>
    <slot />
    <div>Non-slot</div>
   </label>
</Host>

When we're doing slot relocation, calling childNodes on the label element yields:

where slotNode is that first text node, and slotNode.nextSibling in this case is the second text node.

When we call getHostSlotNode, it finds the first text node, and assigns it to slotNode because that node has the Stencil marker '[s-sn]` (slot name) set to an empty string (which is what we wanted).

However, in cases like this, the VDOM has placed the content in that second text node. Attempting to place the value received when setting textContent in the first node will result in concatenation of that received value (from setting textContent) and what's in the second text node. So, we check here for this case first.

However, you also presented a case internally where this isn't always the case. I've updated this PR with additonal tests and hopefully clearer comments. LMK what you think

[johnjenkins]

I've just checked this out .. didn't look too close before... hope it's helpful :)
A few things:

1. If I supplied nothing to the slot: <my-component></my-component> - it would return the next 'internal' element's content.
2. I found textContent returned an empty text node when I laid the html like so:

<my-component>
      <span>nice</span>
</my-component>

Due to the extra empty space.
3) With the following markup <my-component>nice<span>test</span></my-component> I found that it only returned the first text node
4) with the following component markup

render() {
    return (
      <Host>
        <slot />
        <div>internal</div>
      </Host>
    );
  }

textContent = 'something' blasts away the internal content

[rwaskiewicz]

Leaving a note for future us - we know this doesn't cover every edge case. We've marked this as experimental in the docs, and will likely be revisiting this in the future