fix(select): avoid checking option element selected properties in render #5994
Conversation
In Firefox, hovering over an option in an open select menu updates the selected property of option elements. This means that when a render is triggered by the digest cycle, and the list of options is being rendered, the selected properties are reset to the values from the model and the option hovered over changes. This fix changes the code to only use DOM elements' selected properties in a comparison when a change event has been fired. Otherwise, the internal new and existing option arrays are used. Closes angular#2448
|
I like this, but I'd like to see some test cases on plnkr just to verify that the added test is enough. I might have some comments on the code itself in a bit as well |
ghost
assigned 
|
Here's a plnkr with the code I was using for testing (and my build's angular.js file): http://plnkr.co/edit/cCMVP9Uqj3SunPOsT0Am?p=preview I am fairly new to Angular, so I wasn't sure how best to track if a render was the result of a change event. That's my main uncertainty with the change. |
|
So, I take it this test will fail on FF without the changes |
|
Yes, though it fails on other browsers as well. |
|
Hmm, so there's a trade-off here, where previously using DOM manipulation to set the value would work, now it won't. I wonder if there's a way to accomplish this without hurting people who really want to use DOM manipulation. I'm not necessarily saying that it's a huge problem, since DOM manipulation does not have much of a place in Angular, but it's something to think about |
|
Did it work before? If I plug 1.2.9 into that plunker and use jQuery to change the select's value, the model doesn't change (the select changes for a second until the digest cycle runs and then reverts). With this change, the only difference I see is that the select doesn't revert back when the digest runs. Here's what I mean: http://plnkr.co/edit/zxYrvvlP3LTI3hF9Xha4?p=preview |
|
Yeah I see what you mean, I guess we change momentarily but change back because of the model value... So maybe this fixes another bug as well |
|
If you are doing DOM manipulation, you can still trigger a change event on the select and it updates correctly. That's what ui-select2 does, for example. (Here's a plnkr to verify that this doesn't break ui-select2, by the way: http://plnkr.co/edit/IF14n7AYjU7s3Ck8ytBF?p=preview) |
|
I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS. Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match. If you signed the CLA as a corporation, please let us know the company's name. Thanks a bunch! PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR. |
|
Yeah I'm glad to hear it, I realized I was wrong about DOM manipulation being an issue already =) However, I think it would be preferable to find a fix for this that didn't involve tracking whether update was called from an event handler. Going to need someone else to R+ this patch anyways, so if someone else thinks it's an acceptable solution then we can probably merge it. |
|
CLA signature verified! Thank you! Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes). |
|
@caitp Here's an updated plnkr for the commit I just added: http://plnkr.co/edit/fS2TKk1dPhTe9FrKHbjA?p=preview This seems like a clearly better fix than what I had earlier. |
|
I'm going to mark this for 1.2.11, but I need someone else to R+ this. It looks okay to me. |
| }); | ||
|
|
||
| var options = element.find('option'); | ||
| var optionToSelect = options.eq(1); |
There was a problem hiding this comment.
to make the test more readable we should also add expect(optionToSelect.text()).toBe('B');
|
@jbalboni, could you include the change from @IgorMinar and rebase this? It looks good to go. |
|
@Narretz Sorry, I should have put a comment up there. I added his change in my last commit, but it didn't mark the comment as outdated. |
|
Is there any chance this can get pulled back up to something sooner than 1.3.0? That seems like a long way to fix selects in FireFox. Is FireFox that low of a priority? |
|
@mfollett hmm, I don't think this is breaking anything, so we probably could have merged it in 1.2.14. I'll see what people think, but the team isn't all available today or tomorrow, so it might be a few days for an answer |
|
Actually, apparently this is assigned to me, so... since this has an LGTM from me, I think I'll merge this in a few days, after doing a bit more digging to see how the code review has been addressed |
|
@caitp Let me know if I can help at all. Two of the code review comments were pretty simple to address, but there was one that I only commented on, since I think it was a misunderstanding of the fix. |
|
Great, thanks @caitp! |
|
@caitp, I notice there doesn't seem to be a 1.2.14 milestone. Are there other things going out in 1.2.14? |
|
@mfollett 1.2.14 was released on Saturday, but there's no 1.2.15 milestone. I assume the next milestone is (unstable) 1.3.0-beta.1, but I hope that non-breaking fixes will be included in the 1.2.x lifecycle in a timely manner. Previously, 1.0.x received fixes that went into 1.1,x |
|
We were talking about this in the meeting, we don't have much of an ESR strategy, but we will probably be backporting certain things, at least for a while, depending on how difficult it is to backport, similar to how it was done previously. |
|
I noticed this is now pushed back into beta2, is it going to make it into that and get backported? |
|
Is there some work I can take off of someone that would give them the time to review this? |
|
Well it's got an LGTM from me, and it looks like all of Igor's comments have been addressed as far as I can tell, and the patch doesn't look like it should cause a problem for 1.2.x, so I expect this can be cherrypicked in there. |
|
Okay, so I didn't talk to Igor about this one today, but again, it looks alright to me on FF27 and 30, and Igor's comments have been addressed, so I'm going to check this in. If it needs to be reverted or fixed up, can worry about that later, but I doubt it will be |
In Firefox, hovering over an option in an open select menu updates the selected property of option elements. This means that when a render is triggered by the digest cycle, and the list of options is being rendered, the selected properties are reset to the values from the model and the option hovered over changes. This fix changes the code to only use DOM elements' selected properties in a comparison when a change event has been fired. Otherwise, the internal new and existing option arrays are used. Closes angular#2448 Closes angular#5994
In Firefox, hovering over an option in an open select menu updates the selected property of option elements. This means that when a render is triggered by the digest cycle, and the list of options is being rendered, the selected properties are reset to the values from the model and the option hovered over changes. This fix changes the code to only use DOM elements' selected properties in a comparison when a change event has been fired. Otherwise, the internal new and existing option arrays are used. Closes angular#2448 Closes angular#5994
In Firefox, hovering over an option in an open select menu updates the selected property of option elements. This means that when a render is triggered by the digest cycle, and the list of options is being rendered, the selected properties are reset to the values from the model and the option hovered over changes. This fix changes the code to only use DOM elements' selected properties in a comparison when a change event has been fired. Otherwise, the internal new and existing option arrays are used. Closes angular#2448 Closes angular#5994
In Firefox, hovering over an option in an open select menu updates the selected property of option elements. This means that when a render is triggered by the digest cycle, and the list of options is being rendered, the selected properties are reset to the values from the model and the option hovered over changes. This fix changes the code to only use DOM elements' selected properties in a comparison when a change event has been fired. Otherwise, the internal new and existing option arrays are used. Closes #2448 Closes #5994 Closes #6769
| @@ -529,7 +535,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |||
| lastElement.val(existingOption.id = option.id); | |||
| } | |||
| // lastElement.prop('selected') provided by jQuery has side-effects | |||
| if (lastElement[0].selected !== option.selected) { | |||
| if (existingOption.selected !== option.selected) { | |||
There was a problem hiding this comment.
I found that this broke a test on my angular application related to selected option validation. Here's an example of what's broken: http://plnkr.co/edit/hPjcZP?p=preview
If you change the angular version to 1.2.14 or earlier, the behavior I expected works fine. The problem with this fix introduced in 1.2.15, when I revert the value back to the previous selected option each 'existingOption' looks the same as 'option' and so the 'selected' attribute on the select isn't updated. This causes the select element to become out of sync with the model.
Is there a better way to fix this issue? Is there a better way for me to implement my behavior?
I understand that this isn't how validation is typically done, but in my case I would rather revert the model value rather than setting it to undefined, if possible.
There was a problem hiding this comment.
@butchpeters I could be totally wrong, but I would say that what you're doing is not the best use of parsers, since they're meant to transform or inspect values for the model and not to set the values in your view. It looks like 1.3 has some commit and rollback functions which would help here, though.
Is there a reason you can't set the select to invalid ($setValidity)? It's a little weird for me as a user to choose an option and then have it instantly change back (but I don't know what this actually looks like in your app).
The 'correct' behavior here to me would be to set the disabled attribute on the option you don't want chosen. That's hard with ng-options, but there are some solutions here: http://stackoverflow.com/questions/16202254/ng-options-with-disabled-rows. You could also remove the unselectable option with a filter on the ng-options expression.
In Firefox, hovering over an option in an open select menu updates the selected property of option
elements. This means that when a render is triggered by the digest cycle, and the list of options
is being rendered, the selected properties are reset to the values from the model and the option
hovered over changes. This fix changes the code to only use DOM elements' selected properties in a
comparison when a change event has been fired. Otherwise, the internal new and existing option
arrays are used.
This is based on the fix given in #2448, but updated to fix the regression discussed there.
Closes #2448