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

fix(list): selection list incorrectly preselecting options without a value in some cases #17840

Open
wants to merge 1 commit into
base: master
from

Conversation

@crisbeto
Copy link
Member

crisbeto commented Nov 29, 2019

The way we keep track of the value of an entire selection list is by taking all of the selected options and putting their values in an array. When the set of options is swapped out we look through that array to determine whether an option should be preselected. This breaks down if an option doesn't have a value, because we end up with a value array looking like [undefined] which will cause the option to be preselected incorrectly. These changes work around the issue by not adding options without a value to the value array.

Fixes #17839.

src/material/list/selection-list.spec.ts Outdated Show resolved Hide resolved
return this.options.filter(option => option.selected).map(option => option.value);
return this.options.filter(option => {
return option.selected && option.value !== undefined;
}).map(option => option.value);

This comment has been minimized.

Copy link
@devversion

devversion Dec 2, 2019

Member

doesn't this cause semantic changes to the form control value of the MatSelectionList? i.e. previously the form control value could be [undefined].

I see the argument of saying that undefined can never be a value due limitations of how selection-list is implemented. Though we should capture the intended behavior in a test? It could technically break existing apps too.

This comment has been minimized.

Copy link
@crisbeto

crisbeto Dec 2, 2019

Author Member

You're right, but I'm not sure whether there's a cleaner alternative. We could also have a property that keeps track of whether the value setter was ever invoked and gate it based on that, but it feels very hacky.

This comment has been minimized.

Copy link
@devversion

devversion Dec 2, 2019

Member

Yeah not sure either if there is a cleaner way. I have the feeling that there should be no special handling for values like undefined. i.e. if multiple options have the same value, it's expected that all matching ones are selected. Though it feels contradictory to the fact that individual options have a selected input.

This comment has been minimized.

Copy link
@crisbeto

crisbeto Dec 2, 2019

Author Member

Agreed, but we wrote ourselves into a corner with the selected input.

This comment has been minimized.

Copy link
@devversion

devversion Dec 2, 2019

Member

Yeah 😄 I'm not sure how to proceed here. maybe discuss in the next team meeting?

This comment has been minimized.

Copy link
@crisbeto

crisbeto Dec 2, 2019

Author Member

Sure, I'll add it to the agenda.

…value in some cases

The way we keep track of the value of an entire selection list is by taking all of the `selected` options and putting their values in an array. When the set of options is swapped out we look through that array to determine whether an option should be preselected. This breaks down if an option doesn't have a value, because we end up with a value array looking like `[undefined]` which will cause the option to be preselected incorrectly. These changes work around the issue by not adding options without a value to the value array.

Fixes #17839.
@crisbeto crisbeto force-pushed the crisbeto:17839/selection-list-no-value-selection branch from 8a2fe97 to 01ddc27 Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.