Bug 125279 – Change how the form element pointer affects parsing template elements, to reduce weirdness in templates

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Make HTML5 parser changes to match the HTML5 specification change: http://html5.org/tools/web-apps-tracker?from=8330&to=8331

Change how the form element pointer affects parsing <template> elements, to reduce weirdness in templates (e.g. before you couldn't have a template that contained a form if it was itself inside a form).

[Ryosuke Niwa]

Created attachment 218487 [details]
Work in progress

[Ryosuke Niwa]

Filed the corresponding Gecko and Blink bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=946585
https://code.google.com/p/chromium/issues/detail?id=326058

[Ryosuke Niwa]

Created attachment 218489 [details]
Updates the parser

[Antti Koivisto]

Comment on attachment 218489 [details]
Updates the parser

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

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:416
> +    if (!insideTemplateElement())
> +        m_form = static_pointer_cast<HTMLFormElement>(element);
> +    static_pointer_cast<HTMLFormElement>(element)->setDemoted(isDemoted);

Please do the cast only once and put it to a local

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:544
> +inline bool HTMLConstructionSite::insideTemplateElement()
> +{
> +    return !ownerDocumentForCurrentNode().frame();
> +}

Maybe we should just have a boolean for this state? This is bit hard to understand.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1869
> +#if ENABLE(TEMPLATE_ELEMENT)
> +        if (!isParsingTemplateContents()) {
> +#endif

Not a big fan of #ifs like this

[Ryosuke Niwa]

Comment on attachment 218489 [details]
Updates the parser

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

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:416
>> +    static_pointer_cast<HTMLFormElement>(element)->setDemoted(isDemoted);
> 
> Please do the cast only once and put it to a local

Done.

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:544
>> +}
> 
> Maybe we should just have a boolean for this state? This is bit hard to understand.

Yeah, I'd love to. Unfortunately, it's very tricky to do in one patch :(

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1869
>> +#endif
> 
> Not a big fan of #ifs like this

Got rid of the if-def since the latter part doesn't use any template-specific code and "return false" is hard-coded
in isParsingTemplateContents() so that the compiler can easily eliminate the code block.

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r160182: <http://trac.webkit.org/changeset/160182>