Bug 112487 - [HTMLTemplateElement] prevent </template> from matching "template" in a non-HTML tags on the stack of open elements
Summary: [HTMLTemplateElement] prevent </template> from matching "template" in a non-H...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 103547
  Show dependency treegraph
 
Reported: 2013-03-15 18:06 PDT by Rafael Weinstein
Modified: 2013-03-18 10:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.56 KB, patch)
2013-03-15 18:09 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Weinstein 2013-03-15 18:06:35 PDT
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).
Comment 1 Rafael Weinstein 2013-03-15 18:09:18 PDT
Created attachment 193410 [details]
Patch
Comment 2 Adam Klein 2013-03-15 18:29:46 PDT
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.
Comment 3 Rafael Weinstein 2013-03-15 18:36:34 PDT
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.
Comment 4 Adam Barth 2013-03-17 16:52:15 PDT
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.
Comment 5 WebKit Review Bot 2013-03-17 17:00:52 PDT
Comment on attachment 193410 [details]
Patch

Clearing flags on attachment: 193410

Committed r146028: <http://trac.webkit.org/changeset/146028>
Comment 6 WebKit Review Bot 2013-03-17 17:00:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Adam Klein 2013-03-18 10:05:57 PDT
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.
Comment 8 Rafael Weinstein 2013-03-18 10:12:14 PDT
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)