Bug 64509 – <input form="x"> should not associate the input with any forms when there is no form with id="x"

• Component: Forms
• Status: RESOLVED FIXED

[Boris Zbarsky]

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.

[Boris Zbarsky]

Note also http://www.w3.org/Bugs/Public/show_bug.cgi?id=12241

[Kenichi Ishibashi]

Created attachment 100917 [details]
Patch V0

[Kenichi Ishibashi]

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

[Kent Tamura]

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.

[Kent Tamura]

(In reply to comment #4)
> shouldBeNull('getElementByName("input2").form') is enough, right?

getElementsByName("input2")[0].form

[Kenichi Ishibashi]

Created attachment 100921 [details]
Patch V1

[Kenichi Ishibashi]

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.

[Darin Adler]

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.

[Kenichi Ishibashi]

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

[Kent Tamura]

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

[Kent Tamura]

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.

[Kenichi Ishibashi]

Committed r91048: <http://trac.webkit.org/changeset/91048>

[Kenichi Ishibashi]

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

[Boris Zbarsky]

Thank you!

[Darin Adler]

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

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

(In reply to comment #16)
> The former iterates m_formElementsWithFormAttribute,

The letter iterates m_formElementsWithFormAttribute,

[Darin Adler]

Got it. Thanks.