Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(select): don't change form state when adding to a form #11491

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

cstephe
Copy link
Contributor

@cstephe cstephe commented Oct 18, 2018

  • Remove code which causes side effect on form elements
  • Add a delay to the initial render of a multiple select so that it doesn't set the element to dirty on initialization.

Fixes #11490

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #11490
The select/multiple has code it in which causes a side-effect on the containing form by explicitly setting the form state to pristine anytime one is added with a set model value. This errant code was introduced originally to try to fix an issue where during initialization the control thought it was being updated and would implicitly mark the form dirty. This is obviously a deal breaker when using this control in dynamic forms.

What is the new behavior?

Now the control initializes so that it does not think it is being updated and the "corrective" side-effect has been removed. Adding and removing these controls from a dynamic form no longer changes the form state.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Oct 18, 2018
@cstephe
Copy link
Contributor Author

cstephe commented Oct 18, 2018

I already have a CLA, any help on resolving that check point?

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Oct 18, 2018
@Splaktar Splaktar changed the title fix(select(multiple)): Remove side-effects to forms when adding md-s… fix(select): don't change form state when adding to a form Oct 25, 2018
@Splaktar Splaktar self-requested a review October 25, 2018 11:16
@Splaktar Splaktar self-assigned this Oct 25, 2018
@Splaktar Splaktar added this to the 1.1.12 milestone Oct 25, 2018
@Splaktar Splaktar added type: bug needs: review This PR is waiting on review from the team P3: important Important issues that really should be fixed when possible. labels Oct 25, 2018
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me at first glance. I had a few minor consistency and naming recommendations.

I was happy to see that this doesn't require major changes to any of the existing tests.

Thank you for the contribution!

Please make sure to amend your commit and force push it back up to your branch so that you can avoid needing to squash commits.

@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Oct 25, 2018
…lect elements

* Remove code which causes side effect on form elements
* Add a delay to the initial render of a multiple select so that it doesn't set the element to dirty on initialization.

Fixes angular#11490
@cstephe
Copy link
Contributor Author

cstephe commented Oct 26, 2018

@Splaktar I'm not sure if i'm supposed to resolve the conversations for the PR comments. I did one, then I thought, maybe thats for you to resolve.

Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll send this to presubmit testing and hope that it doesn't run into any tests that depend on the old/broken behavior.

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Oct 30, 2018
@jelbourn jelbourn merged commit 97e2d00 into angular:master Nov 6, 2018
@Splaktar Splaktar modified the milestones: 1.1.12, 1.1.11 Nov 7, 2018
marosoft pushed a commit to marosoft/material that referenced this pull request Nov 11, 2018
…lect elements (angular#11491)

* Remove code which causes side effect on form elements
* Add a delay to the initial render of a multiple select so that it doesn't set the element to dirty on initialization.

Fixes angular#11490
@Splaktar
Copy link
Contributor

This appears to have caused the following regression: #11571

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P3: important Important issues that really should be fixed when possible. pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select(multiple): adding to a form causes the form to be marked pristine
5 participants