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

[listbox] dynamic add or remove options #1070

Closed
MathieuPuech opened this issue Oct 30, 2020 · 8 comments
Closed

[listbox] dynamic add or remove options #1070

MathieuPuech opened this issue Oct 30, 2020 · 8 comments
Labels
docs Related to documentation

Comments

@MathieuPuech
Copy link
Collaborator

MathieuPuech commented Oct 30, 2020

Expected behavior

When using listbox and adding/removing options, it fail removing the option

Actual Behavior

https://webcomponents.dev/edit/Qii8XlpIwKaK5Am3FnYC

Additional context

@lion/listbox 0.3.1

@MathieuPuech
Copy link
Collaborator Author

It's working if you use lion-options:

      <lion-listbox>
        <lion-options slot="input">
          ${this.options.map(
            (option) =>
              html`
                <lion-option .choiceValue=${option}>${option}</lion-option>
              `
          )}
        </lion-options>
      </lion-listbox>

@tlouisse
Copy link
Member

tlouisse commented Nov 2, 2020

Yeah, I noticed as well that this api doesn't support all edge cases.

It's sugar on top of wrapping it in lion-options. It moves the nodes it find in the regular slot over to there ('there' is lion-options, which is the element with [role=listbox] that should be in light dom for a11y.

Since the individual lion-options are moved, lit-html asks for a parentNode to rerender the array.
This parentNode. What happens:

  • first <lion-option> is moved from regular slot to <lion-options role=listbox>
  • next time it tries to empty everything inside regular slot (including comment nodes) it asks first <lion-option> for parentNode. This is now the wrong one

This is solvable by a hack (for instance by setting <lion-option>.parentNode to this inside update()) but I don't think we should go there.

So the 'sugared api' works when options are declared declaratively or via a loop wich content is never updated.
For now I think we should just keep using <lion-options slot="input"> and add this in our documentation.

Unless someone sees another posible fix of course

@tlouisse
Copy link
Member

Update on this: the lit repeat directive might solve this issue.

You will run into other problems then, since FormRegistrarMixin assumes the order of registration corresponds with dom order (or, more precisely, parts of the code expect .formElements order to correspond with dom order).

When we make sure inside FormRegistrarMixin that dom order is guarantueed inside formElements, the issue might be solved.

We should keep in mind that (since many elements register synchronously), we do the dom reordering async/batched for perf.

@tlouisse
Copy link
Member

@MathieuPuech This should work now thanks to a fix in the FormRegistrarMixin by @daKmoR 💪

        ${repeat(
          this.options,
          option => option.id,
          option => html`
            <lion-option
              .choiceValue=${option.value}
            ></lion-option>
          `,
        )}

Closing this one, feel free to reopen when needed...

@MathieuPuech
Copy link
Collaborator Author

does it means that dynamic options can only be done with lit?

@MathieuPuech MathieuPuech reopened this Nov 27, 2020
@tlouisse
Copy link
Member

tlouisse commented Dec 1, 2020

Until we have the AOM, yes.
But it's good to mention that (plus the alternative api) in the docs 👍

@tlouisse tlouisse added the docs Related to documentation label Dec 1, 2020
@daKmoR
Copy link
Collaborator

daKmoR commented Dec 1, 2020

does it means that dynamic options can only be done with lit?

what do you mean by that? 🤔
if you use document.createElement directly then it should work fine as well...

the lit directive repeat is primarily there so that lit does not reuse existing dom nodes with external existing properties

@MathieuPuech
Copy link
Collaborator Author

Oh ok, so it’s an issue with Lit html rendering which is too much reusing the nodes. It may not happen with other rendering engines if that’s the case. I will see if there is any issues with other engine and reopen the issue if needed.

Thanks for the clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Related to documentation
Projects
None yet
Development

No branches or pull requests

3 participants