Bug 35056 - Disabled menu options are still submitted.
Summary: Disabled menu options are still submitted.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2010-02-17 13:45 PST by Evan Carroll
Modified: 2010-03-29 01:50 PDT (History)
6 users (show)

See Also:


Attachments
Click the location button, notice the disabled form is submitted (successful) (1.30 KB, application/xhtml+xml)
2010-02-17 13:45 PST, Evan Carroll
no flags Details
Reduction (488 bytes, text/html)
2010-02-17 14:15 PST, Daniel Bates
no flags Details
Patch (5.26 KB, patch)
2010-02-18 01:45 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (5.90 KB, patch)
2010-02-22 19:31 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch rev.3 (Add another test) (8.64 KB, patch)
2010-03-07 17:35 PST, Kent Tamura
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Carroll 2010-02-17 13:45:08 PST
Created attachment 48934 [details]
Click the location button, notice the disabled form is submitted (successful)

Background
--------

The `disabled` attribute on form controls is described here: http://www.w3.org/TR/html401/interact/forms.html#h-17.12.1
For reference here is a snippet of the contents:

17.12.1 Disabled controls
Disabled controls do not receive focus.
Disabled controls are skipped in tabbing navigation.
Disabled controls cannot be successful. ## violation

Successful is defined here: http://www.w3.org/TR/html401/interact/forms.html#successful-controls
For reference here is a snipped of the contents:

A successful control is "valid" for submission. Every successful control has its control name paired with its current value as part of the submitted form data set. A successful control must be defined within a FORM element and must have a control name.

Bug
------

Currently, in webkit, a disabled <option> will still be submitted on the form. Firefox behaves properly.
Comment 1 Daniel Bates 2010-02-17 14:09:47 PST
Confirmed in Mac WebKit Nightly r54843.
Comment 2 Daniel Bates 2010-02-17 14:15:52 PST
Created attachment 48937 [details]
Reduction

Simplified version of the attachment <https://bugs.webkit.org/attachment.cgi?id=48934>.

The issue occurs when a <select> contains exactly one <option>, call it A, and A is disabled. Unless otherwise specified, WebKit selects the first <option> element regardless of whether it is disabled.
Comment 3 Kent Tamura 2010-02-17 22:15:01 PST
Other browsers' behaviors:

Firefox 3.6:
Select nothing initially, and doesn't send the disabled option

IE8:
Select the disabled option initially, and doesn't send the disabled option

Opera10:
Select nothing initially, and DID send the disabled option
Comment 4 Kent Tamura 2010-02-18 01:45:43 PST
Created attachment 48986 [details]
Patch
Comment 5 Darin Adler 2010-02-18 13:43:42 PST
I see the HTML4 spec snippet. But does HTML5 mention this? Does it specify one of the behaviors seen in the other browsers?
Comment 6 Daniel Bates 2010-02-18 14:47:39 PST
(In reply to comment #5)
> I see the HTML4 spec snippet. But does HTML5 mention this? Does it specify one
> of the behaviors seen in the other browsers?

Yes, by step 7(6) of section 4.10.21.3 <http://dev.w3.org/html5/spec/Overview.html#form-submission-algorithm> (*) and by the definition of selectedness of an HTML option element <http://dev.w3.org/html5/spec/Overview.html#concept-option-selectedness> (**), re-printed here for convenience:

(*) If the field element is a select element, then for each option element in the select element whose selectedness is true, append an entry in the form data set with the name as the name, the value of the option element as the value, and type as the type. 

(**) ...If the [option] element is disabled, then the element's selectedness is always false and cannot be set to true...

So, both Firefox and IE follow from this behavior.
Comment 7 Eric Seidel (no email) 2010-02-22 13:55:17 PST
Comment on attachment 48986 [details]
Patch

I don't think this test needs/wants wait until done:

 if (window.layoutTestController)
 38     layoutTestController.waitUntilDone();
 39 window.onload = startOrVerify;

Also, i'm surprised this isn't just a script test.  This would be easy to do with a little .innerHTML action.
Comment 8 Kent Tamura 2010-02-22 19:18:23 PST
(In reply to comment #7)
> (From update of attachment 48986 [details])
> I don't think this test needs/wants wait until done:
> 
>  if (window.layoutTestController)
>  38     layoutTestController.waitUntilDone();
>  39 window.onload = startOrVerify;

If I don't use waitUntilDone()/notifyDone(), the page before submit() is dumped. I don't know the reason.

> Also, i'm surprised this isn't just a script test.  This would be easy to do
> with a little .innerHTML action.

Right.  I'll update the test.
Comment 9 Kent Tamura 2010-02-22 19:31:55 PST
Created attachment 49260 [details]
Proposed patch (rev.2)
Comment 10 Eric Seidel (no email) 2010-03-05 14:47:50 PST
Comment on attachment 49260 [details]
Proposed patch (rev.2)

Shouldn't we have a negative test for the case you're removing?
Comment 11 Kent Tamura 2010-03-07 17:35:04 PST
Created attachment 50182 [details]
Proposed patch rev.3 (Add another test)
Comment 12 Kent Tamura 2010-03-07 17:38:01 PST
(In reply to comment #10)
> (From update of attachment 49260 [details])
> Shouldn't we have a negative test for the case you're removing?

Ok, I have added another test.  It covers the code path which the patch will remove, it fails with unpatched WebKit and succeeeds with the patched WebKit.
Comment 13 Eric Seidel (no email) 2010-03-15 15:48:02 PDT
Comment on attachment 50182 [details]
Proposed patch rev.3 (Add another test)

LGTM.
Comment 14 Kent Tamura 2010-03-15 22:57:34 PDT
Landed as r56041.
Comment 15 Gyuyoung Kim 2010-03-29 01:50:34 PDT
Even though this defect was resolved, I wonder if I can attach a comment in here. There is a build break when wml is enabled as below,

------------------------------------------------------------------------------
DerivedSources/WMLElementFactory.cpp: In function ‘WTF::PassRefPtr<WebCore::WMLElement> WebCore::optionConstructor(const WebCore::QualifiedName&, WebCore::Document*, bool)’:
DerivedSources/WMLElementFactory.cpp:154: error: cannot allocate an object of abstract type ‘WebCore::WMLOptionElement’
./WebCore/wml/WMLOptionElement.h:31: note:   because the following virtual functions are pure within ‘WebCore::WMLOptionElement’:
./WebCore/dom/OptionElement.h:37: note: 	virtual bool WebCore::OptionElement::disabled() const
DerivedSources/WMLElementFactory.cpp: In function ‘WTF::PassRefPtr<WebCore::WMLElement> WebCore::selectConstructor(const WebCore::QualifiedName&, WebCore::Document*, bool)’:
DerivedSources/WMLElementFactory.cpp:179: error: cannot allocate an object of abstract type ‘WebCore::WMLSelectElement’
./WebCore/wml/WMLSelectElement.h:30: note:   because the following virtual functions are pure within ‘WebCore::WMLSelectElement’:
./WebCore/dom/SelectElement.h:64: note: 	virtual void WebCore::SelectElement::listBoxSelectItem(int, bool, bool, bool)
------------------------------------------------------------------------------

I made a patch for the break. Please review my patch.

Bug 36698 -  There is a build break due to the disabled() when wml is enabled.
https://bugs.webkit.org/show_bug.cgi?id=36698