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

[MDC Select] Dynamically loaded menu item values can't be set programmatically #5646

Closed
jasonmauss opened this issue Feb 24, 2020 · 5 comments · Fixed by #5870
Closed

[MDC Select] Dynamically loaded menu item values can't be set programmatically #5646

jasonmauss opened this issue Feb 24, 2020 · 5 comments · Fixed by #5870
Assignees

Comments

@jasonmauss
Copy link

jasonmauss commented Feb 24, 2020

Using MDC 5.0.0 JS lib. Created a fiddle of what I'm seeing here: https://jsfiddle.net/jamauss/bdfcanh9/50/

It seems like if all menu items (<li> elements) are present at the time the MDC Select is created, setting the .value property works just fine.

However, if menu items are added afterwards (e.g. json data loaded via ajax call inserting menu items into the DOM), setting the .value property will not cause the items to be set in the list, despite the MDCSelect:change event firing, which seems to indicate the component knows the value is being set. What is interesting is if I set the .selectedIndex property instead, then it will set the item in the list correctly (even if loaded dynamically), but not if I set the .value property. Again, this seems to only apply to menu items loaded/appended after the MDCSelect is created.

Steps to reproduce

see https://jsfiddle.net/jamauss/bdfcanh9/50/

Actual behavior

Item is not being set in the list / no change occurring for what .value is being set to

Expected behavior

Have item in the list get set when the .value property is set

Software Version(s)
MDC Web 5.0.0
Browser Chrome 78
Operating System Win 10

Additional context

This occured after upgrading from a early version of the MDC JS lib (0.48) to 5.0.0

@jasonmauss jasonmauss added the bug label Feb 24, 2020
@jasonmauss
Copy link
Author

Adding on to this - even in the case of attempting a workaround to where I do not instantiate the MDCSelect until all dynamically loaded DOM elements are present, the value property does not return the value that is initially programmatically selected in the list, it will only provide the selectedIndex value. the value property is always returning an empty string. It only seems to get the value property set if a user interacts with the list manually after initially setup.

@Pupix
Copy link

Pupix commented Feb 26, 2020

I have encountered this problem too. Even using selectedIndex creates problems if the select items change. When you change the index it will try to remove the old --selected class but the items doesn't exist anymore and it throws.

https://github.com/material-components/material-components-web/blob/master/packages/mdc-select/component.ts#L313

@allan-chen
Copy link
Collaborator

allan-chen commented Mar 13, 2020

Hi Jason,

After investigating a little:

  • Where first issue is coming from - the component memoizes the menu items here, and is unaware of any updates. One thought is to move that initialization logic to some sort of client-callable onOptionsUpdate method that forces a refetch of the items.
  • Second issue - seems like the select foundation doesn't correctly initialize its state to account for already selected element; it always assumes the initially selected item is UNSET (index -1), not '' (index 0), so the selected class on the old value ''(empty string) is never removed and we get 2 items selected at the same time, between which select.getValue() picks the first. Thinking about also putting this logic inside some onOptionsUpdate method.

@jasonmauss
Copy link
Author

@allan-chen thanks for the comment - I saw this got labeled backlog. Is this something your team will plan on working to fix in the near future? I will probably just rollback to a previous version until then.

@allan-chen
Copy link
Collaborator

allan-chen commented Apr 23, 2020

@GoldenSlam sorry for the late reply. As you might have seen an overhaul of the select component is beginning to roll in, to yield some much needed quality of life improvements. We'll get to this issue very soon as we polish this component up from the foundation layers and bridge some much needed feature gaps.

copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
…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
…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
…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
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants