fix(select): keep original selection when using shift to add options … #15676

Narretz · 2017-02-03T16:32:12Z

…in IE/Edge

In IE9-11 + Edge, the selected options were previously incorrect under the following
circumstances:

at least two options are selected
shift+click or shift+down/up is used to add to the selection (any number of options)

In these cases, only the last of the previously selected options and the newly selected
options would be selected.

The problems seems to be that the render engine gets confused when an option that
already has selected = true gets selected = true set again.

Note that this is not testable via unit test because it's not possible to simulate
click / keyboard events on option elements (the events are delegated to the select element
change event).

Fixes #15675

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bugfix

What is the current behavior? (You can also link to an open issue here)
See #15675

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
~~[ ] Tests for the changes have been added (for bug fixes / features)~~
~~[ ] Docs have been added / updated (for bug fixes / features)~~

Other information:

jbedard · 2017-02-03T16:54:52Z

src/ng/directive/select.js

+            // IE and Edge will de-select selected options when you set the selected property again, e.g.
+            // when you add to the selection via shift+click/UP/DOWN
+            // Therefore, only set it if necessary
+            if ((shouldBeSelected && !currentlySelected) || (!shouldBeSelected && currentlySelected)) {


shouldBeSelected !== currentlySelected?

Oh, I had the feeling this could be simpler :D

If you swithc to !== you need to make sure both values are booleans.
(I am not sure about option.selected. Theoretically it should always be boolean, but you never know especially with older browsers.)

gkalpak

Couldn't this be tested by directly calling selectCtrl.writeValue?

gkalpak · 2017-02-03T23:08:59Z

src/ng/directive/select.js

-            option.selected = !!value && (includes(value, option.value) ||
-                                          includes(value, selectCtrl.selectValueMap[option.value]));
+            var shouldBeSelected = !!value && (includes(value, option.value) ||
+                                                includes(value, selectCtrl.selectValueMap[option.value]));


Nit: Align with includes above.

gkalpak · 2017-02-03T23:12:32Z

src/ng/directive/select.js

+            // IE and Edge will de-select selected options when you set the selected property again, e.g.
+            // when you add to the selection via shift+click/UP/DOWN
+            // Therefore, only set it if necessary
+            if ((shouldBeSelected && !currentlySelected) || (!shouldBeSelected && currentlySelected)) {


If you swithc to !== you need to make sure both values are booleans.
(I am not sure about option.selected. Theoretically it should always be boolean, but you never know especially with older browsers.)

…in IE/Edge In IE9-11 + Edge, the selected options were previously incorrect under the following circumstances: - at least two options are selected - shift+click or shift+down/up is used to add to the selection (any number of options) In these cases, only the last of the previously selected options and the newly selected options would be selected. The problems seems to be that the render engine gets confused when an option that already has selected = true gets selected = true set again. Note that this is not testable via unit test because it's not possible to simulate click / keyboard events on option elements (the events are delegated to the select element change event), and the problem also doesn't appear when modifying the option elements directly and then using the selectController API. It seems that this only happens when you manipulate the select directly in the user interface. Fixes angular#15675 Closes angular#15676

Narretz · 2017-02-05T16:34:01Z

I tried to test it directly with the selectCtrl API, but this doesn't replicate the bug. It must have something to do with the ui of the select box in IE / Edge.

gkalpak · 2017-02-05T17:43:51Z

Maybe we could just test that we don't re-set the selected property. (I just realized this seems equivalent to #15478, but using <option> instead of ngOptions).

Maybe something like this would work (although it borderline testing "implementation details").

Narretz · 2017-02-06T12:30:41Z

Hm. I agree that this is an implementation detail. I'll let you decide if I should add this test or not.

gkalpak

"No test" is fine by me.

gkalpak · 2017-02-07T20:04:27Z

src/ng/directive/select.js

+            if ((shouldBeSelected && !currentlySelected)) {
+              setOptionAsSelected(jqLite(option));
+            } else if ((!shouldBeSelected && currentlySelected)) {
+              option.selected  = shouldBeSelected;


I wonder why we are using option.selected here, but option.prop('selected', true) in setOptionAsSelected().
Probably doesn't matter, so we should use the same for consistency. In fact, I think it is better to make setOptionSelected() a helper that can both select and unselect an option and use that whenever needed.

(BTW, there is a double space before the = 😱

gkalpak · 2017-02-08T13:08:19Z

src/ng/directive/select.js

+function setOptionSelectedStatus(optionEl, value) {
+  optionEl.prop('selected', value); // needed for IE
+  if (value) {
+    optionEl.attr('selected', value);


FWIW: .attr('selected', null) also removes the attribute. So, this could be:

optionEl.prop('selected', value); optionEl.attr('selected', value || null);

gkalpak · 2017-02-08T15:33:34Z

src/ng/directive/select.js

+   * However, screenreaders might react to the selected attribute instead, see
+   * https://github.com/angular/angular.js/issues/14419
+   */
+  optionEl.attr('selected', value || null);


I said it works, but do we have tests that the property is actually removed? 😛

Oh, you little rascal!
We have the toBeMarkedAsSelected() custom matcher which complains when the selected attr is in the wrong state. And jqlite also removes the attribute when you pass false and the element is boolean. So I'm going with that.

This helps screen readers identify the selected options, see angular#14419

…in IE/Edge In IE9-11 + Edge, the selected options were previously incorrect under the following circumstances: - at least two options are selected - shift+click or shift+down/up is used to add to the selection (any number of options) In these cases, only the last of the previously selected options and the newly selected options would be selected. The problems seems to be that the render engine gets confused when an option that already has selected = true gets selected = true set again. Note that this is not testable via unit test because it's not possible to simulate click / keyboard events on option elements (the events are delegated to the select element change event), and the problem also doesn't appear when modifying the option elements directly and then using the selectController API. It seems that this only happens when you manipulate the select directly in the user interface. Fixes #15675 Closes #15676

…in IE/Edge In IE9-11 + Edge, the selected options were previously incorrect under the following circumstances: - at least two options are selected - shift+click or shift+down/up is used to add to the selection (any number of options) In these cases, only the last of the previously selected options and the newly selected options would be selected. The problems seems to be that the render engine gets confused when an option that already has selected = true gets selected = true set again. Note that this is not testable via unit test because it's not possible to simulate click / keyboard events on option elements (the events are delegated to the select element change event), and the problem also doesn't appear when modifying the option elements directly and then using the selectController API. It seems that this only happens when you manipulate the select directly in the user interface. Fixes angular#15675 Closes angular#15676

Narretz added component: forms type: bug labels Feb 3, 2017

Narretz added this to the 1.6.x milestone Feb 3, 2017

Narretz requested a review from gkalpak February 3, 2017 16:32

googlebot added the cla: yes label Feb 3, 2017

jbedard reviewed Feb 3, 2017

View reviewed changes

gkalpak approved these changes Feb 3, 2017

View reviewed changes

Narretz force-pushed the fix-select-multiple-ie-15675 branch from 471c4de to 14f8ce3 Compare February 5, 2017 16:32

gkalpak suggested changes Feb 7, 2017

View reviewed changes

gkalpak approved these changes Feb 8, 2017

View reviewed changes

gkalpak reviewed Feb 8, 2017

View reviewed changes

fix(select): add attribute "selected" for select[multiple]

0684b39

This helps screen readers identify the selected options, see angular#14419

Narretz force-pushed the fix-select-multiple-ie-15675 branch from dcc9b5e to 0684b39 Compare February 8, 2017 16:19

Narretz merged commit 538f460 into angular:master Feb 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(select): keep original selection when using shift to add options … #15676

fix(select): keep original selection when using shift to add options … #15676

Narretz commented Feb 3, 2017

jbedard Feb 3, 2017

Narretz Feb 3, 2017

gkalpak Feb 3, 2017

gkalpak left a comment

gkalpak Feb 3, 2017

gkalpak Feb 3, 2017

Narretz commented Feb 5, 2017

gkalpak commented Feb 5, 2017

Narretz commented Feb 6, 2017

gkalpak left a comment

gkalpak Feb 7, 2017

gkalpak Feb 7, 2017

gkalpak Feb 8, 2017

gkalpak Feb 8, 2017

Narretz Feb 8, 2017

fix(select): keep original selection when using shift to add options … #15676

fix(select): keep original selection when using shift to add options … #15676

Conversation

Narretz commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Feb 5, 2017

gkalpak commented Feb 5, 2017

Narretz commented Feb 6, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment