stencil icon indicating copy to clipboard operation
stencil copied to clipboard
verified publisher icon ionic-team

feat(renderer): add flag to update default slot text content

Open yigityuce opened this issue 2 years ago • 2 comments

add "experimentalDefaultSlotTextContentFix" flag to update text content of the default slot instead of update all the content

fixes one of the issues raised in #5335

What is the current behavior?

As of now, when the "experimentalScopedSlotChanges" flag is set, it removes all the content of the custom component (including named slots) and then inserts the text.

GitHub Issue Number: #5335

What is the new behavior?

When the newly introduced "experimentalDefaultSlotTextContentFix" is set, updating textContent updates only the "default slot" content while keeping the named slot contents.

Documentation

Does this introduce a breaking change?

  • [ ] Yes
  • [ ] No

Testing

Other information

yigityuce avatar Feb 12 '24 12:02 yigityuce

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1150 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
Path Error Count
src/dev-server/index.ts 37
src/dev-server/server-process.ts 32
src/compiler/prerender/prerender-main.ts 22
src/testing/puppeteer/puppeteer-element.ts 22
src/runtime/client-hydrate.ts 20
src/screenshot/connector-base.ts 19
src/runtime/vdom/vdom-render.ts 17
src/dev-server/request-handler.ts 15
src/compiler/prerender/prerender-optimize.ts 14
src/compiler/sys/stencil-sys.ts 14
src/compiler/transpile/transpile-module.ts 14
src/sys/node/node-sys.ts 14
src/compiler/prerender/prerender-queue.ts 13
src/compiler/sys/in-memory-fs.ts 13
src/runtime/connected-callback.ts 13
src/runtime/set-value.ts 13
src/compiler/output-targets/output-www.ts 12
src/compiler/transformers/test/parse-vdom.spec.ts 12
src/compiler/transformers/transform-utils.ts 12
src/mock-doc/test/attribute.spec.ts 12
Our most common errors
Typescript Error Code Count
TS2322 361
TS2345 350
TS18048 201
TS18047 91
TS2722 37
TS2532 24
TS2531 22
TS2454 14
TS2790 11
TS2352 10
TS2769 8
TS2538 8
TS2416 6
TS2493 3
TS18046 2
TS2684 1
TS2430 1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports
File Line Identifier
src/runtime/bootstrap-lazy.ts 21 setNonce
src/screenshot/screenshot-fs.ts 18 readScreenshotData
src/testing/testing-utils.ts 198 withSilentWarn
src/utils/index.ts 145 CUSTOM
src/utils/index.ts 269 normalize
src/utils/index.ts 7 escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts 25 BUILD
src/compiler/app-core/app-data.ts 115 Env
src/compiler/app-core/app-data.ts 117 NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts 123 updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts 61 satisfies
src/compiler/types/validate-primary-package-output-target.ts 61 Record
src/testing/puppeteer/puppeteer-declarations.ts 485 WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts 7 writeFetchSuccessSync

github-actions[bot] avatar Feb 12 '24 12:02 github-actions[bot]

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8032920823/artifacts/1272284462

If your browser saves files to ~/Downloads you can install it like so:

npm install ~/Downloads/stencil-core-4.12.3-dev.1708805475.9276646.tgz

github-actions[bot] avatar Feb 13 '24 08:02 github-actions[bot]

Thanks for the contribution @yigityuce 🙏

We have discussed this idea/proposal with the team and have decided that this is not something we would like to support. It creates a behavior that contradicts with how the Shadow DOM behaves which is not a direction we would like to take. Furthermore we believe that you can achieve your goals by modifying the query in your example, e.g.:

<body>
  <my-component>
    <span slot="prefix">Prefix</span>
    <span slot="suffix">Suffix</span>
    <span>Content text</span>
  </my-component>
  <button onclick="onButtonClick()" style="margin-top: 48px">Update content</button>
</body>
<script>
  function onButtonClick() {
    const defaultSlot = document.querySelector('my-component span:not([slot])');
    defaultSlot && (defaultSlot.textContent = `${Date.now()}`);
  }
</script>

Is there any reason this wouldn't work for you?

christian-bromann avatar Mar 06 '24 00:03 christian-bromann

hi @christian-bromann

Is there any reason this wouldn't work for you?

What if I don't wrap the "Content text" with span element and want to update it? what would be the selector to get that textNode.

yigityuce avatar Mar 07 '24 12:03 yigityuce

EDIT: this case is not valid, please IGNORE this message

and one more example that comes up my mind is like that:

// component lib file
@Component({  tag: 'my-component' })
export class MyComponent {
  @Event() randomize: EventEmitter<void>;
  @Event() reset: EventEmitter<void>;
 
  render() {
    return (
       <Host>
         <button onClick={() => this.randomize.emit()}>Randomize</button>
         <slot />
         <button onClick={() => this.reset.emit()}>Reset</button>
       </Host>
    );
  }
}

// in react context

export const Example: FC = () => {
    const [counter, setCounter] = useState(0);
    return (
      <>
        <my-component onReset={() => setCounter(0)} onRandomize={() => setCounter(Date.now() % 10)}>
          {counter}
        </my-component>
        <button onClick={() => setCounter(counter + 1)}>Increment</button>
      </>
    );
}

<!-- Expected rendered DOM after clicking increment button -->
<my-component>
  <button>Randomize</button>
  1
  <button>Reset</button>
</my-component>
<button>Increment</button>


<!-- Actual rendered DOM after clicking increment button -->
<my-component>
  1
</my-component>
<button>Increment</button>

because current behavior is to remove all nodes first and then insert an new text to to the host

yigityuce avatar Mar 07 '24 13:03 yigityuce

and one more example that comes up my mind is like that:

Can you share this as a Stackblitz project?

christian-bromann avatar Mar 07 '24 16:03 christian-bromann

Hi @christian-bromann

Can you share this as a Stackblitz project?

I tried to reproduce but I realized it is my bad, it is NOT a valid case, so pls ignore that example.

However, during this try-out, I found out another edge case with the given query selector to query the not slotted element. document.querySelector('my-component span:not([slot])')

I directly copied the content of the files here to explain and also created a new branch with an updated version of the repro repo.

@Component({
  tag: 'my-component',
  styleUrl: 'my-component.css',
  shadow: false,
  scoped: true,
})
export class MyComponent {
  render() {
    return (
      <Host>
        <div class="some-fancy-styles" id="prefix-wrapper">
          <slot name="prefix" />
        </div>
        <slot />
        <div class="some-fancy-styles" id="suffix-wrapper">
          <slot name="suffix" />
        </div>
      </Host>
    );
  }
}


<!DOCTYPE html>
<html dir="ltr" lang="en">
  <head>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=5.0" />
    <title>Stencil Component Starter</title>

    <script type="module" src="/build/stencil-v4-sc-default-slot-textcontent-update.esm.js"></script>
    <script nomodule src="/build/stencil-v4-sc-default-slot-textcontent-update.js"></script>
  </head>
  <body>
    <my-component>
      <span slot="prefix">Prefix</span>
      <span slot="suffix">Suffix</span>
      <div>Content text</div>
    </my-component>
    <button onclick="onButtonClick()" style="margin-top: 48px">Update content</button>
  </body>
  <script>
    function onButtonClick() {
      const contentTextWrapper = document.querySelector('my-component div:not([slot])');
      console.log('not slotted:', contentTextWrapper)
      contentTextWrapper && (contentTextWrapper.textContent = `${Date.now()}`);
    }
  </script>
</html>

so the thing is when you hit the button, the prefix-wrapper div's textContent gets updated because it matches with 'my-component div:not([slot])' selector.

The only way that I see to accomplish this is to provide a unique selector to select <div>Content text</div> element. In that option, you should be aware of the custom component's internal DOM structure to avoid collision in the selectors which should not be the end users' concern for those who consume the UI kit.

Or, a big or, being able to update only the default slot element while mutating the custom component's textContent (which is the solution I propose). What would be the drawbacks of this proposal and the mitigation steps:

  1. Case: If someone wants to change named slot content -> Mitigation: they can always use this selector: 'component-name [slot="slot-name"]'.
  2. Case: If someone wants to put just the text by clearing out all the slots -> Mitigation: they can remove all the slot elements in the custom component and update the custom component's textContent attr. By applying my proposal, this will work as well since only the default slotted element will be updated and user won't need to query this not slotted element which has some potential collision as of now. (also default slotted textNode is not queryable)

Summary: I mean, removing all the content & updating the custom component's textContent has a workaround, but it is not the case for the default slot with textNode.

One more thing might work in future but it's not usable as of today because of the browser compatibility: ::target-text

yigityuce avatar Mar 07 '24 19:03 yigityuce

so the thing is when you hit the button, the prefix-wrapper div's textContent gets updated because it matches with 'my-component div:not([slot])' selector.

Try to make the selector more strict, e.g. my-component > div:not([slot]). You can also loop through the child nodes if you need an even more strict selector.

What would be the drawbacks

The drawbacks that caused the team to push back on this proposal are:

  • this introduces a diverge in how the component behaves compared to the Shadow DOM spec
  • while this seems harmless at first glance it requires us to teach the community that this specific behavior differs from the specification
  • it requires us to maintain this specific edge case when there is no technical reason to introduce this

One more thing might work in future but it's not usable as of today because of the browser compatibility: ::target-text

I am not sure if this selector actually addresses this.

I feel inclined to close the issue. Please see above reasons why we don't feel comfortable merging the proposal. The DOM API provides various options that allows to query the element you like to update. We don't see any need to provide workarounds that introduce contradicting behavior to the Shadow DOM spec.

christian-bromann avatar Mar 11 '24 16:03 christian-bromann

hey @christian-bromann

I understand your concerns and you have a valid point so I'm closing this PR. Thanks for the discussion and for showing the point of your view clearly.

yigityuce avatar Mar 13 '24 18:03 yigityuce