Bug 112487 – [HTMLTemplateElement] prevent </template> from matching "template" in a non-HTML tags on the stack of open elements

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

[Rafael Weinstein]

e.g. <template><svg><template><foreignObject><div></template>

The </template> needs to clear back to the first (HTML) template, not the middle (SVG) template.

Note that this is an open bug against HTML about this issue which exists in other contexts, and this fix is consistent with the most likely resolution (that the parser needs to be more aware about the namespace of elements when it trys to examine the stack of open elements for specific tags).

[Rafael Weinstein]

Created attachment 193410 [details]
Patch

[Adam Klein]

Comment on attachment 193410 [details]
Patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968
>      if (!m_tree.currentStackItem()->hasLocalName(token->name()))

Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here.

[Rafael Weinstein]

Comment on attachment 193410 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968
>>      if (!m_tree.currentStackItem()->hasLocalName(token->name()))
> 
> Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here.

At this point, we know that |token| is an HTMLTemplate element, otherwise we wouldn't be here. I see what you are getting at, but it seems fine to me that HTML parser insertion modes can make the assumption that the current token is HTML. The danger we are fixing her is that the steps for processing foreignContent may have put non-HTML elements on the stack, so we have to be careful when examining it.

[Adam Barth]

Comment on attachment 193410 [details]
Patch

Ok.  Please make sure we stay in sync with the spec if this issue isn't resolved the way we expect.

[WebKit Review Bot]

Comment on attachment 193410 [details]
Patch

Clearing flags on attachment: 193410

Committed r146028: <http://trac.webkit.org/changeset/146028>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

Comment on attachment 193410 [details]
Patch

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

>>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968
>>>      if (!m_tree.currentStackItem()->hasLocalName(token->name()))
>> 
>> Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here.
> 
> At this point, we know that |token| is an HTMLTemplate element, otherwise we wouldn't be here. I see what you are getting at, but it seems fine to me that HTML parser insertion modes can make the assumption that the current token is HTML. The danger we are fixing her is that the steps for processing foreignContent may have put non-HTML elements on the stack, so we have to be careful when examining it.

I'm not sure how you can make the assumption that because |token| is being treated as HTML, that currentStackItem is also HTML. Perhaps because this is processTemplateEndTag? If it were a start tag, then you definitely couldn't make that assumption (since, e.g., <mtext> in MathML will cause the parser to treat tokens as HTML, and in that case, the currentStackItem will have a MathML namespace URI, though of course it won't have a <template> localName).

Anyway, again, just a parse error, but I think we'll want to tighten up _all_ these callsites.

[Rafael Weinstein]

Comment on attachment 193410 [details]
Patch

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

>>>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:968
>>>>      if (!m_tree.currentStackItem()->hasLocalName(token->name()))
>>> 
>>> Not a big deal since it's "just" a parse error, but seems like hasTagName() would be better here.
>> 
>> At this point, we know that |token| is an HTMLTemplate element, otherwise we wouldn't be here. I see what you are getting at, but it seems fine to me that HTML parser insertion modes can make the assumption that the current token is HTML. The danger we are fixing her is that the steps for processing foreignContent may have put non-HTML elements on the stack, so we have to be careful when examining it.
> 
> I'm not sure how you can make the assumption that because |token| is being treated as HTML, that currentStackItem is also HTML. Perhaps because this is processTemplateEndTag? If it were a start tag, then you definitely couldn't make that assumption (since, e.g., <mtext> in MathML will cause the parser to treat tokens as HTML, and in that case, the currentStackItem will have a MathML namespace URI, though of course it won't have a <template> localName).
> 
> Anyway, again, just a parse error, but I think we'll want to tighten up _all_ these callsites.

You are correct. Using hasTagName(templateTag)