Bug 50195 – Progress and meter elements should take a form in their constructor like any other form control.

• Component: DOM
• Status: RESOLVED FIXED

[Yael]

Progress and meter elements should take a form in their constructor like any other form control.

[Yael]

Created attachment 75090 [details]
Patch.

[Darin Adler]

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.

[Early Warning System Bot]

Attachment 75090 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6372115

[Eric Seidel (no email)]

Attachment 75090 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6338113

[WebKit Review Bot]

Attachment 75090 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6414110

[Yael]

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

[Eric Seidel (no email)]

Attachment 75090 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6444040

[Yael]

Any suggestion which file to touch in order to trigger a clean build?
thanks!

[Build Bot]

Attachment 75090 [details] did not build on win:
Build output: http://queues.webkit.org/results/6450028

[WebKit Review Bot]

Attachment 75090 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6384111

[Yael]

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.

[Early Warning System Bot]

Attachment 75136 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6365122

[Eric Seidel (no email)]

Attachment 75136 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6299141

[WebKit Review Bot]

Attachment 75136 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6311113

[Yael]

Created attachment 75146 [details]
Patch.

Fix the build.

[Darin Adler]

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

[Darin Adler]

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.

[Yael]

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

[Darin Adler]

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

[Alexey Proskuryakov]

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.

[Yael]

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.

[Yael]

Committed r72976: <http://trac.webkit.org/changeset/72976>