Bug 35056 – Disabled menu options are still submitted.

• Component: Forms
• Status: RESOLVED FIXED

[Evan Carroll]

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.

[Daniel Bates]

Confirmed in Mac WebKit Nightly r54843.

[Daniel Bates]

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.

[Kent Tamura]

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

[Kent Tamura]

Created attachment 48986 [details]
Patch

[Darin Adler]

I see the HTML4 spec snippet. But does HTML5 mention this? Does it specify one of the behaviors seen in the other browsers?

[Daniel Bates]

(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.

[Eric Seidel (no email)]

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.

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 49260 [details]
Proposed patch (rev.2)

[Eric Seidel (no email)]

Comment on attachment 49260 [details]
Proposed patch (rev.2)

Shouldn't we have a negative test for the case you're removing?

[Kent Tamura]

Created attachment 50182 [details]
Proposed patch rev.3 (Add another test)

[Kent Tamura]

(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.

[Eric Seidel (no email)]

Comment on attachment 50182 [details]
Proposed patch rev.3 (Add another test)

LGTM.

[Kent Tamura]

Landed as r56041.

[Gyuyoung Kim]

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