Bug 74270 – [Forms] Default selection of select(menulist) should not be a disabled option

• Component: Forms
• Status: RESOLVED FIXED

== Steps ==
1. Visit http://jsfiddle.net/FzfL3/
2. Click [Run]

== Result ==
"One" is displayed

== Expect ==
"Three" is displayed

== Note ==
This bug is import from http://crbug.com/100997

== Sample HTML ==
<select name="foo">
<option value="1" disabled>One</option>
<option value="2" disabled>Two</option>
<option value="3">Three</option>
<option value="4">Four</option>
</select>

Created attachment 118936 [details]
Patch

[WebKit Review Bot]

Comment on attachment 118936 [details]
Patch

Attachment 118936 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10845210

New failing tests:
fast/forms/select-disabled-appearance.html
fast/forms/ValidityState-valueMissing-001.html

[Kent Tamura]

Comment on attachment 118936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118936&action=review

r- because of EWS failures.

> Source/WebCore/html/HTMLSelectElement.cpp:719
> +                if (option->disabled())
> +                    option->setSelectedState(false);

What's the behavior of other browsers for <option disabled selected>?

Created attachment 118953 [details]
Patch 2

In FF, IE and OP, "selected" attribute isn't turned off by "disabled" attribute.
In other words, "disabled" state doesn't affect "selected" state.

[Kent Tamura]

Comment on attachment 118953 [details]
Patch 2

(In reply to comment #5)
> In FF, IE and OP, "selected" attribute isn't turned off by "disabled" attribute.
> In other words, "disabled" state doesn't affect "selected" state.

ok.  We should have a test case for <option disabled selected>.

Created attachment 118959 [details]
Patch 3 - Add selected/disabled test

[WebKit Review Bot]

Comment on attachment 118959 [details]
Patch 3 - Add selected/disabled test

Attachment 118959 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10844257

New failing tests:
fast/forms/select-disabled-appearance.html

Created attachment 118972 [details]
Patch 4 - Update selection logic

Created attachment 118985 [details]
Patch 5

[Kent Tamura]

Comment on attachment 118985 [details]
Patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=118985&action=review

> Source/WebCore/ChangeLog:3
> +        [Forms] Disabled option selected in form

This is not a great summary.  It should contain a phrase like "initial selection", "default selection", or something.

> Source/WebCore/html/HTMLSelectElement.cpp:745
> +    if (!foundSelected && m_size <= 1 && firstOption && !firstOption->selected())
> +        firstOption->setSelectedState(true);

Do we have a test covering this behavior?

> LayoutTests/ChangeLog:14
> +        * platform/chromium/test_expectations.txt: Add fast/forms/basic-selects.html
> +        * platform/gtk/test_expectations.txt: Add fast/forms/basic-selects.html
> +        * platform/mac/test_expectations.txt: Add fast/forms/basic-selects.html
> +        * platform/qt/test_expectations.txt: Add fast/forms/basic-selects.html
> +        * platform/win/test_expectations.txt: Add fast/forms/basic-selects.html

Please add explanation why basic-selects.html will fail.

>> Source/WebCore/html/HTMLSelectElement.cpp:745
>> +    if (!foundSelected && m_size <= 1 && firstOption && !firstOption->>selected())
>> +        firstOption->setSelectedState(true);
>Do we have a test covering this behavior?

Yes,  fast/forms/select-disabled-appearance.html checks this.
Actually, I found this by failure of this test.

Created attachment 119142 [details]
Patch 6

[Kent Tamura]

Comment on attachment 119142 [details]
Patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=119142&action=review

> Source/WebCore/ChangeLog:616
>  
> +        [Forms] Disabled option selected in form
> +        https://bugs.webkit.org/show_bug.cgi?id=74270
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Ignore and deselect disabled option element during computing
> +        default select option.
> +
> +        Tests: fast/forms/select/menulist-disabled-option-expected.html
> +               fast/forms/select/menulist-disabled-option.html
> +
> +        * html/HTMLSelectElement.cpp:
> +        (WebCore::HTMLSelectElement::recalcListItems): Checks option element is disabled or not.
> +
> [phone]  Yosifumi Inoue  <[e-mail]>
> +
>          RenderTheme should have a function for disabled text color adjustment

This contains two new ChangeLog entries.

[Kent Tamura]

Comment on attachment 119142 [details]
Patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=119142&action=review

> Source/WebCore/ChangeLog:3
> +        [Forms] Default selection of select(menulist) should not be default

Do you mean  "should not be disabled"?

Created attachment 119146 [details]
Patch 7

[Kent Tamura]

Comment on attachment 119146 [details]
Patch 7

Please look at Comment #14.
Make sure that your patch contains unexpected change. "webkit-patch upload" shows you your patch before uploading.

Created attachment 119150 [details]
Patch 8

[Kent Tamura]

Comment on attachment 119150 [details]
Patch 8

ok

[WebKit Review Bot]

Comment on attachment 119150 [details]
Patch 8

Clearing flags on attachment: 119150

Committed r102741: <http://trac.webkit.org/changeset/102741>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

I understand this has been checked in awhile ago, but is there a place in the HTML5 spec that specifies that this is the correct default behavior?

[Kent Tamura]

(In reply to comment #22)
> I understand this has been checked in awhile ago, but is there a place in the HTML5 spec that specifies that this is the correct default behavior?

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-select-element
> 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.