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

ngOptions : regression 1.2.15 && 1.3.0-beta.3 #6828

Closed
GabrielDelepine opened this issue Mar 25, 2014 · 21 comments · Fixed by #8106
Closed

ngOptions : regression 1.2.15 && 1.3.0-beta.3 #6828

GabrielDelepine opened this issue Mar 25, 2014 · 21 comments · Fixed by #8106

Comments

@GabrielDelepine
Copy link

When the select tag has the required attribute, the default blank option is ignore.

1.2.14 works
1.2.15 doesn't
1.3.0-beta.3 doesn't

See the jsfiddle

@Narretz
Copy link
Contributor

Narretz commented Mar 25, 2014

I can't reproduce this in FF 28. 1.2.14 / 1.2.15 look the same. Which browser did you test? There was a fix for a Firefox ngOptions render bug, maybe this is related. f40f54c

@GabrielDelepine
Copy link
Author

Sorry, I forgot to mention this.

Chrome 30 : bug
Chromium 32 : bug
Firefox 27 : no bug

@caitp
Copy link
Contributor

caitp commented Mar 25, 2014

WFM in Chrome stable

@GabrielDelepine
Copy link
Author

I just update to Chromium 33 and I still have the issue.

@caitp
Copy link
Contributor

caitp commented Mar 25, 2014

Do you have all your extensions disabled when testing?

@GabrielDelepine
Copy link
Author

@caitp Yes, all extensions disabled and with the incognito mode of chrome. My OS is Ubuntu.
Chromium = Ubuntu repo
Chrome = Google repo

I also tried with Chrome under W7 (fresh install) and this bug also ! See this screenshot

IE11 doesn't bug

@caitp
Copy link
Contributor

caitp commented Mar 25, 2014

I can't reproduce on OSX 10.9 or Windows 8.1 with Chrome 33. You should investigate it, but it's possibly a browser bug.

@lgalfaso
Copy link
Contributor

@yappli I am able to reproduce this with Chrome 33 and OSX 10.9 and it does look like a browser bug.
The easiest workaround I was able to find is to change the attribute require to ng-required="true"

@GabrielDelepine
Copy link
Author

@lgalfaso : Your workaround works for me, thanks.

I don't know if it's a browser bug or not. Probably.

@caitp
Copy link
Contributor

caitp commented Mar 25, 2014

The reason I couldn't reproduce it is because you posted an example of the working build, not of the broken build. This is bad, don't do this, it's not helpful. Especially on jsfiddle where the UI doesn't make it obvious that you're using the working build!

So yes, there is a regression from the fix to break the problem in Firefox. Someone should look into this, PRs welcome, although I suppose there is a workaround to fix this.

@caitp caitp added the PRs plz! label Mar 25, 2014
@GabrielDelepine
Copy link
Author

Sorry @caitp, I edited my original post.

@btford btford added this to the 1.3.0 milestone Mar 25, 2014
@IgorMinar
Copy link
Contributor

@lgalfaso can you look into this one please?

@IgorMinar IgorMinar modified the milestones: 1.3.0-beta.6, 1.3.0 Apr 14, 2014
@martineboh
Copy link

Came across this issue, but wasn't able to fix it. Thanks for raising

On Mon, Mar 24, 2014 at 11:50 PM, Gabriel Delépine <notifications@github.com

wrote:

When the select tag has the required attribute, the default blank option
is ignore.

1.2.14 works
1.2.15 doesn't
1.3.0-beta.3 doesn't

See the jsfiddle http://jsfiddle.net/yappli/X8era/18/


Reply to this email directly or view it on GitHubhttps://github.com//issues/6828
.

@lgalfaso
Copy link
Contributor

@IgorMinar sure

@lgalfaso
Copy link
Contributor

it looks like the issue is a Chrome specific bug. The issue is that when adding a <option value="" selected="selected"> to an <select required> then the option is not selected at the UI. Still trying to figure out if the fact that we do not add the original option that was removed but a clone makes a difference, but it looks like it does not

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Apr 16, 2014
When adding an `<option selected="selected">` into the DOM, put
the property `selected` to `true`. This works around an issue
present at Chrome where the new `<option>` is selected at the DOM
not selected at the UI.

Closes angular#6828
@lgalfaso
Copy link
Contributor

The patch at 2011a0d is not nice as it is a workaround a Chrome bug, but does get the work done

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Apr 20, 2014
When adding a new <option> element, if the DOM of this option element
states that the element is marked as `selected`, then select the new
<option> element

Closes angular#6828
@IgorMinar IgorMinar added this to the 1.3.0-beta.7 milestone Apr 22, 2014
@caitp
Copy link
Contributor

caitp commented Jun 5, 2014

At Igor's request, bug has been filed on this with Chromium (https://code.google.com/p/chromium/issues/detail?id=381459)

I've also discussed this briefly with Ian Hickson / folks present on #whatwg, and it seems like the behaviour is underspecified. If I'm misinterpreting anything from the reproduction, don't hesitate to comment on the bugs and correct me.

lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Jun 9, 2014
When adding a new <option> element, if the DOM of this option element
states that the element is marked as `selected`, then select the new
<option> element

Closes angular#6828
@btford btford modified the milestones: 1.3.0-beta.14, 1.3.0 Jun 30, 2014
@btford btford assigned IgorMinar and unassigned lgalfaso Jun 30, 2014
lgalfaso added a commit to lgalfaso/angular.js that referenced this issue Jul 8, 2014
When adding a new <option> element, if the DOM of this option element
states that the element is marked as `selected`, then select the new
<option> element

Closes angular#6828
lgalfaso added a commit that referenced this issue Jul 8, 2014
When adding a new <option> element, if the DOM of this option element
states that the element is marked as `selected`, then select the new
<option> element

Closes #6828
ckknight pushed a commit to ckknight/angular.js that referenced this issue Jul 16, 2014
When adding a new <option> element, if the DOM of this option element
states that the element is marked as `selected`, then select the new
<option> element

Closes angular#6828
@caitp
Copy link
Contributor

caitp commented Aug 7, 2014

Hixie has clarified the spec on this point: http://html5.org/tools/web-apps-tracker?from=8713&to=8714

I'm not sure if the algorithm runs before or after the script's turn, so I'm not totally sure how this would affect the test case on the chromium bug.

It seems like this change to the spec might make FF's behaviour "wrong", will have to check.

@shahata
Copy link
Contributor

shahata commented Oct 4, 2014

Is this issue still happening to anyone? The fiddle in the original post works as expected to me in latest Chrome. I think that the patch that fixed it can be removed...
@caitp @lgalfaso - Can you confirm that the patch is no longer required in latest Chrome or am I just confused?

Also, there is a minor bug (I think) in the patch that causes some unexpected behavior, but I hope it can be solved simply by removing the patch. Here's a plunker that demonstrates it: http://plnkr.co/edit/mqu6HbmjJd3QNiavzdlv?p=preview

WDYT?

@lgalfaso
Copy link
Contributor

lgalfaso commented Oct 5, 2014

@shahata What is going on with your example is the following. The value of default is not the empty string but the string 'default'. If you change

<option selected="selected">{{default}}</option>

to

<option value="" selected="selected">{{default}}</option>

then the value does not change.

I am lean towards thinking that this is undefined behavior. BTW, the same can be said with http://plnkr.co/edit/amnWn3lhLTEwkBkyM65b?p=preview

@shahata
Copy link
Contributor

shahata commented Oct 5, 2014

Yeah, I know what is happening. I'm just saying it doesn't make sense to check hasAttribute('selected') each time the watcher is invoked (it is only needed the first time). But that's not important - the question is if the original issue reproduces for you and if this patch is still needed?

Narretz added a commit to Narretz/angular.js that referenced this issue Jun 2, 2016
In the Chrome issue (https://bugs.chromium.org/p/chromium/issues/detail?id=381459)
I noticed that the bug can't be reproduced in Chrome 45 and the issue was
closed subsequently. I've also tested again with Chrome 51 and it still works.

Related: angular#6828
Narretz added a commit to Narretz/angular.js that referenced this issue Jun 3, 2016
In the Chrome issue (https://bugs.chromium.org/p/chromium/issues/detail?id=381459)
I noticed that the bug can't be reproduced in Chrome 45 and the issue was
closed subsequently. I've also tested again with Chrome 51 and it still works.

Related: angular#6828
Narretz added a commit that referenced this issue Jun 3, 2016
In the Chrome issue (https://bugs.chromium.org/p/chromium/issues/detail?id=381459)
I noticed that the bug can't be reproduced in Chrome 45 and the issue was
closed subsequently. I've also tested again with Chrome 51 and it still works.

Additionally, the Chrome hack was kept in the select code, but wasn't ported to
the ngOptions code when the two split in 7fda214,
and no regression happened.

Nevertheless, a test has been added to guard against a future regression.

Related: #6828
PR: #14705
gkalpak pushed a commit to gkalpak/angular.js that referenced this issue Jun 3, 2016
In the Chrome issue (https://bugs.chromium.org/p/chromium/issues/detail?id=381459)
I noticed that the bug can't be reproduced in Chrome 45 and the issue was
closed subsequently. I've also tested again with Chrome 51 and it still works.

Additionally, the Chrome hack was kept in the select code, but wasn't ported to
the ngOptions code when the two split in 7fda214,
and no regression happened.

Nevertheless, a test has been added to guard against a future regression.

Related: angular#6828
PR: angular#14705
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.