Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

mdCheckbox: wrong display state when using ngChecked #1550

Closed
ngraef opened this issue Feb 17, 2015 · 8 comments
Closed

mdCheckbox: wrong display state when using ngChecked #1550

ngraef opened this issue Feb 17, 2015 · 8 comments
Assignees
Milestone

Comments

@ngraef
Copy link
Contributor

ngraef commented Feb 17, 2015

CodePen example: http://codepen.io/anon/pen/zxWrGP?editors=101

In the example, ngChecked and ngClick are used to track a list of multiple selected items. The mdCheckboxes display correct state when the model is manipulated by the standard checkboxes. However, after interacting with the mdCheckboxes, their display state is reversed. Using the standard checkboxes again fixes the mdCheckbox state.

(Related: ngChecked initially implemented for #535)

@ngraef ngraef changed the title bug(mdCheckbox): wrong display state when using ngChecked mdCheckbox: wrong display state when using ngChecked Feb 17, 2015
@ngraef
Copy link
Contributor Author

ngraef commented Mar 4, 2015

Related: check validity after the #1006 refactor

@ThomasBurleson ThomasBurleson added this to the 0.9.0 milestone Mar 11, 2015
@ThomasBurleson ThomasBurleson self-assigned this Mar 11, 2015
@djgadd
Copy link

djgadd commented Mar 23, 2015

Confirmed, there's a not operator on the checked var inside the apply:

checkbox.js:123

scope.$apply(function() {
      checked = !checked;
      ngModelCtrl.$setViewValue(checked, ev && ev.type);
      ngModelCtrl.$render();
});

Removing the ! has fixed it for me, don't know what the intention of it was and I unfortunately don't have the time to take a more in depth look, sorry! Hope this helps though.

Thanks guys.

@ngraef
Copy link
Contributor Author

ngraef commented Apr 13, 2015

@ThomasBurleson It doesn't look like 942d0b9 fixed the demo in my initial post.

@ThomasBurleson
Copy link
Contributor

@ngraef - switch ng-checked to ng-model="exists(item, selected)" and all works.

@ThomasBurleson
Copy link
Contributor

I have re-opened to investigate issues with ng-checked.

@ThomasBurleson ThomasBurleson modified the milestones: 0.10.0, 0.9.0 Apr 13, 2015
@ngraef
Copy link
Contributor Author

ngraef commented Apr 13, 2015

@ThomasBurleson Interesting, but that gives ngModel:nonassign errors (as expected). Thanks for reopening.

@ngraef
Copy link
Contributor Author

ngraef commented Apr 13, 2015

So the underlying issue is that the ngChecked attribute is watched by mdCheckbox and triggers a view value update, but then the mdCheckbox click listener also fires and toggles the view value. This explains why the display state is reversed and why the correct state is produced when manipulating the value outside of any mdCheckbox interaction (via the standard checkboxes).

A few possible fixes with varying degrees of cleanliness:

  1. Add a check to the click listener that prevents toggling of the value if ngChecked is being used (line 111).

    function listener(ev) {
       if (element[0].hasAttribute('disabled')) return;
       if (attr.ngChecked) return;
       ...
    }
  2. Set the correct value in the click listener (line 114).

    scope.$apply(function() {
       var newVal = attr.ngChecked ? attr.checked : !ngModelCtrl.$viewValue;
       ngModelCtrl.$setViewValue(newVal, ev && ev.type);
       ngModelCtrl.$render();
    });
  3. Delay execution of the scope.$watch handler for ngChecked to make it set the correct value after the click listener toggles the value (line 84).

    if (attr.ngChecked) {
       scope.$watch(
           scope.$eval.bind(scope, attr.ngChecked),
           function (newVal) {
               $timeout(function () {
                   ngModelCtrl.$setViewValue(newVal);
               });
           }
       );
    }

@ThomasBurleson
Copy link
Contributor

@ngraef - the above is very helpful. Thank you.

@ThomasBurleson ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Apr 13, 2015
@ThomasBurleson ThomasBurleson removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Apr 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants