Bug 137553 – abandoned select option is reselected when shift selecting new options

• Component: Forms
• Status: RESOLVED FIXED

[Brian Astrup Mikkelsen]

When using the keyboard to select a new option in a select element, and afterwards shift selecting additional elements. The the selection that you jumped from with the keyboard shortcut is reselected.

I have only been able to reproduce this issue in webkit browsers
Affected browsers: Google Chrome 37, Chromium 37, Safari 7.1

Steps to reproduce:
1. Create html document containing a <select multiple size=10> element with 10 options (eg. a - i), or go to http://jsbin.com/satezijosoze/1/
2. Select option with mouse (eg. c)
   Selection is now [c]
3. Press keyboard key to jump to another option (eg. h)
   Selection is now [h]
4. Hold shift and select another option (eg. shift + arrow down)
5. Observe: selection is [c,h,i]
6. Expected selection was [h,i]

[Alexey Proskuryakov]

> 3. Press keyboard key to jump to another option (eg. h)

Namely, press "h", not an arrow key.

Do you happen to know when this started, did it affect Safari 7.0.x? I'm wondering if this was caused by <http://trac.webkit.org/r161558>.

[Brian Astrup Mikkelsen]

No, I just discovered it yesterday and were able to reproduce in newest chrome and safari

[Pascal Jacquemart]

Indeed, I can observe the same behaviour on EFL
Although http://trac.webkit.org/r161558 is closely related, I believe this issue was already there before

This is crazy to see that even though there are 17 layout tests for such a small feature we are still able to find some bugs. Well spotted!

Should be an easy fix

[Brian Astrup Mikkelsen]

I just discovered that chrome has forked webkit to blink, so I have just submitted an issue on their bugtracker as well.

Let the race begin ;-)
https://code.google.com/p/chromium/issues/detail?id=422344

[Pascal Jacquemart]

Created attachment 239863 [details]
Fix proposal + New layout test for debugging

Naive / quick fix
Expecting some regressions...

[Build Bot]

Comment on attachment 239863 [details]
Fix proposal + New layout test for debugging

Attachment 239863 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5104884068122624

New failing tests:
fast/dom/HTMLSelectElement/selected-index-preserved-when-option-text-changes.html
fast/forms/select/option-selecting.html
fast/forms/select/setting-to-invalid-value.html
fast/dom/HTMLSelectElement/selected-false.html

[Build Bot]

Created attachment 239865 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239863 [details]
Fix proposal + New layout test for debugging

Attachment 239863 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4632006021349376

New failing tests:
fast/dom/HTMLSelectElement/selected-index-preserved-when-option-text-changes.html
fast/forms/select/option-selecting.html
fast/forms/select/setting-to-invalid-value.html
fast/dom/HTMLSelectElement/selected-false.html

[Build Bot]

Created attachment 239866 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Pascal Jacquemart]

Created attachment 239868 [details]
Ready for review

[Pascal Jacquemart]

Comment on attachment 239868 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=239868&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:880
>  

Deselect should be called first, otherwise the active selection wrongly records previously selected states

> Source/WebCore/html/HTMLSelectElement.cpp:884
> +    if (is<HTMLOptionElement>(element)) {

At this stage "element" could be null (if listIndex was -1)
It takes benefit of recent change from Chris Dumez: is<>(T*) is now doing a null check on the pointer argument

[Chris Dumez]

Comment on attachment 239868 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=239868&action=review

> LayoutTests/ChangeLog:7
> +

Please add a description of the problem and how you are fixing it.

> LayoutTests/ChangeLog:8
> +        * fast/forms/listbox-selection-after-typeahead-expected.txt: Added.

The usual format is Test: ...
Please refer to other Changelogs.

> LayoutTests/ChangeLog:9
> +        * fast/forms/listbox-selection-after-typeahead.html: Added.

Shouldn't this test be in fast/dom/HTMLSelectElement/ ?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:7
> +<p id="description"></p>

Not needed.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:8
> +<div id="console"></div>

Not needed.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:10
> +description('<p>Test for <i>'

No need to specify the bug URL here.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:15
> +var parent = document.createElement('div');

Do we need to add those from JS? Can't be simply have them hardcoded in the <body>?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:28
> +var sl1 = document.getElementById('sl1');

Could we use better names for the variables and ids please :)

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:29
> +var itemHeight = Math.floor(sl1.offsetHeight / sl1.size);

s11.options.length? Not sure why you're using a size attribute.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:32
> +sl1.setAttribute('style', 'height: ' + height + 'px; border: 10px solid; padding: 5px;');

Why is this needed?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:34
> +function mouseDownOnSelect(selId, index)

No abbreviations please

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:36
> +    var sl = document.getElementById(selId);

Please use a better name than s1

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:55
> +    for (var i = 0; i < select.options.length; i++)

++i

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:62
> +shouldBe('selectionPattern("sl1")', '"0100000"');

shouldBeEqualToString()

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:66
> +shouldBe('selectionPattern("sl1")', '"0000010"');

shouldBeEqualToString()

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:70
> +shouldBe('selectionPattern("sl1")', '"0000110"');

shouldBeEqualToString()

[Ryosuke Niwa]

Comment on attachment 239868 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=239868&action=review

r- because I'd like to see another iteration.

> LayoutTests/fast/forms/listbox-selection-after-typeahead-expected.txt:9
> +PASS selectionPattern("sl1") is "0100000"
> +PASS selectionPattern("sl1") is "0000010"
> +PASS selectionPattern("sl1") is "0000110"
> +PASS successfullyParsed is true

These outputs don't tell us anything about what we're testing and why the result is correct.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:24
> +parent.innerHTML = '<select id="sl1" multiple size=7>'
> +    + '<option>White</option>'
> +    + '<option>Silver</option>'
> +    + '<option>Gray</option>'
> +    + '<option>Black</option>'
> +    + '<option>Red</option>'
> +    + '<option>Maroon</option>'
> +    + '<option>Yellow</option>'
> +    + '</select>'

This doesn't need to be added by innerHTML.  Also don't use abbreviated names such as sl1.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:42
> +    var event = document.createEvent("MouseEvent");
> +    event.initMouseEvent("mousedown", true, true, document.defaultView, 1, sl.offsetLeft +  borderPaddingLeft, sl.offsetTop + y, sl.offsetLeft + borderPaddingLeft, sl.offsetTop + y, false, false, false, false, 0, document);
> +    sl.dispatchEvent(event);

Since we're already relying on eventSender for keyDown, why don't we just use eventSender for mouse click as well?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:51
> +function selectionPattern(selectId)

I didn't understand what "selection pattern" meant until I read this code.
I would call it selectedStates instead.

>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:62
>> +mouseDownOnSelect("sl1", 1);
>> +shouldBe('selectionPattern("sl1")', '"0100000"');
> 
> shouldBeEqualToString()

You should put mouseDownOnSelect("sl1", 1); into shouldBeEqualToString so that it appears in the expected result.

>> Source/WebCore/html/HTMLSelectElement.cpp:880
>>  
> 
> Deselect should be called first, otherwise the active selection wrongly records previously selected states

I think this code deserves a why comment saying that setActiveSelectionAnchorIndex expects selectedState to be up-to-date.
Alternatively, we should rename setActiveSelectionAnchorIndex to setActiveSelectionAnchorIndexCachingSelectedStates to make the code self explanatory.

[Pascal Jacquemart]

Comment on attachment 239868 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=239868&action=review

>> LayoutTests/ChangeLog:9
>> +        * fast/forms/listbox-selection-after-typeahead.html: Added.
> 
> Shouldn't this test be in fast/dom/HTMLSelectElement/ ?

I believe fast/dom/HTMLSelectElement is more for DOM related unit tests
There is also fast/forms/select containing advanced tests which could be a good candidate
But most tests involving complex user interactions are in fast/forms...

>> LayoutTests/fast/forms/listbox-selection-after-typeahead-expected.txt:9
>> +PASS successfullyParsed is true
> 
> These outputs don't tell us anything about what we're testing and why the result is correct.

Right

>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:10
>> +description('<p>Test for <i>'
> 
> No need to specify the bug URL here.

I would stick with the bug URL + Title as explained here: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Understandableclear

>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:32
>> +sl1.setAttribute('style', 'height: ' + height + 'px; border: 10px solid; padding: 5px;');
> 
> Why is this needed?

CSS style influence the mouse coordinates computation to click on a specific option

>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:51
>> +function selectionPattern(selectId)
> 
> I didn't understand what "selection pattern" meant until I read this code.
> I would call it selectedStates instead.

I agree but in the end the test output would be odd (e.g.: PASS selectedStates("sl1") is "0100000") ?

>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:55
>> +    for (var i = 0; i < select.options.length; i++)
> 
> ++i

Is it just a coding style? Would it make any in difference as per complex C++ iterators?

>>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:62
>>> +shouldBe('selectionPattern("sl1")', '"0100000"');
>> 
>> shouldBeEqualToString()
> 
> You should put mouseDownOnSelect("sl1", 1); into shouldBeEqualToString so that it appears in the expected result.

What about using evalAndLog() instead? 
I would prefer if all selectionPatterns remain aligned

>>> Source/WebCore/html/HTMLSelectElement.cpp:880
>>>  
>> 
>> Deselect should be called first, otherwise the active selection wrongly records previously selected states
> 
> I think this code deserves a why comment saying that setActiveSelectionAnchorIndex expects selectedState to be up-to-date.
> Alternatively, we should rename setActiveSelectionAnchorIndex to setActiveSelectionAnchorIndexCachingSelectedStates to make the code self explanatory.

There is already such comment inside setActiveSelectionAnchorIndex() to explain how active selection records the current selected states
Let's says user hold CTRL / Meta key while clicking to select few items then start dragging the mouse (still holding CTRL / Meta key). It should either complete or inverse previous selection.
In case selectOption() is called with the DeselectOtherOptions flag, it seems logical to clear previous selection before starting a new one

[Pascal Jacquemart]

Created attachment 239977 [details]
After review

The layout test is shortened and improved

[Chris Dumez]

Comment on attachment 239868 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=239868&action=review

>>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:10
>>> +description('<p>Test for <i>'
>> 
>> No need to specify the bug URL here.
> 
> I would stick with the bug URL + Title as explained here: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Understandableclear

I don't think that wiki is up to date... it still recommends using testRunner's waitUntilDone() and notifyDone().

>>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:55
>>> +    for (var i = 0; i < select.options.length; i++)
>> 
>> ++i
> 
> Is it just a coding style? Would it make any in difference as per complex C++ iterators?

Yes, coding style in both JS and C++. No performance difference for builtin types.

[Chris Dumez]

Comment on attachment 239868 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=239868&action=review

>>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:10
>>> +description('<p>Test for <i>'
>> 
>> No need to specify the bug URL here.
> 
> I would stick with the bug URL + Title as explained here: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Understandableclear

I don't think that wiki is up to date... it still recommends using testRunner's waitUntilDone() and notifyDone().

>>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:55
>>> +    for (var i = 0; i < select.options.length; i++)
>> 
>> ++i
> 
> Is it just a coding style? Would it make any in difference as per complex C++ iterators?

Yes, coding style in both JS and C++. No performance difference for builtin types.

[Pascal Jacquemart]

I have to say this part of the code involving form elements turned out to be quite hard to debug. I actually had to use two different machines (then two keyboards) because otherwise while reaching a breakpoint in gdb, the debugger captures the focus and drop selected options from MiniBrowser?!!!
Weird, it might be an EFL bug because I don't observe the same behavior in Safari

Something else. I just observed on Firefox a user can hold the shift key while typing letters for typeAhead search and it would select the whole range. We don't handle this in Webkit

[Ryosuke Niwa]

Comment on attachment 239868 [details]
Ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=239868&action=review

>>>> Source/WebCore/html/HTMLSelectElement.cpp:880
>>>>  
>>> 
>>> Deselect should be called first, otherwise the active selection wrongly records previously selected states
>> 
>> I think this code deserves a why comment saying that setActiveSelectionAnchorIndex expects selectedState to be up-to-date.
>> Alternatively, we should rename setActiveSelectionAnchorIndex to setActiveSelectionAnchorIndexCachingSelectedStates to make the code self explanatory.
> 
> There is already such comment inside setActiveSelectionAnchorIndex() to explain how active selection records the current selected states
> Let's says user hold CTRL / Meta key while clicking to select few items then start dragging the mouse (still holding CTRL / Meta key). It should either complete or inverse previous selection.
> In case selectOption() is called with the DeselectOtherOptions flag, it seems logical to clear previous selection before starting a new one

The problem is that such a comment isn't referenced here.
How does a reader that he/she has to read that comment in order to understand the ordering here?

[Ryosuke Niwa]

Comment on attachment 239977 [details]
After review

View in context: https://bugs.webkit.org/attachment.cgi?id=239977&action=review

r- because there are still some issues in the test and my previous comment wasn't addressed in the new patch.

> LayoutTests/fast/forms/listbox-selection-after-typeahead-expected.txt:8
> +mouseDownOnSelect(1)
> +PASS selectionPattern() is "0100000"

We should put these lines into the same line as in:
mouseDownOnSelect(1); selectionPattern()

> LayoutTests/fast/forms/listbox-selection-after-typeahead-expected.txt:10
> +keyDownOnSelect("M")
> +PASS selectionPattern() is "0000010"

It's not obvious to me why selecting "M" would result in 5th element being selected.
Why don't we rename the value so that we can call keyDownOnSelect("5")?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:10
> +    select {
> +        border: none;
> +        margin: 0px;
> +        padding: 0px;
> +    }

What's the point of this style? Is this required?
Can't we specify in the inline style attribute?

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:26
> +function mouseDownOnSelect(index)

I don't think this function name makes sense.
What we're doing here is selecting an option by a mouse down.
I would call this mouseDownAtOption(index)

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:35
> +function keyDownOnSelect(identifier, modifier) {

Again, I don't think keyDownOnSelect is a descriptive name for this function.
How about sendKeyDownAfterFocusingSelect?
But do we really need to focus select element each time?
It seems like the focus should be on the select element after the first test case.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:41
> +function selectionPattern()

Once again, I don't understand what selectionPattern means.
Please use a more descriptive function name such as bitPatternForSelectedOptions.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:51
> +    + '<a href="https://bugs.webkit.org/show_bug.cgi?id=137553">https://bugs.webkit.org/show_bug.cgi?id=137553</a></br>'
> +    + '<i>Abandoned select option is reselected when shift selecting new options</i>.</p>');

Like Chris pointed out, we shouldn't be linkifying the bug inside here.
It's almost never useful to mention that bug this test is for since that's recoded in the change log
which is available through svn commit log and on trac.webkit.org.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:53
> +// Select 2nd option

Please remove this comment. This is "what" comment we avoid in WebKit. Only add "why" comment.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:55
> +evalAndLog('mouseDownOnSelect(1)');
> +shouldBeEqualToString('selectionPattern()', "0100000");

Combine these two lines as in:
shouldBeEqualToString('mouseDownOnSelect(1); selectionPattern()', "0100000");

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:57
> +// Typeahead to select the "Maroon" options

Ditto about removing the comment.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:59
> +evalAndLog('keyDownOnSelect("M")');
> +shouldBeEqualToString('selectionPattern()', "0000010");

Ditto about merging these two lines.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:61
> +// Start a multiple selection

Ditto about removing the comment.

> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:63
> +evalAndLog('keyDownOnSelect("upArrow", "shiftKey")');
> +shouldBeEqualToString('selectionPattern()', "0000110");

Ditto about merging these two lines.

[Pascal Jacquemart]

Comment on attachment 239977 [details]
After review

View in context: https://bugs.webkit.org/attachment.cgi?id=239977&action=review

>> LayoutTests/fast/forms/listbox-selection-after-typeahead-expected.txt:10
>> +PASS selectionPattern() is "0000010"
> 
> It's not obvious to me why selecting "M" would result in 5th element being selected.
> Why don't we rename the value so that we can call keyDownOnSelect("5")?

Even by renaming the options, it won't be obvious unless you open the test itself

>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:10
>> +    }
> 
> What's the point of this style? Is this required?
> Can't we specify in the inline style attribute?

This style removing border, margin and padding is used to simplify the mouse coordinates computation to click on a specific option
In-lining the style, sure.

>> LayoutTests/fast/forms/listbox-selection-after-typeahead.html:35
>> +function keyDownOnSelect(identifier, modifier) {
> 
> Again, I don't think keyDownOnSelect is a descriptive name for this function.
> How about sendKeyDownAfterFocusingSelect?
> But do we really need to focus select element each time?
> It seems like the focus should be on the select element after the first test case.

It was for debugging purpose, I think few other tests apply the same strategy
I understand it sound redundant. This was to cope with the debugger taking the focus while debugging
As you expected it is working without calling select.focus() now

[Pascal Jacquemart]

Created attachment 240123 [details]
After second review

The layout test is made even simpler with self explaining functions name

[Pascal Jacquemart]

Ryosuke, Chris, any chance to have a review on this?

[WebKit Commit Bot]

Comment on attachment 240123 [details]
After second review

Clearing flags on attachment: 240123

Committed r175263: <http://trac.webkit.org/changeset/175263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.