[webkit-reviews] review granted: [Bug 52436] Setting "selected" attribute to false should have no effect in single line <select> (affects jQuery) : [Attachment 80513] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Emil A Eklund <eae at chromium.org>'s
request for review:
Bug 52436: Setting "selected" attribute to false should have no effect in
single line <select> (affects jQuery)
https://bugs.webkit.org/show_bug.cgi?id=52436

Attachment 80513: Patch
https://bugs.webkit.org/attachment.cgi?id=80513&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80513&action=review

> Source/WebCore/dom/SelectElement.cpp:325
> -    const Vector<Element*>& items = data.listItems(element);
> -    int listIndex = optionToListIndex(data, element, optionIndex);
> +    if (optionIndex == -1 && !deselect && !data.multiple())
> +	   optionIndex = nextSelectableListIndex(data, element, -1);
>      if (!data.multiple())
>	   deselect = true;
>  
> +    const Vector<Element*>& items = data.listItems(element);
> +    int listIndex = optionToListIndex(data, element, optionIndex);
> +

I didn’t criticize this the first time around, but I realize now that the
setSelectedIndex function is a kind of low level place to put this logic.
Normally I’d expect to see the logic in the callers. Is there a higher level
place in the class we can put this?

I also think the new block of code needs a “Why” comment; it’s a bit mysterious
without one.

Maybe we could follow up this patch with a patch that adds that comment.

Also, I don’t know why you chose to reorder the existing code. If you hadn’t
reordered it, this patch would only show the new code you are adding. But
because you chose to put the deselect = true code above the optionToListIndex
code, there is a larger change here. With no rationale for the change.

> LayoutTests/ChangeLog:8
> +	   Add test for changing the selection in a one-line select element
using\

Stray backslash here.

> LayoutTests/fast/dom/HTMLSelectElement/selected-false.html:30
> +    optionElements[2].selected = true;
> +    optionElements[1].selected = true;
> +    shouldBe("selectElement.selectedIndex", "1");

These could be inside the shouldBe too just as the single statement lines are.
There’s no reason to have this part of the test be hidden in the test output.