[Webkit-unassigned] [Bug 137553] abandoned select option is reselected when shift selecting new options

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137553

--- Comment #14 from Pascal Jacquemart <p.jacquemart at samsung.com> ---
Comment on attachment 239868
  --> https://bugs.webkit.org/attachment.cgi?id=239868
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141016/7301993f/attachment-0002.html>