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.

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

fix(slide-toggle): error when disabling while focused #12325

Conversation

devversion
Copy link
Member

  • Fixes that Angular throws an ExpressionChangedAfterItHasBeenCheckedError when disabling the slide-toggle while the component has been focused.

Fixes #12323

@devversion devversion added the target: patch This PR is targeted for the next patch release label Jul 23, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 23, 2018
this.onTouched();
// Do not immediately mark the component as touched because it can happen that the `blur`
// event from `FocusMonitor` fires, while the component is checked after a change detection.
// Immediately updating would then result in a changed after checked exception. Rel: #12323
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite follow this scenario; what is it that's being updated after check?

Copy link
Member Author

@devversion devversion Jul 23, 2018

Choose a reason for hiding this comment

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

Calling onTouched updates the ContolValueAccessor's state (which is basically the slide-toggle) and technically the associated control.

We could theoretically also check for NgControl to be present, but I don't think it's worth having a condition here.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't follow, though, what is triggering an update while/after change detection runs. Handling an event from the focus monitor should inherently happen in a separate tick, no?

Copy link
Member Author

@devversion devversion Jul 23, 2018

Choose a reason for hiding this comment

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

That's at least what I figured out from debugging:

  1. Underlying input element is focused
  2. Disabled binding is set --> Run change detection
  3. Update disabled property of underlying input (part of change detection)
  4. Setting disabled on a native input element while focused causes a blur event to fire (not in the same tick --> so outside of the change detection
  5. Angular checks for changes after the CD ran: Figures out that the ng-untouched class binding of the NgModel changed after the actual change detection ran. --> Error thrown!

The comment tries to shortly explain that general issue. From what it says, it's pretty much what happens in 5) and it's also referring to that specific GitHub issue.

Copy link
Member

@jelbourn jelbourn Jul 24, 2018

Choose a reason for hiding this comment

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

Ah, I was missing the fact that setting an input to disabled immediately fires a blur event. Found angular/angular#17793 that references this. I would phrase the comment as:

// When a focused element becomes disabled, the browser *immediately*
// fires a 'blur' event. Angular does not expect events to be raised during
// change detection, so any template state updates in response to 'blur'
// (such as a form control's 'ng-touched') will cause a changed-after-checked
// error. See https://github.com/angular/angular/issues/17793.
// To work around this, we defer telling the form control it has been touched
// until the next tick.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. I didn't even start thinking about it potentially being an Angular issue.

* Fixes that Angular throws an `ExpressionChangedAfterItHasBeenCheckedError` when disabling the slide-toggle while the component has been focused.

Fixes angular#12323
@devversion devversion force-pushed the fix/slide-toggle-error-when-disabling-while-focused branch from ce81d78 to 0eb5ec1 Compare July 25, 2018 13:19
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 25, 2018
@jelbourn jelbourn merged commit e273a7a into angular:master Aug 1, 2018
@devversion devversion deleted the fix/slide-toggle-error-when-disabling-while-focused branch August 1, 2018 14:19
jelbourn pushed a commit that referenced this pull request Aug 7, 2018
Fixes Angular throwing an `ExpressionChangedAfterItHasBeenCheckedError` when disabling the slide-toggle while the component has been focused.

Fixes #12323
jelbourn pushed a commit that referenced this pull request Aug 7, 2018
Fixes Angular throwing an `ExpressionChangedAfterItHasBeenCheckedError` when disabling the slide-toggle while the component has been focused.

Fixes #12323
@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 Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MatSlideToggle: ngModel & [disabled] gives error
3 participants