-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(select): disabled state, invalid html in unit tests #7778
Conversation
LGTM. FYI: @crisbeto and I had a discussion about the Also notice, that native select elements are not setting |
@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. |
@montgomery1944 I was also considering something similar (re-using the event handler and removing the |
@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. |
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. |
@montgomery1944 Just tried out your solution with |
@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:
|
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? |
@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. |
Good point, that works even better! I suppose instead we should remove the tabindex altogether when the element becomes disabled. |
So I think we should combine our fixes into 1 PR. What do you think @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. |
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. |
9abf117
to
06c58cb
Compare
containerCtrl && containerCtrl.setFocused(false); | ||
inputCheckValue(); | ||
}); | ||
element.on('blur', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removal of tabindex this change is not needed, because non focusable element won't get a blur event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the $evalAsync
makes the <md-select> should set touched only after closing
fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I've reverted those changes from my PR and tried it out to make sure everything passes and it still works.
@@ -153,6 +153,16 @@ describe('<md-select>', function() { | |||
expect($rootScope.myForm.select.$touched).toBe(true); | |||
})); | |||
|
|||
it('should not set touched if the element is disabled', inject(function($compile, $rootScope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, this one is useless after the latest changes. It probably makes more sense to test whether the tabindex gets removed.
ef44262
to
7a97395
Compare
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 angular#7773;
7a97395
to
40b5e78
Compare
@crisbeto - CI is failing. |
@ThomasBurleson seems like a Node error in Travis, they ran fine locally:
|
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; Closes #7778
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; Closes #7778
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 angular#7773; Closes angular#7778
fix(select): disabled state, invalid html in unit tests
Fixes:
mdSelect
elements being marked astouched
when they get blurred.disabled
attributes.mdSelect
unit tests.Cheers to @montgomery1944 for the tip about removing the tabindex.
Closes #7773;