'select' / 'ng-model' / 'ng-options' behaviour changed in 1.4.9

[almilo]

Dear angular guys,

after upgrading AngularJS from version 1.4.8 to 1.4.9 some unit tests in our project related with basic functionality of the 'select' / 'ng-options' / 'ng-model' directives are broken. When using 1.4.9 with a 'select' tag with 'ng-options' and 'ng-model', the corresponding option does not seem to be selected in the unit test.

Reproducible: always
Browsers: Chrome 45, Firefox 43
Operating system: Mac OS X Yosemite (10.10.3)

Steps to reproduce:
see plunker http://plnkr.co/edit/wlgE9zrTduqhlG0i8oGT?p=preview (first plunker version - angularjs 1.4.8 OK, last plunker version angularjs 1.4.9 NOK)

[Narretz]

Do I understand correctly that only the test fails, but the select behaves correctly inside the app?

[Narretz]

Ok, so for some reason if the first option is selected, the selected attribute is not set on it. But apart from that, everything seems to work correctly. It was introduced by this change apparently: f7eab8d
There's also seems to be a difference between jqLite and jQuery in this respect: with jQuery, the attribute is not set, with jqLite it's set.

[gkalpak]

This happens because (as an optimization) we check if select.value === option.selectValue (here), before doing "stuff" including setting the selected attribute. Apparently, the 1st option of a <select> gets automatically selected, so select.value will equal 1st option's value the first time this branch is reached. If the selected option that was determined by the ngModel value happens to correspond to the 1st <option> element, we skip the whole if-block (including setting the selected attribute).

[almilo]

@Narretz Yes, it is seems to be only observable through a test which checks the DOM but this can well be because of the explanation from @gkalpak here "the 1st option of a 'select' gets automatically selected".
In my Plunker I am using JQuery instead of jqLite (to use attribute based support)

[almilo]

@gkalpak I changed our test to use the 'select' tag api ('selectedIndex' and 'value') instead of using the 'selected' DOM attribute and seems to work ok, so I guess that is a more reliable way :)

Thanks!!

[gkalpak]

@almilo, I rather think we should fix this in Angular. It's a bug imo.
If you want a temporary work-around for the test, you could do something like this:

  var opt = $('[value="' + selectElement.val() + '"]');
  expect(opt.prop('selected')).toBe(true);
  expect(opt.text()).toBe(...);

[Narretz]

You can also get the option that should be selected by index:
selectElement.find('option').eq(2) and the check the selected prop, not the attribute. Another possibility should be to access the selectElements selectedIndex property, which is afaik the canocal way of getting the selected option.

[Narretz]

@gkalpak I'm not sure why it breaks with 1.4.9, because the line you've linked has been in the code for a year.

[almilo]

@gkalpak if my opinion counts, it is a change in an expected behaviour, so yes it is a regression bug.
I switched anyway to use select.selectedIndex and select.value which is IMO a better approach than relying on DOM selectors.

Many thanks guys!!

[Narretz]

So there's two things going on:

1. The line @gkalpak mentioned is reponsible for the attribute not being set, because the browser has already selected the first option automatically, but it doesn't set the attribute in this case, only the property. Before f7eab8d, the order of fn calls was different, so this is why it worked. The easy fix for this is to always set the selected attribute even if the option is already selected.
2. jqlite and jquery differ with regard to what they return from .attr('selected'). jquery returns the actual attribute value, but jqlite returns 'selected' if the selected property is defined on the element (it does that for all boolean attributes). So expect(options.eq(0).attr('selected')).toBe('selected'); currently works with jqlite but it doesn't with jquery. I'll have a look how query handles this.

[gkalpak]

Wow, that's pretty unexpected behavior ftom jqLite 😕

[Narretz]

The code for that is very old - I suppose (hope) it's an workaround for IE <= 8

[Narretz]

@mgol @jbedard Do you know how jquery handles attr for boolean attributes? I think it must be simpler than the code we use:

angular.js/src/jqLite.js

Lines 661 to 664 in c900b9c

	return (element[name] ||
	(element.attributes.getNamedItem(name) || noop).specified)
	? lowercasedName
	: undefined;

[mgol]

jQuery has a boolHook that changes .attr('aBoolAttr', anything) into .attr('aBoolAttr', 'aBoolAttr') (TBH I'm not sure if we still need that one... filed jquery/jquery#2946 for that) and .attr('aBoolAttr', false) into .removeAttr('aBoolAttr'). We have a hook for buggy behavior of the type attribute in IE<11 but that's about it. Otherwise we don't always reflect the attribute into the property, there's .prop() for that; and we certainly don't return a property state on the .attr('aBoolAttr') getter. It does seem this code should be reduced.

P.S. Please press y on the keyboard before copying a link to concrete lines in the code to make it point to a specific commit; otherwise the link will become useless once something changes on master in the file and it'd be good to keep it relevant.

[Narretz]

Nice trick with the y.
In jqLite, only the read part of attr has the bug, write seems to be okay. It's not a big deal atm, but it would be nice to have it fixed. I think I'll open an issue about it. Can I assign you @mgol? Like I said, it's not pressing.

[mgol]

@Narretz Sure, assign it to me, I'll have a look.

[mgol]

Checking for the specified property is definitely a relic of IE<9 support, in modern browsers this property is obsolete and always returns true.