fix(select): avoid checking option element selected properties in render #5994

jbalboni · 2014-01-26T21:25:10Z

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

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

caitp · 2014-01-26T21:34:32Z

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

jbalboni · 2014-01-26T22:01:06Z

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.

caitp · 2014-01-26T22:18:38Z

So, I take it this test will fail on FF without the changes

jbalboni · 2014-01-26T22:27:09Z

Yes, though it fails on other browsers as well.

caitp · 2014-01-27T01:11:16Z

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

jbalboni · 2014-01-27T01:36:59Z

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

caitp · 2014-01-27T01:44:04Z

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

jbalboni · 2014-01-28T01:12:41Z

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)

mary-poppins · 2014-01-28T01:14:29Z

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.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

caitp · 2014-01-28T01:19:35Z

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.

mary-poppins · 2014-01-28T05:14:40Z

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).

jbalboni · 2014-01-30T05:02:58Z

@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.

caitp · 2014-01-31T01:01:51Z

I'm going to mark this for 1.2.11, but I need someone else to R+ this. It looks okay to me.

IgorMinar · 2014-01-31T11:27:47Z

test/ng/directive/selectSpec.js

+      });
+
+      var options = element.find('option');
+      var optionToSelect = options.eq(1);


to make the test more readable we should also add expect(optionToSelect.text()).toBe('B');

Narretz · 2014-02-19T10:11:46Z

@jbalboni, could you include the change from @IgorMinar and rebase this? It looks good to go.

jbalboni · 2014-02-19T12:25:27Z

@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.

mfollett · 2014-03-03T23:03:03Z

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?

caitp · 2014-03-03T23:07:04Z

@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

caitp · 2014-03-03T23:10:49Z

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

jbalboni · 2014-03-03T23:17:34Z

@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.

mfollett · 2014-03-04T21:57:34Z

Great, thanks @caitp!

mfollett · 2014-03-04T22:01:16Z

@caitp, I notice there doesn't seem to be a 1.2.14 milestone. Are there other things going out in 1.2.14?

Narretz · 2014-03-04T22:34:28Z

@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

caitp · 2014-03-04T22:46:52Z

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.

mfollett · 2014-03-14T19:33:01Z

I noticed this is now pushed back into beta2, is it going to make it into that and get backported?

mfollett · 2014-03-14T19:33:36Z

Is there some work I can take off of someone that would give them the time to review this?

caitp · 2014-03-14T19:41:37Z

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.

caitp · 2014-03-19T02:57:57Z

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 #2448 Closes #5994 Closes #6769

butchpeters · 2014-05-15T13:11:00Z

src/ng/directive/select.js

@@ -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) {


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.

@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.

ghost assigned caitp Jan 26, 2014

Clean up null option explicitly, instead of tracking change event

956f65d

IgorMinar reviewed Jan 31, 2014
View reviewed changes

Updated test description and added expect statements

ef9b16d

tbosch modified the milestones: 1.2.12, 1.2.11, 1.2.13 Feb 3, 2014

mfollett mentioned this pull request Feb 7, 2014

select basic directive bug in firefox #2448

Closed

btford modified the milestones: 1.2.14, 1.2.13 Feb 15, 2014

Narretz mentioned this pull request Feb 24, 2014

$timeout resets selected option #6424

Closed

IgorMinar modified the milestones: 1.3.0-beta.1, 1.2.14 Mar 1, 2014

btford modified the milestones: 1.3.0-beta.2, 1.3.0-beta.1 Mar 10, 2014

Narretz mentioned this pull request Mar 14, 2014

fix(ngOptions): don't change user selections while <select> open in fire... #6680

Closed

tbosch modified the milestones: 1.3.0-beta.3, 1.3.0-beta.2 Mar 14, 2014

heymatthew mentioned this pull request Mar 17, 2014

Firefox hover doesn't respect highlighted option in a select ng-options after digest #6679

Closed

caitp closed this in f40f54c Mar 19, 2014

caitp mentioned this pull request Mar 20, 2014

ngOptions can cause flickering/jumping highlight on Firefox #6769

Closed

butchpeters reviewed May 15, 2014
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(select): avoid checking option element selected properties in render #5994

fix(select): avoid checking option element selected properties in render #5994

jbalboni commented Jan 26, 2014

caitp commented Jan 26, 2014

jbalboni commented Jan 26, 2014

caitp commented Jan 26, 2014

jbalboni commented Jan 26, 2014

caitp commented Jan 27, 2014

jbalboni commented Jan 27, 2014

caitp commented Jan 27, 2014

jbalboni commented Jan 28, 2014

mary-poppins commented Jan 28, 2014

caitp commented Jan 28, 2014

mary-poppins commented Jan 28, 2014

jbalboni commented Jan 30, 2014

caitp commented Jan 31, 2014

IgorMinar Jan 31, 2014

Narretz commented Feb 19, 2014

jbalboni commented Feb 19, 2014

mfollett commented Mar 3, 2014

caitp commented Mar 3, 2014

caitp commented Mar 3, 2014

jbalboni commented Mar 3, 2014

mfollett commented Mar 4, 2014

mfollett commented Mar 4, 2014

Narretz commented Mar 4, 2014

caitp commented Mar 4, 2014

mfollett commented Mar 14, 2014

mfollett commented Mar 14, 2014

caitp commented Mar 14, 2014

caitp commented Mar 19, 2014

butchpeters May 15, 2014

jbalboni May 23, 2014

fix(select): avoid checking option element selected properties in render #5994

fix(select): avoid checking option element selected properties in render #5994

Conversation

jbalboni commented Jan 26, 2014

caitp commented Jan 26, 2014

jbalboni commented Jan 26, 2014

caitp commented Jan 26, 2014

jbalboni commented Jan 26, 2014

caitp commented Jan 27, 2014

jbalboni commented Jan 27, 2014

caitp commented Jan 27, 2014

jbalboni commented Jan 28, 2014

mary-poppins commented Jan 28, 2014

caitp commented Jan 28, 2014

mary-poppins commented Jan 28, 2014

jbalboni commented Jan 30, 2014

caitp commented Jan 31, 2014

IgorMinar Jan 31, 2014

Choose a reason for hiding this comment

Narretz commented Feb 19, 2014

jbalboni commented Feb 19, 2014

mfollett commented Mar 3, 2014

caitp commented Mar 3, 2014

caitp commented Mar 3, 2014

jbalboni commented Mar 3, 2014

mfollett commented Mar 4, 2014

mfollett commented Mar 4, 2014

Narretz commented Mar 4, 2014

caitp commented Mar 4, 2014

mfollett commented Mar 14, 2014

mfollett commented Mar 14, 2014

caitp commented Mar 14, 2014

caitp commented Mar 19, 2014

butchpeters May 15, 2014

Choose a reason for hiding this comment

jbalboni May 23, 2014

Choose a reason for hiding this comment