[webkit-reviews] review denied: [Bug 137553] abandoned select option is reselected when shift selecting new options : [Attachment 239868] Ready for review

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Pascal Jacquemart
<p.jacquemart at samsung.com>'s request for review:
Bug 137553: abandoned select option is reselected when shift selecting new
options
https://bugs.webkit.org/show_bug.cgi?id=137553

Attachment 239868: Ready for review
https://bugs.webkit.org/attachment.cgi?id=239868&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.