[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


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #239868|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org>  2014-10-15 14:04:18 PST ---
(From update of attachment 239868)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.