Bug 64509 - <input form="x"> should not associate the input with any forms when there is no form with id="x"
: <input form="x"> should not associate the input with any forms when there is ...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Kenichi Ishibashi
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-13 20:54 PDT by Boris Zbarsky
Modified: 2011-07-16 08:08 PDT (History)
6 users (show)

See Also:


Attachments
Testcase showing the problem (153 bytes, text/html)
2011-07-13 20:54 PDT, Boris Zbarsky
no flags Details
Patch V0 (7.80 KB, patch)
2011-07-14 19:43 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V1 (11.59 KB, patch)
2011-07-14 20:29 PDT, Kenichi Ishibashi
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Zbarsky 2011-07-13 20:54:31 PDT
Created attachment 100762 [details]
Testcase showing the problem

BUILD: WebKit trunk, Chrome dev builds

STEPS TO REPRODUCE:
1)  Load attached testcase
2)  Click the submit button

EXPECTED RESULTS: Nothing happens

ACTUAL RESULTS: http://www.webkit.org/?test=test is loaded

ADDITIONAL INFORMATION:  This is poisoning the well for @form: sites are deploying with bogus @form values and breaking in UAs that actually implement the spec.

The relevant spec section is http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#reset-the-form-owner which says in step 3.2 that if there is a "form" attribute on the input then step 4 is never reached.  So both of the inputs in the testcase should not be associated with any form.  It looks like WebKit falls through to step 4 when it doesn't find an element with the given ID.
Comment 1 Boris Zbarsky 2011-07-13 20:56:27 PDT
Note also http://www.w3.org/Bugs/Public/show_bug.cgi?id=12241
Comment 2 Kenichi Ishibashi 2011-07-14 19:43:32 PDT
Created attachment 100917 [details]
Patch V0
Comment 3 Kenichi Ishibashi 2011-07-14 19:46:32 PDT
(In reply to comment #2)
> Created an attachment (id=100917) [details]
> Patch V0

Changed the bug title to workaround the problem that webkit-patch cannot upload patches when the title includes '<form>'.
Comment 4 Kent Tamura 2011-07-14 20:06:18 PDT
Comment on attachment 100917 [details]
Patch V0

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

> LayoutTests/fast/forms/form-attribute-nonexistence-form-id.html:15
> +    document.getElementById('form').submit();

I don't think we need to submit the form to test this issue.
shouldBeNull('getElementByName("input2").form') is enough, right?

> Source/WebCore/html/FormAssociatedElement.cpp:131
> -void FormAssociatedElement::resetFormOwner(HTMLFormElement* form)
> +void FormAssociatedElement::resetFormOwner(HTMLFormElement*)

So, we can remove the HTMLFormElement* argument.
Comment 5 Kent Tamura 2011-07-14 20:07:20 PDT
(In reply to comment #4)
> shouldBeNull('getElementByName("input2").form') is enough, right?

getElementsByName("input2")[0].form
Comment 6 Kenichi Ishibashi 2011-07-14 20:29:09 PDT
Created attachment 100921 [details]
Patch V1
Comment 7 Kenichi Ishibashi 2011-07-14 20:30:50 PDT
Comment on attachment 100917 [details]
Patch V0

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

Hi Kent-san,

Thank you for the prompt review!

>> LayoutTests/fast/forms/form-attribute-nonexistence-form-id.html:15
>> +    document.getElementById('form').submit();
> 
> I don't think we need to submit the form to test this issue.
> shouldBeNull('getElementByName("input2").form') is enough, right?

Certainly. Done.

>> Source/WebCore/html/FormAssociatedElement.cpp:131
>> +void FormAssociatedElement::resetFormOwner(HTMLFormElement*)
> 
> So, we can remove the HTMLFormElement* argument.

Done.
Comment 8 Darin Adler 2011-07-14 20:33:12 PDT
Comment on attachment 100921 [details]
Patch V1

Are the tests here sufficient to cover all the effects of the patch? Could there be websites depending on the old behavior? The part about handling form associated elements that have formAttr seems fine, but I am unclear about why the resetFormOwner changes won’t affect other form associated elements.
Comment 9 Kenichi Ishibashi 2011-07-14 21:07:01 PDT
(In reply to comment #8)
> (From update of attachment 100921 [details])
> Are the tests here sufficient to cover all the effects of the patch? Could there be websites depending on the old behavior? The part about handling form associated elements that have formAttr seems fine, but I am unclear about why the resetFormOwner changes won’t affect other form associated elements.

All changes are enclosed by the condition that checks the elements have formAttr so I think there is no change in behavior of elements which don't have formAttr.  Existing tests (LayoutTests/fast/forms/form-attribute*.html) would cover the effects of this patch, including effects for elements which don't have formAttr.

Thanks,
Comment 10 Kent Tamura 2011-07-14 21:13:58 PDT
'form' attribute is a new feature, which Safari 5.0 doesn't have, and I think form=<non-existent-form-id> is a very rare case.  So I think it's safe to fix this bug.
Comment 11 Kent Tamura 2011-07-14 21:17:06 PDT
Comment on attachment 100921 [details]
Patch V1

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

> LayoutTests/fast/forms/form-attribute-nonexistence-form-id.html:19
> +var element_should_have_form = document.getElementById('input1');
> +var element_should_not_have_form = document.getElementById('input2');

We usually use camelCase for variable names in JavaScript code though we don't have a formal style guide for JavaScript.
Comment 12 Kenichi Ishibashi 2011-07-14 21:36:53 PDT
Committed r91048: <http://trac.webkit.org/changeset/91048>
Comment 13 Kenichi Ishibashi 2011-07-14 21:38:54 PDT
(In reply to comment #11)
> We usually use camelCase for variable names in JavaScript code though we don't have a formal style guide for JavaScript.

Committed with a change to use camelCase.  Thanks!
Comment 14 Boris Zbarsky 2011-07-15 08:10:23 PDT
Thank you!
Comment 15 Darin Adler 2011-07-15 08:25:40 PDT
(In reply to comment #9)
> All changes are enclosed by the condition that checks the elements have formAttr

Really? Are the resetFormOwner changes enclosed by that condition?
Comment 16 Kenichi Ishibashi 2011-07-16 03:47:39 PDT
(In reply to comment #15)
> Really? Are the resetFormOwner changes enclosed by that condition?

Yes. At present, resetFormOwner() is called at two places. 

- FormAssociatedElement::formAttributeChanged()
- Document::resetFormElementsOwner()

The former checks element->fastHasAttribute(formAttr) and when its value is false, resetFormOwner() is never called.  The former iterates m_formElementsWithFormAttribute, which contain only elements which have form attribute, to call resetFormElementsOwner().  I think this patch doesn't affect behavior for elements which don't have form attribute for that reason.

Thanks,
Comment 17 Kenichi Ishibashi 2011-07-16 03:48:48 PDT
(In reply to comment #16)
> The former iterates m_formElementsWithFormAttribute,

The letter iterates m_formElementsWithFormAttribute,
Comment 18 Darin Adler 2011-07-16 08:08:28 PDT
Got it. Thanks.