fix(shadowDomShim): slot behaviour

[johnjenkins]

This PR looks to fix a number of issues relating to slots and how they are managed when a component is not shadow: true OR if the browser in question is using the shadowDomShim. Hence forth, I'll refer to these as 'non-shadow' components.

At present in order for non-shadow components to have similar behaviour to shadow-components (when it comes to the dom api) - stencil provides 2/3 opt-in extras: appendChildSlotFix, cloneNodeFix and slotChildNodesFix (https://stenciljs.com/docs/config-extras).

'appendChildSlotFix' - when you write myComponent.appendChild(newElement);, the new element will arrive in an appropriate slot - not just be appended to the bottom of your non-shadow component.

'slotChildNodesFix' - when you write myComponent.childNodes | myComponent.children only the slotted nodes / elements - not all nodes in the non-shadow component dom - are returned.

I feel quite strongly, that this ^ makes little sense. Until very recently I assumed that shadow: true On an old browser would automatically try to mimic shadow components. I'm pretty sure a number of bugs reported are the result of simply not having these extras applied. Additionally, at present, applying these extras will also (totally unnecessarily) apply them toshadow: true components on modern browsers!

This PR

This PR applies both aforementioned patches automatically on all non-shadow components and also adds new patches for nearly all DOM methods & accessors relating to the addition of new nodes: innerHTML, innerText, textContent, appendChild(), append(), prepend(), insertAdjacentText(), insertAdjacentElement(), insertAdjacentHTML(), replaceChildren()

With these, I believe non-shadow components feel more similar to shadow components. I also believe many of the kinks related to non-shadow components within frameworks (vue / react et al) will go away - I'll do more testing to confirm this. The vdom renderers of these libraries will add nodes via these methods when all is said and done.

All 'native' methods / accessors are accessible by simply prepending '__' to the relevant thing: i.e. myComponent.__innerHTML or myComponent.__appendChild(newThing); think of it as the equivalent of myShadowComponent.shadowRoot.innerHTML. I did think about patching a mock myComponent.shadowRoot but it felt over-engineered and silly.

cloneNodeFix is still not automatically applied as I thought this was a more niche requirement(?). However now it will only apply to non-shadow components.

I've added a deprecated flag to both the 'appendChildSlotFix' and 'slotChildNodesFix' extras within stencil.config. Continuing to include them will not break anything.

Lastly, I've made a small fix to stencil's vdom-renderer. Here's a silly example:

  SlotWrapper = () => {
    if (this.aBoolean) {
      return <span><slot name="bool" /></span>
    } else return <div><slot name="bool" /></div>
  }
  render() {
    return 
      <div>
        <this.SlotWrapper />
      </div>;
  }

At present, within a non-shadow component whenever this.aBoolean changes, the<slot/> is deemed to have been removed and so all it's currently slotted children are also removed.

This PR also fixes that.

Fixes #2004 Fixes #2641 Fixes #1968 Fixes #1997 Fixes #2801 Fixes #2257 Potentially fixes #2259 Potentially fixes #2036 Heavily referenced #2012

I’ve done quite a bit of testing in IE11, Edge 18 and all modern browsers. All current tests within stencil (spec / end-to-end via jest and karma) continue to pass.

New commit!

Commit https://github.com/ionic-team/stencil/pull/2938/commits/f9a00632919d39886c3c950feb2c942f74ff9c8f sees an overhaul of non-shadow component fallback content.

(I did think about putting this in a separate PR, but the issues and requirements were very closely linked (and difficult to untangle) so it made more sense to add it here).

What is the current behavior? In non-shadow components, slot fallback content behaviour is re-created by wrapping all fallback nodes in an <slot-fb> element. Wrapping nodes can create issues regarding css presentation (e.g. flex children)

What is the new behavior? In non-shadow components, slot fallback content nodes are no longer wrapped. Instead, in the same way that non-shadow components store slot meta in an empty text node, this same text node now stores data regarding fallback content.

Does this introduce a breaking change? No

Testing All karma & Jest tests have been updated to reflect the behaviour / lack of wrapping element. I have also manually tested changes in an external, linked project. Also, there was a lack of tests within Stencil for nested slots (e.g. <slot name="content"> <slot></slot> <slot name="after"></slot> </slot>). I've added new spec tests to cover this.

Additionally I have also now patched all, node.remove() and parentNode.removeChild() (and all aforementioned add methods ^) on non-shadow slotted nodes - when there is slot fallback content - to assess whether fallback content should show or hide. I have also tested and amended my changes against open issues related to slot fallback content so have marked them as fixes here.

Fixes #2937 Fixes #2878 Fixes #2997 Potentially fixes #2877

[johnjenkins]

To any requiring this sooner, rather than later I built out a temporary package of all my recent PRs - including this ^.

What else: new @Prop getters / setters (#2899) new @Mixin decorator (#2921) new sourceMap option for all output (including integration with @Mixins) (#2892)

You can test it out with little disruption by just swapping out any reference to @stencil/core in your package.json e.g.

"@stencil/core": "2.6.0"

becomes

"@stencil/core": "npm:@johnjenkins/stencil-community^2.4.0"

I will keep this package in-sync with @stencil/core master up until the relevant PRs get accepted or rejected. Many thanks!

[eriklharper]

@johnjenkins I tried installing your temp package on my repro setup for #2801 but after I npm i the npm start command instantly fails. Do I need to purge the npm cache or remove node_modules and reinstall everything that way?

[johnjenkins]

Thanks for raising @eriklharper - yeah try purging - otherwise give me an hour and I’ll see what’s up. You on windows?

[eriklharper]

Mac Catalina 10.15.7

[eriklharper]

Tried rm -rf node_modules && npm cache clean --force && npm i but it doesn't create even a node_modules folder.

[johnjenkins]

hmm weird - it is a thing :) https://www.npmjs.com/package/@johnjenkins/stencil-community .. try "@stencil/core": "npm:@johnjenkins/stencil-community@^2.0.0" ?

[eriklharper]

I'm still seeing the issue with your fix applied. Here's my repro case if you want to try this out yourself, its just a stencil-component-starter repo so you just install your package of stencil core and run npm start. Here's what the bug looks like:

[johnjenkins]

@eriklharper thanks! Shame - was just a hope. I'll remove it from the fix list for now, but I'll get to the bottom of it.

[eriklharper]

Thanks for looking into this @johnjenkins ! This has long been a thorny issue and we're currently trying to work around it by ditching scoped components altogether, but if you can come up with a fix that will be splendid!

[johnjenkins]

@eriklharper good news! "@stencil/core": "npm:@johnjenkins/stencil-community@^2.4.1" (I bumped the version up to squash some 'min-version stencil version required' warnings)

[eriklharper]

💥 This is great @johnjenkins ! It's working for me in my repro setup! I see that the comment node appears just like the other use case that uses a wrapping element around the text.

[bitflower]

@eriklharper SO this is the working version?

"@stencil/core": "npm:@johnjenkins/stencil-community@^2.4.1"

[PaulRaschke]

Hi @johnjenkins. Many thanks for your work! It does indeed fix the example component I posted in my issue. I also think your additional added features are really valuable. I'm currently using your distribution with "@stencil/core": "npm:@johnjenkins/stencil-community".

But unfortunately I discovered 2 new issues introduced by your changes:

• Following up on the example component in my issue, the following problem arises once a display style is set for the items (other than display: none, which the browser defaults to for elements with the attribute hidden): the items that are used as default slot content are rendered even if slot content is passed to the component (same behavior as reported in this issue). The cause seems to be that the elements are no longer wrapped inside a hidden slot-fb element. As far as I know, the only way I can solve this is to globally force elements with the attribute hidden to be visually hidden via css.

import { Host, Component, h } from '@stencil/core';

@Component({
  tag: 'my-component',
  styles: `
    :host {
      display: flex;
      gap: 0.5rem;
    }

    .item, ::slotted(.item) {
+     display: block;
      border: 2px solid #ccc;
      padding: 0.25rem;
    }
  `,
  shadow: false,
  scoped: true,
})
export class MyComponent {
  render() {
    return (
      <Host>
        <slot>
          <div class="item">Default #1</div>
          <div class="item">Default #2</div>
          <div class="item">Default #3</div>
        </slot>
      </Host>
    );
  }
}

• Component structures with nested slots are broken. This component is rendered incorrectly and throws errors with your package:

import { Component, Host, h } from "@stencil/core";

@Component({
  tag: "my-component",
  styles: `
    :host {
      display: block;
    }
  `,
  scoped: true,
})
export class MyComponent {
  render() {
    return (
      <Host>
        <slot name="content">
          <slot name="before">DEFAULT BEFORE</slot>
          <slot> DEFAULT CONTENT </slot>
          <slot name="after">DEFAULT AFTER</slot>
        </slot>
      </Host>
    );
  }
}

This component yields the following results:

<my-component> CONTENT </my-component>

→ Only CONTENT is rendered instead of DEFAULT BEFORE CONTENT DEFAULT AFTER and the following error is thrown TypeError: parentElm.classList is undefined

<my-component>
  <span slot="before">BEFORE</span>
  CONTENT
  <span slot="after">AFTER</span>
</my-component>

→ Correctly renders as BEFORE CONTENT AFTER, but throws the same error

<my-component></my-component>

→ doesn't render at all instead of rendering as DEFAULT BEFORE DEFAULT CONTENT DEFAULT AFTER and throws the same error

Changing scoped: true to scoped: false results in the following behavior:

<my-component> CONTENT </my-component>

→ Correctly renders as CONTENT, but throws DOMException: Node.appendChild: Cannot add children to a Text

<my-component></my-component>

→ Doesn't render at all and throws the same error

None of these issues occur with @stencil/[email protected]. With shadow: true, everything works as expected, both with your distribution and with the official distribution.

Again, many thanks for your work. It would be great if you could take a look at these problems.

(Tested with Firefox 89.0.2 and Chrome 91.0.4472.124) (ERROR messages as reported by Firefox)

[johnjenkins]

hey @PaulRaschke - thanks so much for taking time to test this! Issue 1, I think I'm out of ideas (unless you can think of something?) - I feel like you have more options with the items not being wrapped though. e.g. as you say, applying [hidden] { display: none !important; } or amending the classes you use for slotted vs default elements. Issues 2 - I'll get on it as soon as I can! - interesting there are no tests for nested slots in the stencil codebase atm

[eriklharper]

@eriklharper SO this is the working version?

"@stencil/core": "npm:@johnjenkins/stencil-community@^2.4.1"

Yes indeedly doodly

[johnjenkins]

Converted to draft whilst I iron out the issues with nested slots

[PaulRaschke]

@johnjenkins

I feel like you have more options with the items not being wrapped though

Yes, there are a few different options to solve this. Nevertheless, I think to solve this optimally, something has to be changed at compiler/runtime level, because this is probably the only way to achieve consistency with shadow: true and shadow: false and thus between browsers without the stencil user having to take action himself. I'd like to help, but I don't think I understand the stencil core and your changes sufficiently well yet. But I will certainly take a deeper look at it! After a quick look, are there any objections to adding something in the form of:

if (hide === true) n.style.display = "none";

to renderSlotFallbackContent?

I'm not sure what the reason is for inserting the fallback slot content into the DOM in the first place?

Thanks for working on a fix for the nested slot bug!

[johnjenkins]

@PaulRaschke Ha - Your solution is so very obvious and I can't see any real disadvantage - I feel pretty silly! :D

I've committed it - along with the nested slot fixes - and built them to my package if you get another chance to check it out: "@stencil/core": "npm:@johnjenkins/stencil-community@^2.4.2"

I'm not sure what the reason is for inserting the fallback slot content into the DOM in the first place?

I'm pretty sure it's due to a mixing of a vdom / jsx coupled with slotted nodes that are not part of that vdom; slotted nodes essentially get moved around the vdom after it's been rendered. vdom renders > slotted nodes are repositioned in that vdom > fallback content gets hidden or shown depending.

If all's good, I'm going to write some tests within stencil for nested slots / fallback content

[PaulRaschke]

@johnjenkins Happy to help :D

Thanks for the insights regarding the vDOM/DOM rendering.

Everything seems to be working flawlessly with the new version 👍 Thanks, nice work!

[vmcbaptista]

I was also facing some issues with slots when shadow: false with latest @stencil/core. Specifically, fallback content being always hidden when no content is passed into the slot. Tried today the latest version of johnjenkins/stencil-community and it works perfectly. Great job @johnjenkins!

@ltm I see that you have been reviewing the latest merged pull requests, would be possible to indicate any ETA for when this could be reviewed, merged and released officially on @stencil/core package?

[vmcbaptista]

@johnjenkins I identified an issue with the types generated when using johnjenkins/stencil-community.

I have some @Prop with type provided by an external package. The generated types is generating with imports like this import {type} from "relative_path_to_node_modules/name_of_package_containing_type/file_defining_type.d"

With @stencil/core it imports without any reference to node_modules so like this import {type} from "name_of_package_containing_type"

I tried several versions of your package, and all seem to have this issue.

This issue prevents consumers to use my web components when compiled with your version 😢

[johnjenkins]

@vmcbaptista sorry to hear that! Do you happen to have a minimum repro i could look at?

[vmcbaptista]

@johnjenkins I've quickly done a minimum repo https://github.com/vmcbaptista/stencil-types-issue/

This commit shows the difference that happens to the auto generated typings after updating to your version https://github.com/vmcbaptista/stencil-types-issue/commit/beed97cefd1bea412fb7ae67ce92c117c2d85cbc

Although, in my real-life scenario the dependency is other than uuid, the same thing happens, so I guess you can rely on this 😄

[johnjenkins]

@vmcbaptista thanks so much for this - great catch (nothing to do with this PR, but still really helpful). I've pushed a fix now - "@stencil/core": "npm:@johnjenkins/stencil-community@^2.5.4" :)

[vmcbaptista]

@johnjenkins it's working fine. thanks 😄

[PaulRaschke]

Hi @johnjenkins, I've been using your stencil-community build quite extensively over the last few weeks and almost everything seems to be working fine!

Unfortunately, I've discovered another bug. The following component throws a TypeError: fbNodes[fbSlot['s-sn']] is undefined error when the text prop is updated. The error doesn't seem to completely break the component, as the text prop continues to be updated correctly. The same behavior is observed when either a mutable prop is updated from inside the component or a mutable or non-mutable prop is updated from outside. The bug isn't directly caused by the prop update, but instead occurs during the component update or rendering, as it also happens when updating the component with forceUpdate(componentRef).

import { Component, Host, h, Prop } from '@stencil/core';

@Component({
  tag: 'my-component',
})
export class MyComponent {
  @Prop({ mutable: true }) text: string = '';

  componentDidLoad() {
    this.text = 'Text';
  }

  render() {
    return (
      <Host>
        <slot name="content">
          <slot name="text">{this.text}</slot>
        </slot>
      </Host>
    );
  }
}

This bug doesn't occur with @stencil/[email protected].

(Tested with Firefox 90.0.2 and Chrome 92.0.4515.131 with @stencil/core@npm:@johnjenkins/stencil-community@^2.5.4) (ERROR messages as reported by Firefox)

[johnjenkins]

hey @PaulRaschke thanks for this again! Unfortunately with the following

import { Component, Host, h, ComponentInterface, Prop } from "@stencil/core";

@Component({
  tag: "my-component",
})
export class MyComponent implements ComponentInterface {
  @Prop({ mutable: true }) text: string = '';

  componentDidLoad() {
    this.text = 'Text';
  }

  render() {
    return (
      <Host>
        <slot name="content">
          <slot name="text">{ this.text }</slot>
        </slot>
      </Host>
    );
  }
}

I'm not seeing any issues atm :/ - maybe you could make a zip / repo?

*edit - I'm seeing a different issue which I'll fix and maybe this will make this go away

[johnjenkins]

@PaulRaschke - the issue I experienced was that dynamic slot fallback text (like in your example) would show after update / re-render even when there is active slotted content. I've fixed that now and whilst doing so, I've tried a cargo-cult attempt to squash the error you're seeing - hopefully it works!

@stencil/core@npm:@johnjenkins/stencil-community@^2.5.5

[PaulRaschke]

@johnjenkins - @stencil/core@npm:@johnjenkins/[email protected] fixes the issue :tada: Strange that you couldn't reproduce the bug with v2.5.4. I cleaned any cache and did a clean install, but the issue persisted. If you're still interested in getting to the root of the problem, and the problem doesn't occur when you set the version to 2.5.4, I can set up a repro repo. BTW, I use Windows, though I don't think there could be a correlation with the problem.

I hope this PR will be reviewed and merged soon :)

[Paul-Hebert]

This is great @johnjenkins ! This appears to fix the issue I reported in #2997!

It looks like I'm still running into some issues related to #2232 but those are likely outside the scope of this PR.

Nice work! Thanks!

(I have not tested extensively in my main repo yet.)