Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Version 1.2.15 broke functionality with select and ng-options #7855

Closed
dnchristiansen opened this issue Jun 16, 2014 · 14 comments
Closed

Version 1.2.15 broke functionality with select and ng-options #7855

dnchristiansen opened this issue Jun 16, 2014 · 14 comments

Comments

@dnchristiansen
Copy link

We have some functionality where it's possible that after a select is changed, the model could end up back at the same value it was before. This worked fine up to version 1.2.14. See this fiddle: http://jsfiddle.net/k8EaA/
As the select is changed, the select shows the correct model ('c') after it is changed.

See this fiddle with version 1.2.15: http://jsfiddle.net/kdG38/
As the select is changed, it stays on what it was changed to, and doesn't reflect what's in the model.

We also have some functionality that removes certain options in certain cases. Basically, we have a few selects that all have the same options, and then don't allow the same option to be selected for the other selects. We do this by removing options that are already selected in the other selects.
This issue only seems to be a problem in IE11. In IE11, view this fiddle with version 1.2.14: http://jsfiddle.net/uLUXP/
Change the last select to AAA. All the other selects show the correct value based on the model.

See this fiddle with version 1.2.15: http://jsfiddle.net/S37Yp/
Change the last select to AAA. All the other selects now show the first option, which does not match the model.

It seems very likely that this behavior was introduced with this commit: dc149de
I tried reverting those changes locally, and then the selects work as expected.

I tried all this with the latest 1.2.18 version, and these issues still exist.

@andhernand
Copy link

I have also seen the same behavior. See this fiddle: http://jsfiddle.net/andhernand/hLNF2/10/

The effects are heightened in Chrome on Android.

@lgalfaso
Copy link
Contributor

Hi,
Can you check is this still an issue with #7136 ?

@andhernand
Copy link

The above patch does not fix the problem. I am seeing the same behavior with the patch applied.

@dnchristiansen
Copy link
Author

With that patch, I still have both the issues mentioned above.

@jeffbcross
Copy link
Contributor

@dnchristiansen I'm having trouble understanding the real-world use case for your first issue, where you would want to always change the value back to a hard-coded value after the user has changed the input. Can you describe a situation where you would want to do this, and what you're expecting Angular to do differently?

@dnchristiansen
Copy link
Author

That fiddle is simplified to show the problem. The real code has a bunch of logic to determine what the model value should be after the select is changed, and there are cases where it ends up back at the same value as it was before. As far as what Angular should do differently, the select should be reflecting what it is in the model. It may seem like an obscure case to set a select back to same value it was before, but the bottom line is that the model and select are getting out of sync.

@ealtenho ealtenho modified the milestones: 1.3.0-beta.16, 1.3.0-beta.15 Jul 11, 2014
@IgorMinar IgorMinar assigned ealtenho and unassigned jeffbcross Jul 14, 2014
ealtenho pushed a commit to ealtenho/angular.js that referenced this issue Jul 16, 2014
A regression angular#7855 was introduced by
angular@dc149de
This test ensures that reverting that commit fixes this regression.
In the regression, changes to a bound model in ng-change were not propagated back to the view.
ealtenho pushed a commit to ealtenho/angular.js that referenced this issue Jul 17, 2014
…render

This reverts commit dc149de. That commit fixes a bug caused by
Firefox updating `select.value` on hover. However, it
causes other bugs with select including the issue described in angular#7715. This issue details how
selects with a blank disabled option skip to the second option. We filed a bug
with Firefox for the problematic behavior the reverted commit addresses
https://bugzilla.mozilla.org/show_bug.cgi?id=1039047, and alternate Angular fixes are being
investigated. The test introduced in this commit does test a valid case and is therefore not
reverted.

Closes angular#7715 angular#7855
ealtenho pushed a commit to ealtenho/angular.js that referenced this issue Jul 17, 2014
A regression angular#7855 was introduced by
angular@dc149de
This test ensures that reverting that commit fixes this regression.
In the regression, changes to a bound model in ng-change were not propagated back to the view.

Test for angular#7855
@btford btford modified the milestones: 1.3.0-beta.17, 1.3.0-beta.16 Jul 18, 2014
@IgorMinar IgorMinar assigned IgorMinar and unassigned ealtenho Jul 23, 2014
@IgorMinar
Copy link
Contributor

we need to revert the commit that introduced this regression, but we don't have a fix for the issue that the reverted commit tried to fix yet. I looked into that but it's a bit more involved so it will take a few days.

@caitp
Copy link
Contributor

caitp commented Jul 23, 2014

I think there is a pretty narrow set of cases which would run into problems if we revert that commit --- it's basically only if digests are happening frequently while selecting an option... And in those cases (like an $interval firing frequently) it might be simpler to just tell people not to do that, until it's fixed in Gecko. There isn't much else we can do about it since we can't force the browser to do the right thing.

(So in other words, should just revert)

petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this issue Jul 28, 2014
…render

This reverts commit dc149de. That commit fixes a bug caused by
Firefox updating `select.value` on hover. However, it
causes other bugs with select including the issue described in angular#7715. This issue details how
selects with a blank disabled option skip to the second option. We filed a bug
with Firefox for the problematic behavior the reverted commit addresses
https://bugzilla.mozilla.org/show_bug.cgi?id=1039047, and alternate Angular fixes are being
investigated. The test introduced in this commit does test a valid case and is therefore not
reverted.

Closes angular#7715 angular#7855
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this issue Jul 28, 2014
A regression angular#7855 was introduced by
angular@dc149de
This test ensures that reverting that commit fixes this regression.
In the regression, changes to a bound model in ng-change were not propagated back to the view.

Test for angular#7855
ealtenho pushed a commit to ealtenho/angular.js that referenced this issue Jul 28, 2014
…render

This reverts commit dc149de. That commit fixes a bug caused by
Firefox updating `select.value` on hover. However, it
causes other bugs with select including the issue described in angular#7715. This issue details how
selects with a blank disabled option skip to the second option. We filed a bug
with Firefox for the problematic behavior the reverted commit addresses
https://bugzilla.mozilla.org/show_bug.cgi?id=1039047, and alternate Angular fixes are being
investigated.

Closes angular#7715 angular#7855
ealtenho pushed a commit to ealtenho/angular.js that referenced this issue Jul 28, 2014
…ith no change event

Commit dc149de was reverted to fix regressions angular#7715 and angular#7855.
This commit introduced this test case and a corresponding fix for preventing the update of the
selected property of an option element on a digest with no change event. Although the previous fix
introduced regressions, the test covers a valid issue and should be included.
ealtenho pushed a commit to ealtenho/angular.js that referenced this issue Jul 28, 2014
A regression angular#7855 was introduced by
angular@dc149de
This test ensures that reverting that commit fixes this regression.
In the regression, changes to a bound model in ng-change were not propagated back to the view.

Test for angular#7855
ealtenho pushed a commit that referenced this issue Jul 30, 2014
…render

This reverts commit dc149de. That commit fixes a bug caused by
Firefox updating `select.value` on hover. However, it
causes other bugs with select including the issue described in #7715. This issue details how
selects with a blank disabled option skip to the second option. We filed a bug
with Firefox for the problematic behavior the reverted commit addresses
https://bugzilla.mozilla.org/show_bug.cgi?id=1039047, and alternate Angular fixes are being
investigated.

Closes #7715 #7855
ealtenho pushed a commit that referenced this issue Jul 30, 2014
…ith no change event

Commit dc149de was reverted to fix regressions #7715 and #7855.
This commit introduced this test case and a corresponding fix for preventing the update of the
selected property of an option element on a digest with no change event. Although the previous fix
introduced regressions, the test covers a valid issue and should be included.
ealtenho pushed a commit that referenced this issue Jul 30, 2014
A regression #7855 was introduced by
dc149de
This test ensures that reverting that commit fixes this regression.
In the regression, changes to a bound model in ng-change were not propagated back to the view.

Test for #7855
ealtenho pushed a commit that referenced this issue Jul 30, 2014
…render

This reverts commit dc149de. That commit fixes a bug caused by
Firefox updating `select.value` on hover. However, it
causes other bugs with select including the issue described in #7715. This issue details how
selects with a blank disabled option skip to the second option. We filed a bug
with Firefox for the problematic behavior the reverted commit addresses
https://bugzilla.mozilla.org/show_bug.cgi?id=1039047, and alternate Angular fixes are being
investigated.

Closes #7715 #7855
ealtenho pushed a commit that referenced this issue Jul 30, 2014
…ith no change event

Commit dc149de was reverted to fix regressions #7715 and #7855.
This commit introduced this test case and a corresponding fix for preventing the update of the
selected property of an option element on a digest with no change event. Although the previous fix
introduced regressions, the test covers a valid issue and should be included.
ealtenho pushed a commit that referenced this issue Jul 30, 2014
A regression #7855 was introduced by
dc149de
This test ensures that reverting that commit fixes this regression.
In the regression, changes to a bound model in ng-change were not propagated back to the view.

Test for #7855
@Narretz
Copy link
Contributor

Narretz commented Jul 31, 2014

@dnchristiansen
There's a fix (cdc7db3) in master that also fixes both regressions you posted:

http://jsfiddle.net/k8EaA/1/
http://jsfiddle.net/S37Yp/1/

Can you confirm that your issues are fixed with that?

@dnchristiansen
Copy link
Author

I tried my app with this commit and the 1st issue is resolved.

I am still having some issues in my app with the 2nd issue, but the fiddle is obviously working. As the 2nd issue only happens in IE11, the 1st issue is higher priority for us. I don't have a problem with this issue being closed, and then I'll see if I can create a fiddle that shows the problem we still have in IE11. If I'm able to do that, I'll just open a new issue.

Is it safe to assume this commit will make its way into the v1.2.x branch?

@caitp
Copy link
Contributor

caitp commented Jul 31, 2014

The commit which broke it has already been reverted in v1.2.x (I think it was actually a pair of commits, so hopefully both were reverted)

@dnchristiansen
Copy link
Author

Are those v1.2.x commits in a released version? Or will they be included in the next 1.2.x version?

@caitp
Copy link
Contributor

caitp commented Jul 31, 2014

they'll be released on monday I believe

@dnchristiansen
Copy link
Author

As I am using the 1.2.x versions of angular, I wanted to confirm this issue was fixed with the next released version. However, the fiddles above with the latest code have definitely fixed the issue, so I will close this issue for now.

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

10 participants