fix(forms): not picking up disabled state if group is swapped out and disabled by crisbeto · Pull Request #43499 · angular/angular

crisbeto · 2021-09-18T09:49:00Z

Fixes a long-standing issue where swapping out the FormGroup and calling disable immediately afterwards doesn't actually disable the ControlValueAccessor.

Fixes #22556.

AndrewKushnir · 2021-09-21T02:02:16Z

Presubmit (internal-only link) to gather some data as requested in #43499 (comment).
Also, I've started a global presubmit (internal-only link).

AndrewKushnir

@crisbeto I've just left one comment about another directive that would need an update. Please have a look when you get a chance.

I've also got some news after running a global presubmit. You were correct, the change is not as break-y as #35311 and there was just 1 failing target that would require investigation/cleanup. We'll provide more info after initial investigation.

// cc @dylhunn

dylhunn · 2021-09-21T19:16:35Z

I'll have a look at this later this afternoon, sorry I didn't see the mention yesterday!

… disabled Fixes a long-standing issue where swapping out the `FormGroup` and calling `disable` immediately afterwards doesn't actually disable the `ControlValueAccessor`. Fixes angular#22556.

…out and disabled

dylhunn · 2021-09-22T16:48:23Z

@AndrewKushnir Do you have a link to the failing target from the TGP?

AndrewKushnir · 2021-09-22T17:09:28Z

Do you have a link to the failing target from the TGP?

@dylhunn yes, see #43499 (comment).

dylhunn · 2021-09-24T20:03:47Z

I had some difficulty understanding why this breaks the failing target, unfortunately. I will allocate some more time to try to figure it out on Monday. Sorry I don't have an answer yet.

dylhunn · 2021-11-30T21:57:45Z

@crisbeto Since you're a Googler now, do you want to take over the presubmit on this? Sorry I let it drag for so long.

crisbeto · 2021-11-30T21:58:41Z

Sure, I ran a presubmit for it some time ago, but I haven't had the time to investigate it either.

wawyed · 2022-03-09T17:27:01Z

Could we have some progress on this?

dylhunn · 2022-03-25T03:07:01Z

This is sort of a half-fix, as @crisbeto described: it fixes the issue if an enabled control is swapped for a disabled one, but not vice versa.

However, this "half" of the fix by far the more impactful half, and it should be a lot less breaking. I think we should merge this for v14 if the breakage isn't too bad. I'll check back tomorrow and assess the fresh TGP.

dylhunn · 2022-03-25T17:11:19Z

I ran a fresh TGP, and there is only one failing target in all of google3. It looks like it should be easy to fix, and then I think we can just merge this. I will have a look later today.

dylhunn

reviewed-for: fw-forms

dylhunn · 2022-03-25T21:46:27Z

Merge is blocked on cl/437323972

dylhunn · 2022-03-28T16:11:48Z

The g3 fix has been merged.

dylhunn · 2022-03-28T16:26:21Z

This PR was merged into the repository by commit b36dec6.

… disabled (angular#43499) Fixes a long-standing issue where swapping out the `FormGroup` and calling `disable` immediately afterwards doesn't actually disable the `ControlValueAccessor`. Fixes angular#22556. PR Close angular#43499

angular-automatic-lock-bot · 2022-04-28T16:08:10Z

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.}

google-cla Bot added the cla: yes label Sep 18, 2021

crisbeto force-pushed the 22556/forms-disabled-state branch 3 times, most recently from 28ed1e0 to f07d48f Compare September 18, 2021 11:05

crisbeto commented Sep 18, 2021

View reviewed changes

Comment thread packages/forms/src/directives/shared.ts Resolved

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms target: patch This PR is targeted for the next patch release labels Sep 18, 2021

ngbot Bot modified the milestone: Backlog Sep 18, 2021

crisbeto marked this pull request as ready for review September 18, 2021 11:28

ngbot Bot added this to the Backlog milestone Sep 18, 2021

pullapprove Bot requested a review from AndrewKushnir September 18, 2021 11:29

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Sep 21, 2021

AndrewKushnir reviewed Sep 21, 2021

View reviewed changes

Comment thread packages/forms/src/directives/reactive_directives/form_control_directive.ts Resolved

AndrewKushnir requested a review from dylhunn September 21, 2021 19:10

AndrewKushnir assigned dylhunn Sep 21, 2021

crisbeto added 2 commits September 22, 2021 06:37

fix(forms): not picking up disabled state if group is swapped out and…

97df03c

… disabled Fixes a long-standing issue where swapping out the `FormGroup` and calling `disable` immediately afterwards doesn't actually disable the `ControlValueAccessor`. Fixes angular#22556.

fixup! fix(forms): not picking up disabled state if group is swapped …

9bf293e

…out and disabled

crisbeto force-pushed the 22556/forms-disabled-state branch from f07d48f to 9bf293e Compare September 22, 2021 04:44

AndrewKushnir assigned crisbeto and unassigned dylhunn Oct 15, 2021

AndrewKushnir added state: blocked and removed action: presubmit The PR is in need of a google3 presubmit labels Oct 29, 2021

dylhunn removed their request for review December 1, 2021 22:53

dylhunn added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 1, 2021

destus90 mentioned this pull request Dec 14, 2021

Creating a new FormGroup instance for the same <form> Element does not initialize disabled states correctly #44478

Closed

dylhunn added target: major This PR is targeted for the next major release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Mar 25, 2022

dylhunn self-requested a review March 25, 2022 20:47

dylhunn approved these changes Mar 25, 2022

View reviewed changes

dylhunn added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Mar 28, 2022

dylhunn closed this in b36dec6 Mar 28, 2022

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Apr 28, 2022

Conversation

crisbeto commented Sep 18, 2021

Uh oh!

Uh oh!

AndrewKushnir commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dylhunn commented Sep 21, 2021

Uh oh!

dylhunn commented Sep 22, 2021

Uh oh!

AndrewKushnir commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylhunn commented Sep 24, 2021

Uh oh!

dylhunn commented Nov 30, 2021

Uh oh!

crisbeto commented Nov 30, 2021

Uh oh!

wawyed commented Mar 9, 2022

Uh oh!

dylhunn commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylhunn commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylhunn left a comment

Choose a reason for hiding this comment

Uh oh!

dylhunn commented Mar 25, 2022

Uh oh!

dylhunn commented Mar 28, 2022

Uh oh!

dylhunn commented Mar 28, 2022

Uh oh!

angular-automatic-lock-bot Bot commented Apr 28, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AndrewKushnir commented Sep 21, 2021 •

edited

Loading

AndrewKushnir commented Sep 22, 2021 •

edited

Loading

dylhunn commented Mar 25, 2022 •

edited

Loading

dylhunn commented Mar 25, 2022 •

edited

Loading