Bug 74270 - [Forms] Default selection of select(menulist) should not be a disabled option
Summary: [Forms] Default selection of select(menulist) should not be a disabled option
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL: http://jsfiddle.net/FzfL3/
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-12 01:22 PST by yosin
Modified: 2012-05-15 18:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.30 KB, patch)
2011-12-12 19:37 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (4.16 KB, patch)
2011-12-12 22:00 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 3 - Add selected/disabled test (4.54 KB, patch)
2011-12-12 23:14 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 4 - Update selection logic (4.67 KB, patch)
2011-12-13 01:10 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 5 (9.44 KB, patch)
2011-12-13 02:44 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 6 (10.81 KB, patch)
2011-12-13 20:11 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 7 (10.81 KB, patch)
2011-12-13 20:29 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 8 (10.04 KB, patch)
2011-12-13 20:57 PST, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2011-12-12 01:22:23 PST
== 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>
Comment 1 yosin 2011-12-12 19:37:13 PST
Created attachment 118936 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-12 21:09:44 PST
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
Comment 3 Kent Tamura 2011-12-12 21:19:19 PST
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>?
Comment 4 yosin 2011-12-12 22:00:33 PST
Created attachment 118953 [details]
Patch 2
Comment 5 yosin 2011-12-12 22:04:44 PST
In FF, IE and OP, "selected" attribute isn't turned off by "disabled" attribute.
In other words, "disabled" state doesn't affect "selected" state.
Comment 6 Kent Tamura 2011-12-12 22:13:19 PST
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>.
Comment 7 yosin 2011-12-12 23:14:17 PST
Created attachment 118959 [details]
Patch 3 - Add selected/disabled test
Comment 8 WebKit Review Bot 2011-12-13 00:48:53 PST
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
Comment 9 yosin 2011-12-13 01:10:54 PST
Created attachment 118972 [details]
Patch 4 - Update selection logic
Comment 10 yosin 2011-12-13 02:44:19 PST
Created attachment 118985 [details]
Patch 5
Comment 11 Kent Tamura 2011-12-13 17:09:14 PST
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.
Comment 12 yosin 2011-12-13 18:14:15 PST
>> 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.
Comment 13 yosin 2011-12-13 20:11:24 PST
Created attachment 119142 [details]
Patch 6
Comment 14 Kent Tamura 2011-12-13 20:24:25 PST
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.
> +
> +2011-12-13  Yosifumi Inoue  <yosin@chromium.org>
> +
>          RenderTheme should have a function for disabled text color adjustment

This contains two new ChangeLog entries.
Comment 15 Kent Tamura 2011-12-13 20:25:53 PST
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"?
Comment 16 yosin 2011-12-13 20:29:13 PST
Created attachment 119146 [details]
Patch 7
Comment 17 Kent Tamura 2011-12-13 20:52:03 PST
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.
Comment 18 yosin 2011-12-13 20:57:51 PST
Created attachment 119150 [details]
Patch 8
Comment 19 Kent Tamura 2011-12-13 21:26:19 PST
Comment on attachment 119150 [details]
Patch 8

ok
Comment 20 WebKit Review Bot 2011-12-13 22:15:05 PST
Comment on attachment 119150 [details]
Patch 8

Clearing flags on attachment: 119150

Committed r102741: <http://trac.webkit.org/changeset/102741>
Comment 21 WebKit Review Bot 2011-12-13 22:15:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Jon Lee 2012-05-15 11:34:44 PDT
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?
Comment 23 Kent Tamura 2012-05-15 18:08:26 PDT
(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.