fix(forms): Call setDisabledState in in reactive directives when appropriate by jermowery · Pull Request #35311 · angular/angular

jermowery · 2020-02-10T20:23:12Z

When a CVA is added a form and that CVA implements setDisabledState the
reactive form directives should call setDisabledState with true when the
FormControl is disabled and false when it is enabled. Currently,
setDisabledState is only called when the FormControl is disabled.

Fixes #35309

PR Checklist

Please check if your PR fulfills the following requirements:

[ x] The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
[ x] Tests for the changes have been added (for bug fixes / features)
[ x] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ x] 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?

setDisabledState when adding a control to a form is called with true with the FormControl is disabled and not called when the FormControl is enabled.

Issue Number: #35309

What is the new behavior?

setDisabledState is called when adding a control to a form with false when the FormControl is enabled and true with the FormControl is disabled.

Does this PR introduce a breaking change?

Yes
[x ] No

AndrewKushnir

@jermowery thanks for creating a PR! 👍 I left a couple nits, please have a look.

It looks like this PR also introduces behavior change for Forms, where setDisabledState is called in situations where it was not called before. Could you please try to run Global TAP with this change to see if there are targets that might be potentially affected?

Thank you.

jermowery · 2020-02-11T01:37:11Z

It looks like this PR also introduces behavior change for Forms, where setDisabledState is called in situations where it was not called before. Could you please try to run Global TAP with this change to see if there are targets that might be potentially affected?

There are 3 specs failing as a result of this change. 2 of which are in my own codebase. I will put together a CL that can be patched with the fixes for the Angular caretaker doing the sync

appropriate When a CVA is added a form and that CVA implements setDisabledState the reactive form directives should call setDisabledState with true when the FormControl is disabled and false when it is enabled. Currently, setDisabledState is only called when the FormControl is disabled. Fixes #35309 Fix based on comments

AndrewKushnir

Thanks for additional changes @jermowery!

There are 3 specs failing as a result of this change. 2 of which are in my own codebase. I will put
together a CL that can be patched with the fixes for the Angular caretaker doing the sync

Thanks for verifying this in g3 👍Given that there are some (minor) breakages, I'd like to ask @kara to have a look at this PR too, to make sure we assess the risks accordingly.

Thank you.

jermowery · 2020-02-11T02:33:35Z

I just took another look at my Global TAP and it looks like there are more changes than anticipated. I will discuss with you all tomorrow

sonukapoor · 2020-03-29T20:04:22Z

Hi @jermowery, I was working on a similar PR, but noticed that your PR already covers what I was going to change as well. Is there anything I can help with for this PR?

jermowery · 2020-03-30T13:47:33Z

Unfortunately, this breaks a significant number of tests inside Google, and I don't have to bandwidth to fix them, so this PR is pretty much abandoned

AndrewKushnir · 2020-06-09T20:20:46Z

FYI, potentially related PR #18575 and related (duplicate?) issue #17806.

AndrewKushnir · 2020-06-10T06:17:49Z

Hi @jermowery, since this is a breaking change and it affects a lot of targets internally in google3, I was thinking if we should consider adding a config option similar to the warnOnNgModelWithFormControl one defined here. That should keep existing behavior, allow enabling new/corrected one and simplify the rollout of this change (make it incremental). WDYT?

jermowery · 2020-06-10T06:54:51Z

That seems reasonable. Sadly I don't have the bandwidth right now to incorporate that change but feel free to adopt this PR if it is a priority

sonukapoor · 2020-06-10T12:03:49Z

@jermowery / @AndrewKushnir I would be happy to make this change.

grosch · 2020-07-21T22:57:26Z

What's the status of this being implemented?

mohamed-gara · 2020-07-29T05:36:10Z

There is already this PR #20310. I think it's the same fix.

Jakartoine · 2021-02-09T16:30:46Z

@AndrewKushnir,

Sorry for disturbing you but do you have any update about this point?

To workaround this, I had to recreate a custom radio button component with an override on NG_VALUE_ACCESSOR.

Already 3 years since this bug has been discovered without any fix...

Thank you in advance

AndrewKushnir · 2021-02-10T03:32:28Z

Hi @Jakartoine,

Unfortunately we don't have any updates on this issue at this moment.

The problem with the fix is that it requires a breaking change: it changes the behavior (to be more correct), but this affects existing apps since some of them rely on the current sequence of calls to the setDisabledState methods defined on CVAs. Running tests in Google's codebase indicated that the breaking change is quite severe and there is no easy way to auto-correct the code with schematic/migration. We have to come up with a migration strategy, such as introducing a flag (something like ReactiveFormsModule.withConfig({setDisabledStateBehavior: 'legacy' /* or 'new' */})) for apps to opt into using the "correct" behavior, wait for 1-2 major versions and only after that flip the flag to make "correct" behavior default. This work requires some planning and additional investigations and we didn't have a chance to prioritize this yet. We'll keep this PR updated once we have more info available.

Thank you.

Jakartoine · 2021-02-10T08:34:51Z

@AndrewKushnir

Alright, it sounds clearer now. Thank you for your response mate.

angular-automatic-lock-bot · 2022-03-24T16:06:14Z

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

googlebot added the cla: yes label Feb 10, 2020

pullapprove Bot requested a review from AndrewKushnir February 10, 2020 20:23

AndrewKushnir requested changes Feb 11, 2020

View reviewed changes

Comment thread packages/forms/test/value_accessor_integration_spec.ts Resolved

Comment thread packages/forms/test/value_accessor_integration_spec.ts Resolved

Comment thread packages/forms/test/value_accessor_integration_spec.ts Resolved

AndrewKushnir added area: forms action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Feb 11, 2020

ngbot Bot added this to the needsTriage milestone Feb 11, 2020

AndrewKushnir requested a review from kara February 11, 2020 02:20

jermowery requested review from AndrewKushnir and removed request for kara February 11, 2020 02:22

AndrewKushnir reviewed Feb 11, 2020

View reviewed changes

AndrewKushnir requested a review from kara February 11, 2020 02:34

JoostK mentioned this pull request Mar 24, 2020

fix(forms): fix enabling form control after it was disabled #36227

Closed

4 of 14 tasks

AndrewKushnir added state: blocked breaking changes labels Mar 30, 2020

This was referenced Nov 17, 2020

fix(forms): fix disabled attribute handling #18575

Closed

fix(forms): disable radio inputs in template driven forms #20310

Closed

AndrewKushnir mentioned this pull request Apr 14, 2021

fix(forms): toggle disabled attribute when replacing control #41609

Closed

4 tasks done

JoostK mentioned this pull request Sep 18, 2021

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

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Mar 24, 2022

This pull request was closed.

Conversation

jermowery commented Feb 10, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jermowery commented Feb 11, 2020

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

jermowery commented Feb 11, 2020

Uh oh!

sonukapoor commented Mar 29, 2020

Uh oh!

jermowery commented Mar 30, 2020

Uh oh!

AndrewKushnir commented Jun 9, 2020

Uh oh!

AndrewKushnir commented Jun 10, 2020

Uh oh!

jermowery commented Jun 10, 2020

Uh oh!

sonukapoor commented Jun 10, 2020

Uh oh!

grosch commented Jul 21, 2020

Uh oh!

mohamed-gara commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jakartoine commented Feb 9, 2021

Uh oh!

AndrewKushnir commented Feb 10, 2021

Uh oh!

Jakartoine commented Feb 10, 2021

Uh oh!

angular-automatic-lock-bot Bot commented Mar 24, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mohamed-gara commented Jul 29, 2020 •

edited

Loading