fix(select): disabled state, invalid html in unit tests

[crisbeto]

fix(select): disabled state, invalid html in unit tests
Fixes:

• Disabled mdSelect elements being marked as touched when they get blurred.
• Click and key events being bound to elements with empty disabled attributes.
• Invalid, comma-separated attributes in the mdSelect unit tests.

Cheers to @montgomery1944 for the tip about removing the tabindex.
Closes #7773;

[devversion]

LGTM.

FYI: @crisbeto and I had a discussion about the blur handler, to stop propagation, and we both agreed, that this is a good way to probably prevent the change of $touched

Also notice, that native select elements are not setting $touched as well.

[montgomery1944]

@devversion, this is not the only solution to prevent from setting untouched. I've already implemented touched setting prevention in different case using $evalAsync in #7646. This solution does not have the downside of preventing all blur event handlers from firing. I think that for both cases the same implementation should be used to be consistent.

[crisbeto]

@montgomery1944 I was also considering something similar (re-using the event handler and removing the nextTick wrapper), but in that case it's being set as touched, then back as untouched. That might still cause something to flicker or some other unexpected behavior.

[montgomery1944]

@crisbeto, because of how $evalAsync works, it is not going to cause any flicker. Async evals are run just after the regular eval, where the touched is set, and just before the digest loop, so in the digest all places in the code using touched property will see false value. As I said, consistency is important, so if your solution will be the preffered one, I will have to change my pull request.

[crisbeto]

Alright, let me try your solution and see how it goes. We might have to work out a way of combining these since whatever gets merged first will definitely cause a conflict in the other.

[crisbeto]

@montgomery1944 Just tried out your solution with $evalAsync. It works in blocking the first blur, but it doesn't work on the next. Here's what I mean:

[montgomery1944]

@crisbeto, works for me, just don't put your code inside "if (untouched) {" condition, it is needed only in my case and this is why your prevention works only once. The exact code working for me:

element.on('blur', function() {
  if (untouched) {
    untouched = false;
    if (selectScope.isOpen) {
      scope.$evalAsync(function() {
        ngModelCtrl.$setUntouched();
      });
    }
  }

  if (element[0].hasAttribute('disabled')) {
    scope.$evalAsync(function() {
      ngModelCtrl.$setUntouched();
    });
  }

  if (selectScope.isOpen) return;
  containerCtrl && containerCtrl.setFocused(false);
  inputCheckValue();
});

[crisbeto]

Alright, that works but we can reduce it to something like:

element.on('blur', function() {
  if (element[0].hasAttribute('disabled') || untouched && selectScope.isOpen) {
    scope.$evalAsync(function() {
      ngModelCtrl.$setUntouched();
    });
  }

  if (untouched) {
    untouched = false;
  }

  if (selectScope.isOpen) return;
  containerCtrl && containerCtrl.setFocused(false);
  inputCheckValue();
});

How should we go about this?

[montgomery1944]

@crisbeto, I think that there is a better solution to this issue. The root cause of the problem is the tabindex with -1 value on mdSelect element, which is removing the element from tabbing order, but it makes the element focusable by clicking. By default, mdSelect element is not focusable, so I think that changing "element.attr({'tabindex': -1, 'aria-disabled': 'true'});" to "element.attr({'aria-disabled': 'true'});" will fix the issue without the need for setting untouched.

[crisbeto]

Good point, that works even better! I suppose instead we should remove the tabindex altogether when the element becomes disabled.

[crisbeto]

So I think we should combine our fixes into 1 PR. What do you think @montgomery1944?

[montgomery1944]

@crisbeto, if you are going to fix this issue by removing tabindex, then there is no conflict between my and your pull request. Still, if you want to combine those fixes in one pull request, feel free to include changes from my pull request in yours.

[crisbeto]

Alright, I'll include the tabindex stuff in my PR. I'll also try to see if I can do anything about the events not being bound if the select goes from disabled to not disabled.

[montgomery1944]

After removal of tabindex this change is not needed, because non focusable element won't get a blur event.

[crisbeto]

Yes, but I believe that this is necessary if you're switching between an enabled and disabled element. It broke a few unit tests if I remove it.

[montgomery1944]

Weird, I've checked out your branch, reverted this change, while keeping the rest of the changes, and all tests are passing. Futhermore, I am still not sure how this is going to work, when we are not going to get any blur events without tabindex.

[crisbeto]

Commenting out that whole handler makes these fail:

• <md-select> should set touched only after closing.
• <md-select> should remove the input-container focus state FAILED.

As for the event not firing, my point is that the disabled attribute can be removed at a later point and it'll need to fire then.

[montgomery1944]

Yes, commenting the whole handler will indeed make those tests fail. I meant, that the change to $evalAsync is not needed. In later point of time, when user enables disabled mdSelect, everything should work as before. Problems with blur on mdSelect will be fixed in #7646. If you want to include those changes in your PR, as we talked about, then please include it as it is there, including the test and condition on "isOpen".

[crisbeto]

Removing the $evalAsync makes the <md-select> should set touched only after closing fail.

[montgomery1944]

Yes, if you're going to remove the whole line: "scope.$evalAsync(ngModelCtrl.$setUntouched);", then yes, it's going to fail. What I meant, is that the whole change done to those lines are not needed for the fix based on tabindex to work. Furthermore, as I said, those changes are not completely correct, because they are missing the "isOpen" condition, which is already included in #7646. If you'are going to change those lines to the previous state:

$mdUtil.nextTick(function () {
    element.on('blur', function() {
        if (untouched) {
            untouched = false;
            ngModelCtrl.$setUntouched();
        }

        if (selectScope.isOpen) return;
        containerCtrl && containerCtrl.setFocused(false);
        inputCheckValue();
    });
});

then no test is going to fail.

[crisbeto]

Got it. I've reverted those changes from my PR and tried it out to make sure everything passes and it still works.

[montgomery1944]

I think that this test is not testing the primary change in this PR. In the test blur event is triggered, while element without tabindex is not focusable, so it won't get blur event at all.

[crisbeto]

Good point, this one is useless after the latest changes. It probably makes more sense to test whether the tabindex gets removed.

[ThomasBurleson]

@crisbeto - CI is failing.

[crisbeto]

@ThomasBurleson seems like a Node error in Travis, they ran fine locally:

npm ERR! network read ECONNRESET
npm ERR! network This is most likely not a problem with npm itself
npm ERR! network and is related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! network 
npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! network 'proxy' config is set properly.  See: 'npm help config'
npm ERR! Linux 3.13.0-40-generic
npm ERR! argv "/home/travis/.nvm/versions/node/v4.2.3/bin/node" "/home/travis/.nvm/versions/node/v4.2.3/bin/npm" "install"
npm ERR! node v4.2.3
npm ERR! npm  v2.14.7
npm ERR! code ECONNRESET
npm ERR! errno ECONNRESET
npm ERR! syscall read