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.

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

fix(forms): always call setDisabledState, not just when the control is disabled #47576

Closed
wants to merge 1 commit into from
Closed

fix(forms): always call setDisabledState, not just when the control is disabled #47576

wants to merge 1 commit into from

Conversation

dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Sep 29, 2022

When a CVA is added to a form, reactive forms should always call the setDisabledState method, not just when it's disabled.

This PR provides an option in the forms module config to opt-out, since it's a breaking change. We will likely use that option inside of g3 if too many tests are broken.

Fixes #35309.

@dylhunn dylhunn added type: bug/fix state: WIP area: forms target: major This PR is targeted for the next major release labels Sep 29, 2022
@dylhunn dylhunn added this to the v15 feature freeze blockers milestone Sep 29, 2022
@dylhunn dylhunn added the action: global presubmit The PR is in need of a google3 global presubmit label Sep 30, 2022
@dylhunn dylhunn force-pushed the call-setdisabledstate branch 4 times, most recently from 82f6605 to f6f5e07 Compare September 30, 2022 18:10
@dylhunn dylhunn marked this pull request as ready for review October 3, 2022 17:57
@pullapprove pullapprove bot requested review from alxhub and atscott October 3, 2022 17:58
AndrewKushnir
AndrewKushnir reviewed Oct 3, 2022
View reviewed changes
goldens/public-api/forms/index.md Show resolved Hide resolved
packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
goldens/public-api/forms/index.md Outdated Show resolved Hide resolved
packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/shared.ts Show resolved Hide resolved
packages/forms/test/value_accessor_integration_spec.ts Show resolved Hide resolved
@dylhunn dylhunn force-pushed the call-setdisabledstate branch 2 times, most recently from 64f2e6a to 10cdf83 Compare October 3, 2022 20:43
@dylhunn dylhunn added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 3, 2022
AndrewKushnir
AndrewKushnir approved these changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments below. Thank you.

packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
packages/forms/test/value_accessor_integration_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/value_accessor_integration_spec.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested review from atscott and removed request for atscott October 3, 2022 21:04
@dylhunn dylhunn added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 10, 2022
@crisbeto crisbeto mentioned this pull request Oct 11, 2022
Merged
@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 11, 2022 •
edited

The components breakage has now been fixed in angular/components#25790

@dylhunn dylhunn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 11, 2022
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Oct 11, 2022
@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 11, 2022 •
edited

merge-assistance: wait for CI to finish, then submit this together with g3 local mod

@dylhunn
fix(forms): call setDisabledState on ControlValueAcessor when con…
6ab382e 
…trol is enabled

Previously, `setDisabledState` was never called when attached if the control is enabled. This PR fixes the bug, and creates a configuration option to opt-out of the fix.

Fixes angular#35309.

BREAKING CHANGE: setDisabledState will always be called when a `ControlValueAccessor` is attached. You can opt-out with `FormsModule.withConfig` or `ReactiveFormsModule.withConfig`.
@pullapprove pullapprove bot requested a review from devversion October 11, 2022 15:00
@dylhunn dylhunn closed this Oct 11, 2022
@dylhunn dylhunn deleted the call-setdisabledstate branch October 11, 2022 15:02
@angular angular deleted a comment from ngbot bot Oct 11, 2022
@angular angular deleted a comment from ngbot bot Oct 11, 2022
@dylhunn dylhunn restored the call-setdisabledstate branch October 11, 2022 15:05
@dylhunn dylhunn mentioned this pull request Oct 11, 2022
Closed
@dylhunn dylhunn reopened this Oct 11, 2022
devversion
devversion approved these changes Oct 11, 2022
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM for dev-infra

jessicajaniuk
jessicajaniuk approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: size-tracking

@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 96b7fe9.

@dylhunn dylhunn deleted the call-setdisabledstate branch October 17, 2022 13:38
@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 Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott atscott left review comments

@devversion devversion devversion approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

@alxhub alxhub Awaiting requested review from alxhub

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: forms forms: ControlValueAccessor forms: directives merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
v15 feature freeze blockers
5 participants
@dylhunn @jessicajaniuk @atscott @devversion @AndrewKushnir