[Webkit-unassigned] [Bug 52436] Setting "selected" attribute to false should have no effect in single line <select> (affects jQuery)

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #9 from Darin Adler <darin at apple.com>  2011-01-31 14:48:49 PST ---
(From update of attachment 80513)
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.

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