WebKit Bugzilla
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.
Note also http://www.w3.org/Bugs/Public/show_bug.cgi?id=12241
Created attachment 100917 [details] Patch V0
(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 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.
(In reply to comment #4) > shouldBeNull('getElementByName("input2").form') is enough, right? getElementsByName("input2")[0].form
Created attachment 100921 [details] Patch V1
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 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.
(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,
'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 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.
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.