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

checkValidity can be called on a form's elements before the element has been loaded which causes an error #1745

Closed
alianam opened this issue Nov 27, 2023 · 3 comments
Labels
bug Things that aren't working right in the library. waiting on element internals Issues to tackle once ElementInternals lands in all supported browsers.

Comments

@alianam
Copy link

alianam commented Nov 27, 2023

Describe the bug

After the change introduced in #1708 , form.checkValidity() can be called but can potentially raise an error if the function is called before the elements that FormControlController loops through are loaded since the element(s) can be null.

To Reproduce

  1. Set up a form with a form control from Shoelace in it, in our case the Sl-Select component
  2. Call form.checkValidity() immediately after appending the Shoelace form control to the document so that the validity check is called before the element finishes loading
  3. In the console, you will see the error "Uncaught TypeError: Cannot read properties of null (reading 'checkValidity')"

Demo

Here is a quick demo repo https://github.com/alianam/shoelace-bug/tree/main which includes simple setup instructions to clone the repo, install the dependencies, and then run the server. Once you open the page locally, and open the console, you will immediately see the error mentioned in this bug report.

Screenshots

image

Browser / OS

  • OS: Mac
  • Browser: Chrome, Firefox or Safari all show the same error
  • Browser version: N/A as this error can be reproduced across browser versions and is not version specific

Additional information

As a temporary workaround, I added an async function that finds and loops over all Shoelace elements in the document, then it calls updateComplete on each of them (inspired by your docs here - thanks!), and then I await Promise.allSettled() to ensure all of them have completed rendering. Only then do I proceed to call checkValidity and am able to avoid the error reported in this bug.

While this works, it seems like this check should exist in the Shoelace code and not be a requirement for those using the form controls, which is why I've created this bug report

@alianam alianam added the bug Things that aren't working right in the library. label Nov 27, 2023
@KonnorRogers
Copy link
Collaborator

Thanks for the report! Appreciate it, definitely a bug. Load order problems are always fun! I'll see if I can make a regression test for this and get a fix in!

@KonnorRogers
Copy link
Collaborator

KonnorRogers commented Nov 30, 2023

@alianam so the bug is a little more complex.

So for example <sl-select> does something like this:

  checkValidity() {
    return this.valueInput.checkValidity();
  }

Where this.valueInput is rendered in the shadowRoot.

@claviska there's a couple ways we could handle this.

 async checkValidity () {
   await this.updateComplete
   return this.valueInput.checkValidity();
  }

This kinda breaks the contract though because checkValidity is a synchronous function.

Of course there's the other option of moving to formAssociated custom elements. I guess the other option is to do the "virtualized" temporary element kinda like we do with valueAsDate. Not 100% how we want to approach this.

@claviska claviska added the waiting on element internals Issues to tackle once ElementInternals lands in all supported browsers. label Dec 11, 2023
@claviska
Copy link
Member

I don't see a way to fix this properly without implementing ElementInternals. I've tagged this issue so we can revisit it once we get that far. For now, I'm closing it to get it off the backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library. waiting on element internals Issues to tackle once ElementInternals lands in all supported browsers.
Projects
None yet
Development

No branches or pull requests

3 participants