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

fix(shadowDomShim): slot behaviour #2938

Closed
wants to merge 71 commits into from

Conversation

johnjenkins
Copy link
Contributor

@johnjenkins johnjenkins commented Jun 25, 2021

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

  1. assumed that shadow: true On an old browser would automatically try to mimic shadow components.
  2. assumed that shadow: false / scoped: true would not behave so differently regarding native APIs.

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 to shadow: 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
Fixes #3913
Fixes #3977
Fixes #4284
Fixes #4523
Fixes #4525
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 f9a0063 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?
Potentially - if anyone was using the slot-fb for reference or styling, that will now not work.

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() 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
Fixes #3278
Potentially fixes #2877

New commits!

New commits address client side hydration (after using something like renderToString()).

Fixes #3413
Fixes #3593
Fixes #2905
Fixes #2366
Fixes #2036
Fixes #1993

Whilst investigating, a number of these issues were related to <slot />s and how they are managed.
Additionally my changes around slot fallback content weren’t working with server / client hydration.
Finally, I found when working with frameworks (like nuxt) they would attempt to resolve their vdom against the real DOM after SSR by stepping through it with accessors like ‘nextSibling’. This fails for non-shadow elements so nuxt duplicates the nodes it thinks it cannot find.
To mitigate this I’ve patched these accessors just for non-shadow, client hydrated, slotted nodes.

A number of these bugs are demoed - https://stackblitz.com/edit/stencil-start-yoemx6
And here's the same demos with this PR applied - https://stackblitz.com/edit/stencil-start-brthw2

…hods dealing with adding to components

fix(scoped / shadow-shim slots): vdom-render was removing all slotted children when a slot or slot parent was removed
apply dom patches to no-encapsulated components.
@johnjenkins johnjenkins changed the title fix & feature(scoped / shadow-shim slots) fix & feature(scoped / shadowDomShim slot behaviour) Jun 25, 2021
@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jun 25, 2021

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
Copy link

@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?

image

@johnjenkins
Copy link
Contributor Author

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

@eriklharper
Copy link

Mac Catalina 10.15.7

@eriklharper
Copy link

eriklharper commented Jul 1, 2021

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

image

@johnjenkins
Copy link
Contributor Author

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
Copy link

eriklharper commented Jul 1, 2021

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:

image

@johnjenkins
Copy link
Contributor Author

@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
Copy link

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
Copy link
Contributor Author

johnjenkins commented Jul 1, 2021

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

@eriklharper
Copy link

💥 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.

image

@bitflower
Copy link
Contributor

@eriklharper SO this is the working version?

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

@PaulRaschke
Copy link

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/core@2.6.0. 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
Copy link
Contributor Author

johnjenkins commented Jul 2, 2021

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
Copy link

@eriklharper SO this is the working version?

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

Yes indeedly doodly

@choshin314
Copy link

Hi just a heads up, I brought in @stencil/core@3.3.0-dev.1685496483.a311f36 for testing in our Nuxt 3 app. Unfortunately for non-shadow DOM components w/ slots I am still seeing the same behavior as before, i.e. the ***

This was a false alarm on my part – as I was working on getting a minimum reproduction together I realized that the slotted component that I was testing had invalid html structure. After reworking that, was able to confirm this works as expected. Thanks again, and sorry for the distraction!

@pedroresende
Copy link

I've tested this and it seems to work, except for the Hydration errors that started to appear on Next.js

@pedroresende
Copy link

The only way, I've found to overcome the hydration problem was by adding a useEffect

  const [loaded, setLoaded] = useState<boolean>(false)

  useEffect(() => {
    if (!loaded) {
      setLoaded(true)
    }
  }, [loaded])

@pedroresende
Copy link

@JessicaSachs Since the release of version 4 was already done, could you guys take this PR into account 🙏🏻

@phhbr
Copy link

phhbr commented Jul 25, 2023

Is there already an ETA for this bugfix?

@antonselukh
Copy link

Is there already an ETA for this bugfix?

i also want to know.

@pedroresende
Copy link

Is there already an ETA for this bugfix?

i also want to know.

+1

@eWert-Online
Copy link

eWert-Online commented Jul 26, 2023

Hello 👋

I tested the released alpha version with our app.
The original issue we had (duplicate elements after hydration) is fixed. There are however two new issues introduced with this PR:

  1. We are getting a TypeError: e.split is not a function when rendering some elements. The problem originates in the setAccessor function when calling parseClassList. You are expecting a string to be given to parseClassList, however sometimes there are SVGAnimatedStrings passed to it, which leads to the error being thrown.
  2. There seems to be some kind of race-condition when accessing the shadow-dom inside componentDidLoad.
    We are having the following code inside one of our components:
    componentDidLoad() {
     const els = this.el.shadowRoot.querySelector("slot").assignedElements({ flatten: true });
     ...
    }
    
    Sometimes the querySelector("slot") is returning an element and sometimes it returns undefined.
    This race-condition seems to only happen with a large amount of stencil components.

I have updated my reproduction repository with this behavior: https://github.com/eWert-Online/stencil-ssr-hydrate-reproduction/tree/alpha-test

You can also see the resulting behavior in this video:

reproduction-video.webm

@mayerraphael
Copy link

Hello 👋

I tested the released alpha version with our app. The original issue we had (duplicate elements after hydration) is fixed. There are however two new issues introduced with this PR:

  1. We are getting a TypeError: e.split is not a function when rendering some elements. The problem originates in the setAccessor function when calling parseClassList. You are expecting a string to be given to parseClassList, however sometimes there are SVGAnimatedStrings passed to it, which leads to the error being thrown.

We are using johnjenkins/stencil-community@2.23.0 and can confirm that the e.split error with SVGs exists with this PR.
Created a bug issue some time ago: #4278

We worked around it by using innerHTML to set the svg.

@johnjenkins
Copy link
Contributor Author

johnjenkins commented Aug 11, 2023

@eWert-Online

The original issue we had (duplicate elements after hydration) is fixed. There are however two new issues introduced with this PR:

We are getting a TypeError: e.split is not a function when rendering some elements. The problem originates in the setAccessor function when calling parseClassList. You are expecting a string to be given to parseClassList, however sometimes there are SVGAnimatedStrings passed to it, which leads to the error being thrown.

As per @mayerraphael issue (#4278) it seems like this is an existing issue in the current codebase.. not sure I should make this PR any bigger :D / this bug is beyond the scope of this PR

There seems to be some kind of race-condition when accessing the shadow-dom inside componentDidLoad.
We are having the following code inside one of our components...This race-condition seems to only happen with a large amount of stencil components.

I just took your repo for a spin - it seems the SVG error halts the hydration process (before it's finished re-slotting elements) so causes the second error / 'race condition'

@mayerraphael
Copy link

@johnjenkins I can only reproduce the error with this PR. It seems to be related. I updated #4278 and linked a repo for reproduction.

@rwaskiewicz
Copy link
Member

Hey folks 👋

With today's release of Stencil v4.1.0, we introduced a new flag called experimentalSlotFixes. Currently, the only flag enables Stencil's pre-existing slot-related fix configuration flags. Going forward, the Stencil team is going to be working with @johnjenkins to incrementally add the fixes in this PR into Stencil under experimentalSlotFixes in subsequent releases 🚀

@rwaskiewicz
Copy link
Member

The following bug(s) have fixes included in today's v4.2.0 release:

alicewriteswrongs added a commit that referenced this pull request Sep 12, 2023
This adds a regression test for the issue reported in #4278, which
doesn't currently occur on `main` but does occur in the branch
associated with #2938 (a PR containing a bunch of changes for fixing
bugs with slots).

This regression test will give us a bit more safety as we go to add more
slot behavior fixes in the future.
alicewriteswrongs added a commit that referenced this pull request Sep 12, 2023
This adds a regression test for the issue reported in #4278, which
doesn't currently occur on `main` but does occur in the branch
associated with #2938 (a PR containing a bunch of changes for fixing
bugs with slots).

This regression test will give us a bit more safety as we go to add more
slot behavior fixes in the future.
alicewriteswrongs added a commit that referenced this pull request Sep 13, 2023
This adds a regression test for the issue reported in #4278, which
doesn't currently occur on `main` but does occur in the branch
associated with #2938 (a PR containing a bunch of changes for fixing
bugs with slots).

This regression test will give us a bit more safety as we go to add more
slot behavior fixes in the future.
alicewriteswrongs added a commit that referenced this pull request Sep 13, 2023
This adds a regression test for the issue reported in #4278, which
doesn't currently occur on `main` but does occur in the branch
associated with #2938 (a PR containing a bunch of changes for fixing
bugs with slots).

This regression test will give us a bit more safety as we go to add more
slot behavior fixes in the future.
alicewriteswrongs added a commit that referenced this pull request Sep 14, 2023
This adds a regression test for the issue reported in #4278, which
doesn't currently occur on `main` but does occur in the branch
associated with #2938 (a PR containing a bunch of changes for fixing
bugs with slots).

This regression test will give us a bit more safety as we go to add more
slot behavior fixes in the future.
alicewriteswrongs added a commit that referenced this pull request Sep 14, 2023
This adds a regression test for the issue reported in #4278, which
doesn't currently occur on `main` but does occur in the branch
associated with #2938 (a PR containing a bunch of changes for fixing
bugs with slots).

This regression test will give us a bit more safety as we go to add more
slot behavior fixes in the future.
github-merge-queue bot pushed a commit that referenced this pull request Sep 14, 2023
This adds a regression test for the issue reported in #4278, which
doesn't currently occur on `main` but does occur in the branch
associated with #2938 (a PR containing a bunch of changes for fixing
bugs with slots).

This regression test will give us a bit more safety as we go to add more
slot behavior fixes in the future.
@pedroresende
Copy link

The following bug(s) have fixes included in today's v4.2.0 release:

This problem is still occurring, even on version 4.3.0 with the experimentalSlotFixes option

@rwaskiewicz
Copy link
Member

@pedroresende

#2938 (comment)

Can you please open a new issue with a minimal reproduction case for the team to take a look at?

@pedroresende
Copy link

@pedroresende

#2938 (comment)

Can you please open a new issue with a minimal reproduction case for the team to take a look at?

Sure, thanks for the fast feedback @rwaskiewicz

@pedroresende
Copy link

@pedroresende
#2938 (comment)
Can you please open a new issue with a minimal reproduction case for the team to take a look at?

Sure, thanks for the fast feedback @rwaskiewicz

@rwaskiewicz After investigating more, I've concluded that it's actually not a stencil bug but actually it only occurs when using the React Wrapper, if I attempt to use without the wrapper everything works as expected. Therefore I've opened the ticket on stencil-ds-output-targets ionic-team/stencil-ds-output-targets#393

@tanner-reits
Copy link
Member

Hi all!

You may have seen we've been chipping away at a list of slot-related issues (pulled from this PR) over the last several months. These changes have been placed behind the (newly introduced) experimentalSlotFixes and experimentalScopedSlotChanges extras config flags. A list of issues these flags address can be found in their respective documentation.

With a majority of this work wrapped up, we're encouraging you all to enable and test out these changes so we can verify their correctness in a larger set of use cases. The plan is that these flags will be enabled by default in Stencil v5.

We do, however, want to note that these flags do not address SSR related issues that were mentioned in this PR. This includes #3413 and #2905. SSR related issues will be handled separately.

If anyone still experiences any of the issues claimed to be fixed by these config flags (or encounters any new issues), please open a new issue with a reproduction. Opening new issues will help us stay organized and prioritize fixes!

Lastly, we want to give a HUGE shoutout to @johnjenkins for his work on this PR! It was truly helpful as a starting point and reference while working through these issues. Thank you!

@ionic-team ionic-team locked and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.