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

select: Should <selectedoption> update when selecting the already-selected option #1119

Open
jakearchibald opened this issue Oct 25, 2024 · 5 comments
Labels
select These are issues that relate to the select component

Comments

@jakearchibald
Copy link
Contributor

This is following up on the resolution of #825, where the selected <option>'s content is only cloned when the selection is changed, and not when the content of the selected <option> changes.

Which of the following cases should result in the triggering of the clone operation, even though the result is the same option being selected:

  • The user opens the picker and closes it without making a selection
  • The user opens the picker and clicks on the <option> that is already selected
  • select.value = select.value
  • select.selectedIndex = select.selectedIndex
  • select.selectedOptions[0].replaceWith(select.selectedOptions[0])
  • select.append(document.createElement('optgroup'))
  • form.reset()

My opinions:

  • The user opens the picker and closes it without making a selection

No change. Nothing happened.

  • The user opens the picker and clicks on the <option> that is already selected

Now I've thought about it, I don't think this should re-clone. The spec seems a little loose on this but browser don't fire change or input when selecting the already selected option, so internally this seems like a no-op.

  • select.value = select.value
  • select.selectedIndex = select.selectedIndex

I think these should trigger the cloning operation, since it's an explicit set of the value. Although my feelings on this aren't strong.

  • select.selectedOptions[0].replaceWith(select.selectedOptions[0])

This should clear the <selectedoption> when the selected <option> is removed, then trigger the cloning operation when the <option> is reinserted.

  • select.append(document.createElement('optgroup'))

This should not trigger the cloning operation. Nor should adding an <option> unless it become the new selected option.

  • form.reset()

This should trigger the cloning operation only if the selected option changes to another option.

@sorvell
Copy link

sorvell commented Oct 25, 2024

Another case to consider: the select dynamically becomes rendered as a custom select via, say, adding a class to it that sets appearance.

@sorvell
Copy link

sorvell commented Oct 25, 2024

My opinions:

... seem reasonable.

@josepharhar
Copy link
Collaborator

  • The user opens the picker and clicks on the <option> that is already selected

Now I've thought about it, I don't think this should re-clone. The spec seems a little loose on this but browser don't fire change or input when selecting the already selected option, so internally this seems like a no-op.

  • select.value = select.value
  • select.selectedIndex = select.selectedIndex

I think these should trigger the cloning operation, since it's an explicit set of the value. Although my feelings on this aren't strong.

Making a distinction between these two feels a bit inconsistent to me, but making re-assigning the value from script trigger cloning means that we don't need a separate method to do so.

It looks like the spec already says that assigning to select.value resets a bunch of stuff, so no special casing would be needed for this hopefully:

On setting, the value attribute must set the selectedness of all the option elements in the list of options to false, and then the first option element in the list of options, in tree order, whose value is equal to the given new value, if any, must have its selectedness set to true and its dirtiness set to true.

@josepharhar josepharhar added select These are issues that relate to the select component agenda+ Use this label if you'd like the topic to be added to the meeting agenda labels Oct 28, 2024
@josepharhar
Copy link
Collaborator

Proposed resolution: adopt Jake's suggestions: Clone when re-assigning to select.value, don't clone when user selects the same option again

@css-meeting-bot
Copy link

The Open UI Community Group just discussed select: Should `<selectedoption>` update when selecting the already-selected option, and agreed to the following:

  • RESOLVED: adopt Jake's suggestions: Clone when re-assigning to select.value, don't clone when user selects the same option again
The full IRC log of that discussion <gregwhitworth> jarhar: Jake made this issue and listed a bunch of scenarios that may or may not clone into the selectedcontent element
<gregwhitworth> jarhar: I agree with him that we want to adopt them. The gist of it is when the user interacts with the select where an option is not selected it will not trigger the clone
<gregwhitworth> jarhar: but if it tiggers a .value change then it will fire the re-clone
<masonf> s/tiggers/triggers
<gregwhitworth> jarhar: if we trigger a clone on re-assign on .value we don't need a new method to trigger the clone. Doesn't mean we don't add a method later but we don't NEED one now
<masonf> q+ sorvell
<masonf> q+
<gregwhitworth> jarhar: that's why it makes sense to me and reading the HTML spec we should get this for free
<masonf> ack sorvell
<gregwhitworth> sorvell: the platform generally does what is annoying but if you re-assign an attribute to the same element and an element is being observed then it will be observed. This would make it consistent
<gregwhitworth> sorvell: I agree with Jake's proposals but I added one, if you change custom to not custom then it should re-clone in that case
<gregwhitworth> masonf: we should list it explicitly anyways
<scott> q+
<gregwhitworth> masonf: explicitly it is taking Jake's suggestions and some of them don't trigger a clone
<gregwhitworth> ack masonf
<jarhar> q+
<gregwhitworth> masonf: plus Steve's proposal
<gregwhitworth> ack scott
<gregwhitworth> scott: only a question about when a clone would happen is if someone modifies the chosen option and then re-selects. I'm seeing headshake no but what would be populated in there isn't being re-cloned if it has been changed
<gregwhitworth> masonf: let's say that you have a RAF on the option that is changing then you would be re-cloning over and over
<sarah> q+
<gregwhitworth> scott: I agree overall, just thinking through what the dev is doing
<gregwhitworth> q+
<gregwhitworth> masonf: the dev should blow away the options if they want to change them
<gregwhitworth> sorvell: I wish Jake were here, the case that would possibly be problematic is if the Option is updated. For example the first option is saying Loading and then that changes and you update them it's still going to say loading and the VDOM will change the content
<gregwhitworth> sorvell: The feedback was that you need to know what you're doing
<gregwhitworth> jarhar: I do not think the clone should trigger when the appearance property changes
<gregwhitworth> jarhar: the cloning is going to do its thing whatever the CSS is
<gregwhitworth> jarhar: if you're in appearance: auto we're still going to do the cloning regardless
<sarah> eh, I was just going to go on the queue to say essentially what masonf and Steve already said, and also that if a dev is modifying options in-place for e.g. virtualization, they almost certainly are also modifying the value/selection programmatically, otherwise things would probably break anyway (and display the wrong selection styles on the option
<sarah> as well)
<sarah> q-
<masonf> ack jarhar
<gregwhitworth> jarhar: we have a method in the element and if we recalculate the styles that's where I would put this it may result in odd re-flow issues and be complicated to spec
<gregwhitworth> masonf: selectedcontent always gets cloned
<jarhar> q?
<gregwhitworth> sorvell: It's confusing to me but to your point that it's always there that means it can be unrelated thank you for the explanation
<masonf> gregwhitworth: steve covered "what updating the option" means. E.g. I have 5 options, I wrote JS to change contents of existing options, what happens?
<masonf> gregwhitworth: Is that going to be an issue? I'll be changing the values, even in the vdom case, it'll work right? Unless they changed the content of the options, with the same values.
<bkardell_> q+
<jarhar> q?
<jarhar> q+
<gregwhitworth> ack gregwhitworth
<sorvell> q+
<jarhar> q-
<gregwhitworth> masonf: modification of the select vs the option and you choose a different option will result in a re-clone but monkeying with the options will not re-clone and in order to do that you should remove the option and put a whole new option element in its place
<gregwhitworth> ack bkardell_
<masonf> ack bkardell
<gregwhitworth> bkardell_: I'm also confused, I don't think that Loading.... thing is comperable
<gregwhitworth> bkardell_: you wouldn't replace that one and produce the new one. You would loop through them and produce new options
<gregwhitworth> steve: the point of it is that in the VDOM it will change the diff between the work
<gregwhitworth> bkardell_: people could _do_ a lot of things but I don't see anyone doing that
<gregwhitworth> sorvell: we'll make sure to have them key their elements to make sure and we'll need to contend with the fact that this isn't a complete corner case
<gregwhitworth> sarah: they all kind of work that and the laziest devs default will be to update in place
<gregwhitworth> sarah: yeah, kind of
<gregwhitworth> masonf: it will be visually obvious and they'll add the keys on there like they should to
<gregwhitworth> q?
<masonf> ack sorvell
<gregwhitworth> ack sorvell
<gregwhitworth> sorvell: I'm proxying Jake and I think in a poor manner we're making an intentional tradeoff
<gregwhitworth> sorvell: things can get out of whack but practically speaking it's a valid tradeoff and informing devs what to do
<masonf> Proposed resolution: adopt Jake's suggestions: Clone when re-assigning to select.value, don't clone when user selects the same option again
<jarhar> Proposed resolution: adopt Jake's suggestions: Clone when re-assigning to select.value, don't clone when user selects the same option again
<gregwhitworth> masonf: I think that was great in that there are downsides on either side but it's the other side that has more dragons and this is the safer side
<sarah> +1
<masonf> +1
<jarhar> RESOLVED: adopt Jake's suggestions: Clone when re-assigning to select.value, don't clone when user selects the same option again
<sorvell> +1

@gregwhitworth gregwhitworth removed the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
select These are issues that relate to the select component
Projects
None yet
Development

No branches or pull requests

5 participants