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

bug/question: ref element validity is not up-to-date on Prop Watch trigger #5965

Open
3 tasks done
dpellier opened this issue Sep 3, 2024 · 3 comments
Open
3 tasks done
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@dpellier
Copy link
Contributor

dpellier commented Sep 3, 2024

Prerequisites

Stencil Version

4.21

Current Behavior

We are implementing form elements components using ElementInternals.
On our component value change, we want to update the internals validity to match the validity of the native form element embedded.

Using this kind of setup:

@Watch('value')
  onValueChange(): void {
    this.internals.setFormValue(value?.toString() ?? '');
    this.internals.setValidity(...); // with a loop over the form element flags to copy its validityState
}

This does work fine when the trigger comes from a change on the form element directly (like an onInput callback), but not when the Prop value of the component change, unless we delay it using a setTimeout 0.
(see example below and repro code repo)

Expected Behavior

I can guess why this behaviour is actually happening as the Watch trigger in the case of Prop change does not care about waiting for other DOM changes to occurs.
But I'm not sure the setTimeout 0 is the best way to handle this.

Do you have any best practices about how to sync the internals validity without using such hack?
Or is it something that can actually be fixed on the Watch side?

System Info

System: node 18.17.1
    Platform: darwin (23.4.0)
   CPU Model: Apple M1 (8 cpus)
    Compiler: /.../node_modules/@stencil/core/compiler/stencil.js
       Build: 1724698030
     Stencil: 4.21.0 🐷
  TypeScript: 5.5.3
      Rollup: 2.56.3
      Parse5: 7.1.2
      jQuery: 4.0.0-pre
      Terser: 5.31.1

Steps to Reproduce

Using a basic input component (or clone https://github.com/dpellier/stencil-set-validity-issue):

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

@Component({
  formAssociated: true,
  shadow: true,
  tag: 'my-component',
})
export class MyComponent {
  private inputElement?: HTMLInputElement;

  @AttachInternals() private internals!: ElementInternals;

  @Prop({ mutable: true, reflect: true }) public value: string = null;

  @Method()
  reset() {
    // Here the <input> has not been updated yet as we modify the Prop directly
    this.value = null;
  }

  @Watch('value')
  onValueChange(): void {
    // Here we want to update the internals value and validity using
    // this.internals.setFormValue(value?.toString() ?? '');
    // this.internals.setValidity(...); // with a loop over the inputElement flags to copy its validityState

    // If we arrive from the onInput, <input> is already updated so the value is accurate
    // But if we arrive from the reset Method, the <input> is not yet updated, so the it's still the old validityState
    console.log('onWatch, isValid?', this.inputElement.validity.valid);

    setTimeout(() => {
      // If we delay it, we can see the new expected value
      console.log('onWatch after timeout, isValid?', this.inputElement.validity.valid);
    }, 0);
  }

  private onInput(): void {
    // Here the <input> validityState has been updated already
    console.log('onInput, isValid?', this.inputElement.validity.valid);

    // So the Watch trigger will be able to set the internals validity with the <input> value correctly
    this.value = this.inputElement?.value ?? null;
  }

  render() {
    return (
      <Host>
        <input
          onInput={ (): void => this.onInput() }
          ref={ (el): HTMLInputElement => this.inputElement = el as HTMLInputElement }
          required={ true }
          type="text"
          value={ this.value || '' }>
        </input>
      </Host>
    );
  }
}

Run the example of the component with a reset button and check the console.log

<body>
    <label>Required input</label>

    <my-component>
    </my-component>

    <button>Reset value</button>

    <script>
      const component = document.querySelector('my-component');
      const resetButton = document.querySelector('button');

      resetButton.addEventListener('click', () => {
        component.reset();
      })
    </script>
  </body>
  1. Type a value on the input => all validity checks return true, as expected
  2. Click the reset button => first Watch log return true, which is wrong as the input is required, but the delayed log return true

NB: the reset Method is just an example, other usage can be problematic, like global form reset:

async formResetCallback(): Promise<void> {
  this.value = null; // will cause the same behaviour
}

Code Reproduction URL

https://github.com/dpellier/stencil-set-validity-issue

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Sep 3, 2024
@christian-bromann
Copy link
Member

Thanks for raising the issue @dpellier the Stencil team currently has a lot of competing priorities and can't promise to be able to take a look at this anytime soon. We recommend to get involved and help us resolve this bug by providing a pull request with a fix. We are happy to provide necessary guidance and appreciate any help!

@christian-bromann christian-bromann added Bug: Validated This PR or Issue is verified to be a bug within Stencil and removed triage labels Sep 5, 2024
@MatteoFeltrin
Copy link

@dpellier I have understood the problem here, but when you link a @prop to an element, you are re-rendering it every time, and I think that's not what you want.
value={ this.value || '' }>
This means that everytime you write something in the input, you update your local this.value and then the component is re-rendered uselessly.
I don't know your complete use-case, but from what I see here basically you just want to reset the component through an external call.

Try this approch, just rely on the native input event and to reset the value, act directly on the element it-self.
May this solve your use-case issue?

import { AttachInternals, Component, Host, Method, Prop, Watch, h, State } from '@stencil/core';

@Component({
  formAssociated: true,
  shadow: true,
  tag: 'my-component',
})
export class MyComponent {
  private inputElement?: HTMLInputElement;

  @AttachInternals() private internals!: ElementInternals;

  @Method()
  reset() {
    this.inputElement.value = null;
    // Directly acting on element value does not trigger the event automatically, so dispatch it yourself
    this.inputElement.dispatchEvent(new Event('input', { bubbles: true }));
  }

  private onInput(): void {
    // Do what you want here. Assign the value to a local variable if needed.
    console.log('onInput, isValid?', this.inputElement.validity.valid);
  }

  render() {
    return (
      <Host>
        <input
          onInput={(): void => this.onInput()}
          ref={(el): HTMLInputElement => (this.inputElement = el as HTMLInputElement)}
          required={true}
          type="text"
        ></input>
      </Host>
    );
  }
}

@Sukaato
Copy link
Contributor

Sukaato commented Nov 5, 2024

@dpellier I think your problem comes from understanding the stencil life cycle.

In the reset method, you try to reset the component value, but when the value property is updated, it must go through the onValueChange watch process, but this method only has access to the current rendered input, not the updated one, so it's normal that your validity status is still valid.

Your validity status will not be invalidated until after the render.

To reset the current input value, simply proceed as follows:

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

@Component({
  formAssociated: true,
  shadow: true,
  tag: 'my-component',
})
export class MyComponent {
  private inputElement?: HTMLInputElement;

  @AttachInternals() private internals!: ElementInternals;

  @Prop({ mutable: true, reflect: true }) public value: string = null;

  @Method()
  reset() {
    if (this.inputElement) {
      this.inputElement.value = null;
    }
    this.value = null;
  }

  @Watch('value')
  onValueChange(): void {
    console.log('onWatch, isValid?', this.inputElement.validity.valid);
  }

  private onInput(): void {
    this.value = this.inputElement?.value ?? null;
  }

  render() {
    return (
      <Host>
        <input
          onInput={ (): void => this.onInput() }
          ref={ (el): HTMLInputElement => this.inputElement = el as HTMLInputElement }
          required={ true }
          type="text"
          value={ this.value || '' }>
        </input>
      </Host>
    );
  }
}

And that will solve your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

No branches or pull requests

4 participants