HTML parser should not associate elements inside templates with forms

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Merge https://chromium.googlesource.com/chromium/blink/+/45aadf7ee7ee010327eb692066cf013315ef3ed7

When parsing <form><template><input>, the previous behavior was to associate
the <input> with the <form>, even though they're not in the same tree (or
even the same document!).

This patch changes that by checking, prior to creating a form control element,
whether the element to be created lives in a document with a browsing context.

[Ryosuke Niwa]

Created attachment 217392 [details]
Patch

[Ryosuke Niwa]

Created attachment 217393 [details]
Patch

[Antti Koivisto]

Comment on attachment 217393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217393&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:548
> -    RefPtr<Element> element = HTMLElementFactory::createElement(tagName, ownerDocumentForCurrentNode(), form(), true);
> +    Document& ownerDocument = ownerDocumentForCurrentNode();
> +    bool ownerDocumentHasBrowsingContext = !!ownerDocument.frame();
> +    RefPtr<Element> element = HTMLElementFactory::createElement(tagName, ownerDocument, ownerDocumentHasBrowsingContext ? form() : 0, true);

I'm not entirely sure this is the best way to do this. Shouldn't m_form be null if it does not apply in the current context?

ownerDocumentHasBrowsingContext boolean seems unnecessary.
0 -> nullptr

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 217393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217393&action=review
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:548
> > -    RefPtr<Element> element = HTMLElementFactory::createElement(tagName, ownerDocumentForCurrentNode(), form(), true);
> > +    Document& ownerDocument = ownerDocumentForCurrentNode();
> > +    bool ownerDocumentHasBrowsingContext = !!ownerDocument.frame();
> > +    RefPtr<Element> element = HTMLElementFactory::createElement(tagName, ownerDocument, ownerDocumentHasBrowsingContext ? form() : 0, true);
> 
> I'm not entirely sure this is the best way to do this. Shouldn't m_form be null if it does not apply in the current context?
> 
> ownerDocumentHasBrowsingContext boolean seems unnecessary.
> 0 -> nullptr

That's a good point. Let me dig into it.

[Ryosuke Niwa]

Created attachment 217516 [details]
Patch

[Ryosuke Niwa]

We can't update m_form because HTMLTreeBuilder::processIsindexStartTagForInBody uses it.

I've asked why this behavior is desirable for isindex on WHATWG:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-November/041616.html

If the spec. changes to also change is index's behavior we can match update our code accordingly.  Until then, we should land this patch so that our behavior matches that of Chrome & Firefox.

[Build Bot]

Comment on attachment 217516 [details]
Patch

Attachment 217516 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/32028027

New failing tests:
fast/forms/formmove3.html
fast/parser/form-pointer-4.html
fast/forms/preserveFormDuringResidualStyle.html
fast/parser/form-pointer-1.html
fast/parser/form-pointer-2.html
fast/forms/parser-associated-form-removal.html
fast/dom/HTMLTemplateElement/no-form-association.html

[Build Bot]

Created attachment 217517 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Ryosuke Niwa]

Created attachment 217518 [details]
Fixed the bug by negating the boolean logic

[WebKit Commit Bot]

Comment on attachment 217518 [details]
Fixed the bug by negating the boolean logic

Clearing flags on attachment: 217518

Committed r159618: <http://trac.webkit.org/changeset/159618>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.