Bug 52436

Summary: Setting "selected" attribute to false should have no effect in single line <select> (affects jQuery)
Product: WebKit Reporter: Christian <christian.lange.81>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, eae
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
reduced test case
none
Patch
none
Patch none

Description Christian 2011-01-14 01:56:59 PST
Hi

(I am using Ubuntu 10.04)
I got a <select> element with three <option> inside (my page ist XHTML 1.0 Transitional), looks like this:

<select>
  <option value="0">0</option>
  <option value="1">1</option>
  <option value="2">2</option>
</select>

None of those is selected manually (i.e. has the "selected='selected'" tag) but on first loading the page, the option "0" ist shown by default. When I use jQuery to delete all "selected" tags in all <option>-element (which are not there), suddenly no option is selected anymore. You can check it yourself with following (w3c validated) little code:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="de" lang="de">
<head>
    <title>test</title>
    <meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
    <script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.4/jquery.min.js"></script>
</head>
<body>
    <select id="s" size="1">
        <option value="0">0</option>
        <option value="1">1</option>
        <option value="2">2</option>
    </select>
    <input id="i" type="button" value="Remove 'selected'"/>
    <script type="text/javascript">
    $(document).ready(function() {
        $("#i").click(function() {
            $("#s").find("option").removeAttr("selected");
        });
    });
    </script>
</body>
</html>

This disappearing of a selected value happens in Google Chrome 8.0.552.237, Chromium 10.0.639.0 (71411) as well as Safari 5.0.3 (7533.19.4). You see, pretty new browsers. According to webkit.org the webkit version of Chromium is 534.16 but the newest webkit version is r75294. So I can't really tell if I got the newest one, it's a confusing name scheme.
This behaviour, however, does not appear in Firefox 3.6.13, Opera 11.00 or Internet Explorer 8.

I think that the way Firefox, Opera and IE handle this is the correct way. Could you please check if this is a bug in WebKit?

Best regards,
Christian
Comment 1 Alexey Proskuryakov 2011-01-14 10:54:17 PST
Created attachment 78962 [details]
reduced test case
Comment 2 Alexey Proskuryakov 2011-01-14 11:02:22 PST
This is not really about removing the "selected" attribute - behind the scenes, jQuery first sets it to false.

I'm not sure if WebKit is actually broken here, but we should investigate this as a difference with Firefox. Please consider also filing a bug against jQuery, as there is no apparent reason for removeAttr() to behave this way.


removeAttr: function( name, fn ) {
    return this.each(function(){
        jQuery.attr( this, name, "" ); // <-- this sets .selected to false in your test case
        if ( this.nodeType === 1 ) {
            this.removeAttribute( name );
        }
    });
},
Comment 3 Christian 2011-01-15 00:21:57 PST
I filed this bug at jQuery today: http://bugs.jquery.com/ticket/7981
Comment 4 Emil A Eklund 2011-01-28 14:36:30 PST
Created attachment 80498 [details]
Patch

While the original specification doesn't specify a behavior for this case the HTML5 [1] draft does. This patch changes the behavior to match the HTML5 draft specification and to match the Mozilla behavior. IE selects the last selectable element rather than the first but apart from that exhibits the same behavior.

1: http://dev.w3.org/html5/spec/Overview.html#the-select-element
Comment 5 Alexey Proskuryakov 2011-01-28 15:06:19 PST
Could you please quote the relevant bits from HTML5?
Comment 6 Darin Adler 2011-01-28 15:17:37 PST
Comment on attachment 80498 [details]
Patch

I’d like to see a test that covers trying to set the selected index to -2.

For future reference, you can sometimes make better tests by putting more of the test into the shouldBe function:

    shouldBe("selectElement.selectedIndex = 0; selectElement.selectedIndex", "0");
Comment 7 Emil A Eklund 2011-01-28 15:20:09 PST
"If the multiple attribute is absent and the element's display size is 1, then whenever there are no option elements in the select element's list of options that have their selectedness set to true, the user agent must set the selectedness of the first option element in the list of options in tree order that is not disabled, if any, to true."
Comment 8 Emil A Eklund 2011-01-28 15:41:14 PST
Created attachment 80513 [details]
Patch

Thanks Darin. Added a test for -2 and used shouldBe as suggested.
Comment 9 Darin Adler 2011-01-31 14:48:49 PST
Comment on attachment 80513 [details]
Patch

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.
Comment 10 Eric Seidel (no email) 2011-01-31 16:06:58 PST
Comment on attachment 80498 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 80498 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 11 WebKit Commit Bot 2011-01-31 19:45:57 PST
Comment on attachment 80513 [details]
Patch

Clearing flags on attachment: 80513

Committed r77206: <http://trac.webkit.org/changeset/77206>