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
: WebKit
Forms
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-07-13 20:54 PST by
Modified: 2011-07-16 08:08 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-13 20:54:31 PST
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.
------- Comment #1 From 2011-07-13 20:56:27 PST -------
Note also http://www.w3.org/Bugs/Public/show_bug.cgi?id=12241
------- Comment #2 From 2011-07-14 19:43:32 PST -------
Created an attachment (id=100917) [details]
Patch V0
------- Comment #3 From 2011-07-14 19:46:32 PST -------
(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>'.
------- Comment #4 From 2011-07-14 20:06:18 PST -------
(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.
------- Comment #5 From 2011-07-14 20:07:20 PST -------
(In reply to comment #4)
> shouldBeNull('getElementByName("input2").form') is enough, right?

getElementsByName("input2")[0].form
------- Comment #6 From 2011-07-14 20:29:09 PST -------
Created an attachment (id=100921) [details]
Patch V1
------- Comment #7 From 2011-07-14 20:30:50 PST -------
(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.
------- Comment #8 From 2011-07-14 20:33:12 PST -------
(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.
------- Comment #9 From 2011-07-14 21:07:01 PST -------
(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,
------- Comment #10 From 2011-07-14 21:13:58 PST -------
'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 From 2011-07-14 21:17:06 PST -------
(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.
------- Comment #12 From 2011-07-14 21:36:53 PST -------
Committed r91048: <http://trac.webkit.org/changeset/91048>
------- Comment #13 From 2011-07-14 21:38:54 PST -------
(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 From 2011-07-15 08:10:23 PST -------
Thank you!
------- Comment #15 From 2011-07-15 08:25:40 PST -------
(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 From 2011-07-16 03:47:39 PST -------
(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 From 2011-07-16 03:48:48 PST -------
(In reply to comment #16)
> The former iterates m_formElementsWithFormAttribute,

The letter iterates m_formElementsWithFormAttribute,
------- Comment #18 From 2011-07-16 08:08:28 PST -------
Got it. Thanks.