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

Jump to bottom

Initializing a Select with value immediately calls notifyChange handler #5783

Closed
jamesmfriedman opened this issue Apr 5, 2020 · 0 comments · Fixed by #5870
Closed

Initializing a Select with value immediately calls notifyChange handler #5783

jamesmfriedman opened this issue Apr 5, 2020 · 0 comments · Fixed by #5870
Assignees
@allan-chen
Labels
backlog

Comments

@jamesmfriedman
Copy link
Contributor

This is a relatively confusing issue, I'll do my best to explain it.

  • If you render a native HTML select on a page, it doesn't fire a change event. This is what people generally expect
  • I work mostly in React, selects and other form controls also do not fire a change event on init. Again, this is expected
  • The MDC select fires it's notifyChange event on init due to a sequence of calls that start with setValue. This is counter to general expectations of form elements.
  • In RMWC, it caused enough issues to where I had to implement a workaround to disable emitting the change event on init since it would fire form handlers and cause immediate validation, submission, etc.
  • Had this issue come up because now users have to add a delay in unit testing since trying to manually fire an onChange event is artificially disabled <Select /> onChange handler does not trigger in jsdom rmwc/rmwc#596

material-components-web/packages/mdc-select/foundation.ts

Line 380 in 6a40ef2

this.setValue(value);

@allan-chen allan-chen self-assigned this Apr 6, 2020
@allan-chen allan-chen mentioned this issue Apr 28, 2020
Closed
@copybara-service copybara-service bot mentioned this issue Apr 29, 2020
Merged
copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
@allan-chen
fix(select): Fix redundant calculations & allow resyncing foundation …
ba3760c 
…to options

- Method `layout` and private method `updateLabel` have been collapsed into one `layout` method. The reasoning is partially to reduce redundancy, but also since there's no reason they should be updated separately. The old layout method may have simply been broken since it doesn't update the label, just the notch, but lack of usage meant this problem didn't surface.
- `layoutOptions` method can now be called by the client whenever the underlying list elements change (i.e. dynamic server fetch), to ensure that the foundation's state is sync'ed up. When typeahead lands in list foundation, this will also allow an optional call to list's layout method to reinitialize typeahead cache.
- The empty "" option is now properly accounted for as an option. Previously it was ignored, so if you initialized the select with "" having the selected class, it wouldn't be sync'ed to the internal state. This meant that selecting a different option via select.value="blah" wouldn't remove the "selected" classes and attributes on the "" option, meaning it would still act like "" is selected even though selected text shows up as "blah".
- `foundation.init` is no longer called twice: once by base component.ts and once by select component.ts
- `foundation.init` no longer calls `notchOutline` 3 (???) times (closes #5686)
- `foundation.init` no longer emits change event (closes #5783).

PiperOrigin-RevId: 308892334
copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
@allan-chen
fix(select): Fix redundant calculations & allow resyncing foundation …
9a460ef 
…to options

- Method `layout` and private method `updateLabel` have been collapsed into one `layout` method. The reasoning is partially to reduce redundancy, but also since there's no reason they should be updated separately. The old layout method may have simply been broken since it doesn't update the label, just the notch, but lack of usage meant this problem didn't surface.
- `layoutOptions` method can now be called by the client whenever the underlying list elements change (i.e. dynamic server fetch), to ensure that the foundation's state is sync'ed up. When typeahead lands in list foundation, this will also allow an optional call to list's layout method to reinitialize typeahead cache.
- The empty "" option is now properly accounted for as an option. Previously it was ignored, so if you initialized the select with "" having the selected class, it wouldn't be sync'ed to the internal state. This meant that selecting a different option via select.value="blah" wouldn't remove the "selected" classes and attributes on the "" option, meaning it would still act like "" is selected even though selected text shows up as "blah".
- `foundation.init` is no longer called twice: once by base component.ts and once by select component.ts
- `foundation.init` no longer calls `notchOutline` 3 (???) times (closes #5686)
- `foundation.init` no longer emits change event (closes #5783).

PiperOrigin-RevId: 308892334
copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
@allan-chen
fix(select): Fix redundant calculations & allow resyncing foundation …
f5b0d4b 
…to options

- Method `layout` and private method `updateLabel` have been collapsed into one `layout` method. The reasoning is partially to reduce redundancy, but also since there's no reason they should be updated separately. The old layout method may have simply been broken since it doesn't update the label, just the notch, but lack of usage meant this problem didn't surface.
- `layoutOptions` method can now be called by the client whenever the underlying list elements change (i.e. dynamic server fetch), to ensure that the foundation's state is sync'ed up. When typeahead lands in list foundation, this will also allow an optional call to list's layout method to reinitialize typeahead cache.
- The empty "" option is now properly accounted for as an option. Previously it was ignored, so if you initialized the select with "" having the selected class, it wouldn't be sync'ed to the internal state. This meant that selecting a different option via select.value="blah" wouldn't remove the "selected" classes and attributes on the "" option, meaning it would still act like "" is selected even though selected text shows up as "blah". (closes #5646)
- `foundation.init` is no longer called twice: once by base component.ts and once by select component.ts
- `foundation.init` no longer calls `notchOutline` 3 (???) times (closes #5686)
- `foundation.init` no longer emits change event (closes #5783).

PiperOrigin-RevId: 308892334
copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
@allan-chen
fix(select): Fix redundant calculations & allow resyncing foundation …
0ead217 
…to options

- Method `layout` and private method `updateLabel` have been collapsed into one `layout` method. The reasoning is partially to reduce redundancy, but also since there's no reason they should be updated separately. The old layout method may have simply been broken since it doesn't update the label, just the notch, but lack of usage meant this problem didn't surface.
- `layoutOptions` method can now be called by the client whenever the underlying list elements change (i.e. dynamic server fetch), to ensure that the foundation's state is sync'ed up. When typeahead lands in list foundation, this will also allow an optional call to list's layout method to reinitialize typeahead cache. (closes #5646)
- The empty "" option is now properly accounted for as an option. Previously it was ignored, so if you initialized the select with "" having the selected class, it wouldn't be sync'ed to the internal state. This meant that selecting a different option via select.value="blah" wouldn't remove the "selected" classes and attributes on the "" option, meaning it would still act like "" is selected even though selected text shows up as "blah". (closes #5646)
- `foundation.init` is no longer called twice: once by base component.ts and once by select component.ts
- `foundation.init` no longer calls `notchOutline` 3 (???) times (closes #5686)
- `foundation.init` no longer emits change event (closes #5783).

PiperOrigin-RevId: 308892334
copybara-service bot pushed a commit that referenced this issue Apr 30, 2020
@allan-chen
fix(select): Fix redundant calculations & allow resyncing foundation …
de8eaab 
…to options

- Method `layout` and private method `updateLabel` have been collapsed into one `layout` method. The reasoning is partially to reduce redundancy, but also since there's no reason they should be updated separately. The old layout method may have simply been broken since it doesn't update the label, just the notch, but lack of usage meant this problem didn't surface.
- `layoutOptions` method can now be called by the client whenever the underlying list elements change (i.e. dynamic server fetch), to ensure that the foundation's state is sync'ed up. When typeahead lands in list foundation, this will also allow an optional call to list's layout method to reinitialize typeahead cache. (closes #5646)
- The empty "" option is now properly accounted for as an option. Previously it was ignored, so if you initialized the select with "" having the selected class, it wouldn't be sync'ed to the internal state. This meant that selecting a different option via select.value="blah" wouldn't remove the "selected" classes and attributes on the "" option, meaning it would still act like "" is selected even though selected text shows up as "blah". (closes #5646)
- `foundation.init` is no longer called twice: once by base component.ts and once by select component.ts
- `foundation.init` no longer calls `notchOutline` 3 (???) times (closes #5686)
- `foundation.init` no longer emits change event (closes #5783).

PiperOrigin-RevId: 308892334
copybara-service bot pushed a commit that referenced this issue Apr 30, 2020
@allan-chen
fix(select): Fix redundant calculations & allow resyncing foundation …
c801c4b 
…to options

- Method `layout` and private method `updateLabel` have been collapsed into one `layout` method. The reasoning is partially to reduce redundancy, but also since there's no reason they should be updated separately. The old layout method may have simply been broken since it doesn't update the label, just the notch, but lack of usage meant this problem didn't surface.
- `layoutOptions` method can now be called by the client whenever the underlying list elements change (i.e. dynamic server fetch), to ensure that the foundation's state is sync'ed up. When typeahead lands in list foundation, this will also allow an optional call to list's layout method to reinitialize typeahead cache. (closes #5646)
- The empty "" option is now properly accounted for as an option. Previously it was ignored, so if you initialized the select with "" having the selected class, it wouldn't be sync'ed to the internal state. This meant that selecting a different option via select.value="blah" wouldn't remove the "selected" classes and attributes on the "" option, meaning it would still act like "" is selected even though selected text shows up as "blah". (closes #5646)
- `foundation.init` is no longer called twice: once by base component.ts and once by select component.ts
- `foundation.init` no longer calls `notchOutline` 3 (???) times (closes #5686)
- `foundation.init` no longer emits change event (closes #5783).

PiperOrigin-RevId: 308892334
@allan-chen allan-chen mentioned this issue Jun 29, 2020
Closed
@adrianschmidt adrianschmidt mentioned this issue Dec 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@allan-chen allan-chen

Labels
backlog
Projects
None yet
Milestone
No milestone
3 participants
@jamesmfriedman @patrickrodee @allan-chen