Progress and meter elements should take a form in their constructor like any other form control.
Created attachment 75090 [details] Patch.
Comment on attachment 75090 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=75090&action=review review- because this bug does require a test case > WebCore/ChangeLog:8 > + No new tests since this is a code cleanup only. That is incorrect. The reason form elements get a form argument is so that they are correctly associated with the form in cases when the form is not an ancestor of the form element. We need to construct a test case that demonstrates this not occurring for meter elements before and happening correctly for meter elements after the patch.
Attachment 75090 [details] did not build on qt: Build output: http://queues.webkit.org/results/6372115
Attachment 75090 [details] did not build on mac: Build output: http://queues.webkit.org/results/6338113
Attachment 75090 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6414110
(In reply to comment #2) > (From update of attachment 75090 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75090&action=review > > review- because this bug does require a test case > > > WebCore/ChangeLog:8 > > + No new tests since this is a code cleanup only. > > That is incorrect. The reason form elements get a form argument is so that they are correctly associated with the form in cases when the form is not an ancestor of the form element. We need to construct a test case that demonstrates this not occurring for meter elements before and happening correctly for meter elements after the patch. Sorry, I thought that HTMLElement::findFormAncestor() was taking care of associating the correct form. I'll add a test. I also need to touch some header file to force a rebuild on the buildbots.
Attachment 75090 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6444040
Any suggestion which file to touch in order to trigger a clean build? thanks!
Attachment 75090 [details] did not build on win: Build output: http://queues.webkit.org/results/6450028
Attachment 75090 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6384111
Created attachment 75136 [details] Patch. Added tests and also touch HTMLTagNames.in, to trigger re-generating HTMLElementFactory.cpp, to keep buildbots happy. Please note that the tests are passing even without this patch because HTMLFormControlElement::insertedIntoTree will associate the form control with the form if the form was inserted first. HTMLFormElement::insertedIntoDocument will associate the form control with the form if the form is inserted after the form control.
Attachment 75136 [details] did not build on qt: Build output: http://queues.webkit.org/results/6365122
Attachment 75136 [details] did not build on mac: Build output: http://queues.webkit.org/results/6299141
Attachment 75136 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6311113
Created attachment 75146 [details] Patch. Fix the build.
(In reply to comment #11) > Please note that the tests are passing even without this patch because HTMLFormControlElement::insertedIntoTree will associate the form control with the form if the form was inserted first. HTMLFormElement::insertedIntoDocument will associate the form control with the form if the form is inserted after the form control. Then the tests are incorrect. The whole point is to create tests that show the behavior that requires passing the form pointer. If there is no behavior that requires passing a form pointer, then we should eliminate the form pointer from WebKit or at least eliminate it from the constructor arguments for form elements.
Comment on attachment 75146 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=75146&action=review Thanks very much for writing these tests. But we need tests that fail to prove there is a bug to fix, not tests that already pass! > LayoutTests/fast/dom/HTMLMeterElement/script-tests/meter-element-outside-form.js:1 > +description('Test that a meter element is correctly associated with its for even if the form is not its ancestor.'); Typo "with its for". > LayoutTests/fast/dom/HTMLProgressElement/script-tests/progress-element-outside-form.js:1 > +description('Test that a progress element is correctly associated with its for even if the form is not its ancestor.'); Same typo here.
(In reply to comment #17) > (From update of attachment 75146 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75146&action=review > > Thanks very much for writing these tests. But we need tests that fail to prove there is a bug to fix, not tests that already pass! > Darin, could you please elaborate on the bug? It seems to me that the form association of meter element and the form is done correctly even if the constructor does not take form as an argument. This is done when the meter element is inserted into the tree. HTMLFormControlElement::insertedIntoTree() line 163 : m_form = static_cast<HTMLFormElement*>(element) If the meter element is created with createElement('meter') and not inserted into the tree, then we are not passing a form attribute to the constructor anyways. What am I missing?
(In reply to comment #18) > Darin, could you please elaborate on the bug? To find tests that depend on passing the form in when creating an element you could change HTMLConstructionSite::createHTMLElement so it passes 0 instead of form(). Then you will see some tests fail, which might give you a hint about the types of cases that require this. Then you could make similar tests with these element types. Or just add these element types to the existing test.
An input can be associated with a form even when it's not a descendant of the form element in DOM. Darin's suggestion above is certainly the best way to discover more detail.
Created attachment 75207 [details] Patch. Thank you for the advice, it helped me find that forms were not handled correctly e.g. when they are directly inside a table. Modified tests are attached.
Committed r72976: <http://trac.webkit.org/changeset/72976>