fix(jqLite): Chrome specific issue for `<select required>` #7136

lgalfaso · 2014-04-16T17:28:32Z

Request Type: bug, regression

How to reproduce: Look at #6828

Component(s): jqLite

Impact: medium

Complexity: small

This issue is related to: regression

Detailed Description:

When using Chrome and adding a <option selected="selected"> to a <select required> then the new added option is not selected

Other Comments:

mary-poppins · 2014-04-16T17:28:37Z

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

Uses the issue template (#7136)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

caitp · 2014-04-16T17:35:46Z

how was this related to a performance regression?

lgalfaso · 2014-04-16T17:59:49Z

@caitp You are right, I try to use the template and thought that "performance/regression" was a performance issue or a regression. Just fixed it

lgalfaso · 2014-04-20T23:11:08Z

I do not like this patch, just too hacky. Will close it until I find a better solution

lgalfaso · 2014-04-20T23:50:14Z

The patch at 9c66807 is a lot better than the previous patch (and it fixes the issue at the right location)

chrisirhc · 2014-04-28T20:55:52Z

+1 to this patch. Works for resolving #7202 which causes workaround on #638 (comment) to break.

Edit: I take that back. This patch doesn't work for me. I was testing on the wrong version of AngularJS when I reported that it works.

element.attr('selected') returns undefined even when element[0].getAttribute('selected') === 'selected' on jQuery 2.0.3 with jQuery Migrate.

chrisirhc · 2014-05-02T00:10:34Z

I used:

        if (element[0].hasAttribute('selected')) {
          element[0].selected = true;
        }

.attr('selected') is broken by jQuery-migrate and some versions of jQuery (e.g. 1.7.2). See http://jsfiddle.net/sirhc/VSqQf/

chrisirhc · 2014-05-13T19:09:59Z

@lgalfaso , please consider using hasAttribute in your fix, otherwise this change won't fix users that are on some versions of jQuery or using jQuery-Migrate.

lgalfaso · 2014-05-13T20:28:00Z

@mzgol can you please comment on #7136 (comment) ?

IgorMinar · 2014-06-05T22:20:26Z

src/ng/directive/select.js

@@ -171,6 +171,9 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
          $element.val(value);
          if (unknownOption.parent()) unknownOption.remove();
        }
+        if (element.attr('selected') === 'selected') {


hasAttribute('selected') is likely safer

@IgorMinar there is no hasAttribute at the jqLite level, two options, do
element[0].hasAttribute && element[0].hasAttribute('selected') or
isDefined(element.attr('selected'))

Which alternative you think it is best?

element[0].hasAttribute('selected') is fine

IgorMinar · 2014-06-10T13:26:44Z

lgtm

mgol · 2014-06-10T13:41:24Z

@lgalfaso

@mzgol can you please comment on #7136 (comment) ?

TBH I'd rather just say we support jQuery >= 1.8 and that we don't support jQuery Migrate than try to bypass the method made for what we're trying to do here just because older jQuery versions had broken semantics with respect to treating .attr/.prop.

IgorMinar · 2014-07-08T15:07:43Z

@lgalfaso I think that @mzgol is right. unless the version of jQuery are testing against is affected by this, we should just use jQuery apis.

Also can you please add a comment that this is to work around https://code.google.com/p/chromium/issues/detail?id=381459 into the code?

Can you merge this in?

chrisirhc · 2014-07-08T16:45:57Z

Take note that jQuery >= 1.9 with jQuery Migrate won't work if you use the jQuery APIs (as I mentioned above).

jQuery Migrate helps users migrate from pre-1.9 to jQuery >= 1.9, so if users add jQuery Migrate to upgrade to jQuery >= 1.8 to upgrade to say jQuery 2.0, the API will be broken (for this PR).

If you do use jQuery APIs for this PR, users that are now dependent on jQuery < 1.8 or jQuery Migrate (jQuery >= 1.9) are now restricted to jQuery >= 1.8 && jQuery < 1.9 . That is unless they rewrite their application to fix all the breaking jQuery API changes in jQuery 1.9 so they can avoid using jQuery Migrate.

IgorMinar · 2014-07-08T17:04:08Z

We currently don't test against the migrate plugin and I'm quite sure that
there are many other places where we use jquery prop() api in a way that
would be incompatible with jquery 1.8.
On Jul 8, 2014 9:46 AM, "Chris Chua" notifications@github.com wrote:

Take note that jQuery >= 1.9 with jQuery Migrate won't work if you use the
jQuery APIs (as I mentioned above).

jQuery Migrate helps users migrate from pre-1.9 to jQuery >= 1.9, so if
users add jQuery Migrate to upgrade to jQuery >= 1.8 to upgrade to say
jQuery 2.0, the API will be broken (for this PR).

If you do use jQuery APIs for this PR, users that are now dependent on
pre-jQuery 1.8 are now restricted to jQuery >= 1.8 && jQuery < 1.9 .
That is unless they rewrite their application to fix all the breaking
jQuery API changes in jQuery 1.9 so they can avoid using jQuery Migrate.

—
Reply to this email directly or view it on GitHub
#7136 (comment).

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

chrisirhc · 2014-07-08T17:14:02Z

The migrate plugin is forward compatible, and the use of the .prop API is encouraged. The use of .attr is what's broken and incompatible in past jQuery versions, especially when the .attr is checking something that's been set in .prop.

Conflicts: src/ng/directive/select.js

IgorMinar · 2014-07-08T17:18:02Z

If that's the case the we can perform the attribute check via the raw DOM
api. But I still doubt in full compatibility with jquery migrate.

On Tue, Jul 8, 2014, 10:14 AM Chris Chua notifications@github.com wrote:

The migrate plugin is forward compatible, and the use of the .prop API is
encouraged. The use of .attr is what's broken and incompatible in past
jQuery versions, especially when the .attr is checking something that's
been set in .prop.

—
Reply to this email directly or view it on GitHub
#7136 (comment).

chrisirhc · 2014-07-08T17:20:20Z

I understand that doubt since you're not testing against the migrate plugin.

I'm just trying to drive at the point that I feel there are a number of people dependent on jQuery Migrate when upgrading jQuery and this seems trivial enough to pick the safer option.

fix(jqLite): Chrome specific issue for `<select required>`

lefos987 added component: forms labels Apr 16, 2014

lefos987 added this to the 1.3.0-beta.6 milestone Apr 16, 2014

lgalfaso added cla: yes and removed cla: no labels Apr 18, 2014

lgalfaso closed this Apr 20, 2014

lgalfaso reopened this Apr 20, 2014

IgorMinar modified the milestones: 1.3.0-beta.7, 1.3.0-beta.6 Apr 22, 2014

lgalfaso mentioned this pull request Apr 22, 2014

Select option with 'disabled' attribute is not selected by default when model value is empty. #7202

Closed

petebacondarwin modified the milestones: 1.3.0-beta.8, 1.3.0-beta.7 Apr 27, 2014

lgalfaso mentioned this pull request May 9, 2014

Preselected disabled elements no longer work since Angular 1.2.15. #7413

Closed

caitp modified the milestones: 1.3.0-beta.9, 1.3.0-beta.8 May 9, 2014

IgorMinar modified the milestones: 1.3.0, 1.3.0-beta.9 May 12, 2014

IgorMinar reviewed Jun 5, 2014
View reviewed changes

lgalfaso mentioned this pull request Jun 17, 2014

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

Closed

btford modified the milestones: 1.3.0-beta.14, 1.3.0 Jun 30, 2014

btford assigned IgorMinar Jun 30, 2014

caguillen214 modified the milestones: 1.3.0-beta.15, 1.3.0-beta.14 Jun 30, 2014

fix(select): auto-select new option that is marked as selected

8bc2667

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

Merge branch 'master' into issue-6828

c3a0bcc

Conflicts: src/ng/directive/select.js

lgalfaso added a commit that referenced this pull request Jul 8, 2014

Merge pull request #7136 from lgalfaso/issue-6828

14e3ba8

fix(jqLite): Chrome specific issue for `<select required>`

lgalfaso merged commit 14e3ba8 into angular:master Jul 8, 2014

lgalfaso mentioned this pull request Jul 8, 2014

fix(select): auto-select new option that is marked as selected #8106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jqLite): Chrome specific issue for `<select required>` #7136

fix(jqLite): Chrome specific issue for `<select required>` #7136

lgalfaso commented Apr 16, 2014

mary-poppins commented Apr 16, 2014

caitp commented Apr 16, 2014

lgalfaso commented Apr 16, 2014

lgalfaso commented Apr 20, 2014

lgalfaso commented Apr 20, 2014

chrisirhc commented Apr 28, 2014

chrisirhc commented May 2, 2014

chrisirhc commented May 13, 2014

lgalfaso commented May 13, 2014

IgorMinar Jun 5, 2014

lgalfaso Jun 9, 2014

caitp Jun 9, 2014

IgorMinar commented Jun 10, 2014

mgol commented Jun 10, 2014

IgorMinar commented Jul 8, 2014

chrisirhc commented Jul 8, 2014

IgorMinar commented Jul 8, 2014

chrisirhc commented Jul 8, 2014

IgorMinar commented Jul 8, 2014

chrisirhc commented Jul 8, 2014

fix(jqLite): Chrome specific issue for <select required> #7136

fix(jqLite): Chrome specific issue for <select required> #7136

Conversation

lgalfaso commented Apr 16, 2014

mary-poppins commented Apr 16, 2014

caitp commented Apr 16, 2014

lgalfaso commented Apr 16, 2014

lgalfaso commented Apr 20, 2014

lgalfaso commented Apr 20, 2014

chrisirhc commented Apr 28, 2014

chrisirhc commented May 2, 2014

chrisirhc commented May 13, 2014

lgalfaso commented May 13, 2014

IgorMinar Jun 5, 2014

Choose a reason for hiding this comment

lgalfaso Jun 9, 2014

Choose a reason for hiding this comment

caitp Jun 9, 2014

Choose a reason for hiding this comment

IgorMinar commented Jun 10, 2014

mgol commented Jun 10, 2014

IgorMinar commented Jul 8, 2014

chrisirhc commented Jul 8, 2014

IgorMinar commented Jul 8, 2014

chrisirhc commented Jul 8, 2014

IgorMinar commented Jul 8, 2014

chrisirhc commented Jul 8, 2014

fix(jqLite): Chrome specific issue for `<select required>` #7136

fix(jqLite): Chrome specific issue for `<select required>` #7136