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

Created an attachment (id=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.

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

Created an attachment (id=100917) [details]
Patch V0

(In reply to comment #2)
> Created an attachment (id=100917) [details] [details]
> Patch V0

Changed the bug title to workaround the problem that webkit-patch cannot upload patches when the title includes '<form>'.

(From update of attachment 100917 [details])
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.

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

getElementsByName("input2")[0].form

Created an attachment (id=100921) [details]
Patch V1

(From update of attachment 100917 [details])
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.

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

(In reply to comment #8)
> (From update of attachment 100921 [details] [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,

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

(From update of attachment 100921 [details])
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.

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

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

Thank you!

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

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

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

The letter iterates m_formElementsWithFormAttribute,

Got it. Thanks.