Summary: | [Forms] Default selection of select(menulist) should not be a disabled option | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||||||
Component: | Forms | Assignee: | yosin | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, jonlee, tkent, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
URL: | http://jsfiddle.net/FzfL3/ | ||||||||||||||||||||
Attachments: |
|
Description
yosin
2011-12-12 01:22:23 PST
Created attachment 118936 [details]
Patch
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 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. 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
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
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
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 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
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
Comment on attachment 119150 [details]
Patch 8
ok
Comment on attachment 119150 [details] Patch 8 Clearing flags on attachment: 119150 Committed r102741: <http://trac.webkit.org/changeset/102741> All reviewed patches have been landed. Closing bug. 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? (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. |