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(scoped): fixes for <slot /> and slotted nodes #6082

Merged

Conversation

johnjenkins
Copy link
Contributor

@johnjenkins johnjenkins commented Dec 18, 2024

What is the current behavior?

A bunch of bugs related to scoped: true

GitHub Issue Number:

What is the new behavior?

All attached bugs have been fixed combined with a bunch of refactoring (see below for in-depth reasoning).

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

  • New unit tests have been added for scoped slot classes addition and removal.
  • New e2e tests have been added to check for correct SSR slot / vdom resolution and duplicate style tags.
  • New wdio tests have been tweaked to check of ::slotted(...) selector specificity and making sure slotted nodes do not receive scoped classes.

Other information

image
image
image

TL version

Refactored a bunch of internals in vdom-render.ts

Removed updateElementScopeIds() & findScopeIds() (in-favour of new addRemoveSlotScopedClass())

These methods combined would (too regularly) loop through an entire component tree adding scoped class variants (internal (sc-...), slot (internal + ...-s), and host (internal + ...-h)).

This behaviour is incorrect for a number of reasons:

Children component internals receiving parent scoped class

Given an internal setup like:

// cmp-b
<div class="internal"><slot /></div>
// cmp-a
<cmp-b><slot /><cmp-b>

cmp-bs internal div would receive the internal scope class sc-cmp-a - when it should not as it's not owned by cmp-a in any way.

All component internals receiving the scoped slot class

The scoped slot class (e.g. sc-cmp-a-s) is a hook to use with ::slotted(...) type selectors. Currently it can be applied to the entire parent tree:

<cmp-a class="sc-cmp-a-s ...">
  <div class="sc-cmp-a-s ...">
    <div class="sc-cmp-a-s ...">
      <slot />
  ...

This is incorrect - only the element surrounding the slot should have the class. The result is that ::slotted(...) selectors are applied too liberally.

Scope classes being applied to slotted elements

At present scoped classes apply to slotted elements (elements coming from outside of the component internals). This is incorrect as slotted elements are not 'owned' by the component and results in the behaviour in #6081.

New behaviour

All internal scoped classes are appropriately managed as elements are created. To manage the appropriate scoped slot class, as <slot /> nodes move around the new addRemoveSlotScopedClass() adds / removes the class.

Refactored updateFallbackSlotVisibility()

updateFallbackSlotVisibility() seemed unnecessarily opaque and unreliable. Much of the logic for slot & slotted nodes are managed quite succinctly within dom-extras.ts so I reused it.
As part of that process I moved all shared slot polyfill logic into a new slot-polyfill-utils.ts file.

Tweaks to isSameVnode()

isSameVnode would intentionally not resolve <slot /> / <slot-fb /> nodes during SSR, forcing them to be reconstructed on the client. This had the side-effect of incorrectly resolving the overall VDOM in some instances (#6080).
I've made it so a combination of changes to client-hydrate.ts & mock-doc/element.ts now provide everything needed during SSR - these nodes no longer need to be reconstructed.

@johnjenkins johnjenkins changed the title fix(ssr): scoped: true fixes for <slot /> and slotted nodes fix(scoped): fixes for <slot /> and slotted nodes Jan 3, 2025
@johnjenkins johnjenkins marked this pull request as ready for review January 3, 2025 13:17
@johnjenkins johnjenkins requested a review from a team as a code owner January 3, 2025 13:17
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@christian-bromann
Copy link
Member

@johnjenkins it seems like the WebdriverIO test fail for legit reasons.

@johnjenkins
Copy link
Contributor Author

@christian-bromann ty - test snapshots were incorrect. Updated now.

@christian-bromann christian-bromann merged commit 13ee704 into ionic-team:main Jan 4, 2025
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants