Skip to content

fix(forms): ngModel select [multiple=false] should return a value #38719

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.

Sign up for GitHub

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
Closed
wants to merge 1 commit into from
Closed

fix(forms): ngModel select [multiple=false] should return a value #38719

wants to merge 1 commit into from

Conversation

vytautas-petrikas
Copy link
Contributor

@vytautas-petrikas vytautas-petrikas commented Sep 5, 2020 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

When ngModel is added to a select element with multiple attribute value set to false SelectMultipleControlValueAccessor
is used instead of SelectControlValueAccessor, therefore an array is returned instead of a value.

Issue Number: #12585

What is the new behavior?

When ngModel is added to a select element with multiple attribute value set to false SelectControlValueAccessor, therefore a value is returned.

Does this PR introduce a breaking change?

  • Yes
  • No

There might be applications/libraries that already depend on this behavior as this issue was reported for version 2.1.2. These changes should be reviewed by project maintainers manually.

Other information

I hoped to fix this issue by modifying selectors for previously mentioned directives, however I was surprised to find out that multiple attribute selector is not working with values.

Any guidance on how to proceed with this PR would be appreciated.

@pullapprove pullapprove bot requested a review from AndrewKushnir September 5, 2020 11:51
@AndrewKushnir
Copy link
Contributor

Hi @vytautas-petrikas, thanks for creating this PR.

As you mentioned, Angular doesn't match dynamic bindings with selectors that have an attribute and its value specified, i.e. in your example with the [multiple=false] selector:

  • <select multiple="false"> will be matched
  • <select [multiple]="false"> will NOT be matched
  • <select [multiple]="someBooleanField"> will NOT be matched

Here is a quick Stackblitz demo page I put together to illustrate this. As it's mentioned in #11716, this is a framework limitation.

I believe the 2nd and 3rd cases are most common/desirable (and they are not covered by this PR) and in the first case, the multiple attribute can just be removed. Given that and the fact that this is a framework limitation, I don't think we can land this change.

One possible improvement to developer experience might be adding a logic to the SelectMultipleControlValueAccessor class to output a message to a console in case multiple input is set to false-y value (that'd cover all 3 cases listed above).

Thank you.

@ngbot ngbot bot added this to the needsTriage milestone Sep 8, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

Assignees
No one assigned
Labels
area: forms cla: yes forms: ngModel state: blocked
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.
5 participants
@vytautas-petrikas @AndrewKushnir @sonukapoor @atscott @googlebot