Bug 78734 - Modify HTML parser to support <template> element
Summary: Modify HTML parser to support <template> element
Status: RESOLVED DUPLICATE of bug 86031
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 78733
  Show dependency treegraph
 
Reported: 2012-02-15 13:15 PST by Dimitri Glazkov (Google)
Modified: 2012-11-28 12:30 PST (History)
5 users (show)

See Also:


Attachments
WIP: First stab. (11.82 KB, patch)
2012-02-15 13:18 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Moar better, moar tests. (15.79 KB, patch)
2012-02-22 15:15 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Faster, harder, stronger. (16.11 KB, patch)
2012-02-23 15:57 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2012-02-15 13:15:39 PST
Modify HTML parser to support <template> element
Comment 1 Dimitri Glazkov (Google) 2012-02-15 13:18:08 PST
Created attachment 127223 [details]
WIP: First stab.
Comment 2 Dimitri Glazkov (Google) 2012-02-15 13:19:13 PST
For the moment, this is a playground for me to learn about parser and understand what changes to the algorithm are necessary.
Comment 3 Adam Barth 2012-02-15 13:32:01 PST
Comment on attachment 127223 [details]
WIP: First stab.

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

I know you said that this patch was a just a playground, but I thought the comments below might be helpful:

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2248
> +        if (token.name() == templateTag) {
> +            m_tree.openElements()->pop();
> +            return;
> +        }

This change is slightly surprising.  You could be popping any sort of element off the stack, not necessarily a template element.  You probably either want:

if (m_tree.currentNode()->hasTagName(templateTag)) {
    m_tree.openElements()->pop();
    return;
}

or

m_tree.openElements()->popUntilPopped(templateTag.localName());
resetInsertionModeAppropriately();
return;

That's likely what's triggering your ASSERT on line 1686.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2275
> +    case TemplateContentsMode:
> +        m_tree.openElements()->pop();
> +        resetInsertionModeAppropriately();
> +        break;

This parsing might be somewhat surprising:

<div><template></div>Hello

I think it would result in the following DOM:

- <div>
    - <template>
    - #text: hello

I'm not 100% sure what the best thing to do in this case is.  Maybe:

1) If the end tag is </template>, then pop the open elements.
2) otherwise, pop the pop the open elements, reset the insertion mode, and reprocess the token

?
Comment 4 Dimitri Glazkov (Google) 2012-02-15 13:33:38 PST
Comment on attachment 127223 [details]
WIP: First stab.

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

> LayoutTests/html5lib/resources/template.dat:187
> +|     <table>
> +|       <thead>
> +|         <template>
> +|           <tr>
> +|             <td>

This case is interesting. Where should implied <tr> appear?
Comment 5 Adam Barth 2012-02-15 13:40:46 PST
Comment on attachment 127223 [details]
WIP: First stab.

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

>> LayoutTests/html5lib/resources/template.dat:187
>> +|             <td>
> 
> This case is interesting. Where should implied <tr> appear?

Conceptually, from a parsing point of view, what you've written here is the most natural.  The <td> creates the implied <tr>, and it would do so as a child of the currently open element, which is the <template>.
Comment 6 Dimitri Glazkov (Google) 2012-02-15 13:50:39 PST
Comment on attachment 127223 [details]
WIP: First stab.

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2248
>> +        }
> 
> This change is slightly surprising.  You could be popping any sort of element off the stack, not necessarily a template element.  You probably either want:
> 
> if (m_tree.currentNode()->hasTagName(templateTag)) {
>     m_tree.openElements()->pop();
>     return;
> }
> 
> or
> 
> m_tree.openElements()->popUntilPopped(templateTag.localName());
> resetInsertionModeAppropriately();
> return;
> 
> That's likely what's triggering your ASSERT on line 1686.

Yay, thanks! I misunderstood what token means in this context.

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2275
>> +        break;
> 
> This parsing might be somewhat surprising:
> 
> <div><template></div>Hello
> 
> I think it would result in the following DOM:
> 
> - <div>
>     - <template>
>     - #text: hello
> 
> I'm not 100% sure what the best thing to do in this case is.  Maybe:
> 
> 1) If the end tag is </template>, then pop the open elements.
> 2) otherwise, pop the pop the open elements, reset the insertion mode, and reprocess the token
> 
> ?

Oh, right. Yes. I think your plan makes sense.
Comment 7 Adam Barth 2012-02-15 14:08:11 PST
There's a general question of how "scoping" you want </template> to be.  In the

<div><template></div>Hello

example, there are two reasonable parses:

- <div>
  - <template>
    - #text: Hello

or

- <div>
  - <template>
- #text: Hello

In the first case, <template> is strongly scoping, which means it's hard to escape from the template element's scope.  In the second case, the template element isn't very scoping, which means the </div> token can find and close the div element.

I'd recommend trying to match the scoping of down-rev parsers as much as feasible, which, in this case, means you'll want the second (less scoping) parsing.
Comment 8 Adam Barth 2012-02-15 14:10:16 PST
Some examples:

<div><object></div>Hello
<div><foo></div>Hello

These examples parse differently because <object> is pretty strongly scoping.
Comment 9 Dimitri Glazkov (Google) 2012-02-22 15:15:51 PST
Created attachment 128299 [details]
Moar better, moar tests.
Comment 10 Adam Barth 2012-02-22 15:35:14 PST
Comment on attachment 128299 [details]
Moar better, moar tests.

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

This is starting to look really good.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2281
> +    case TemplateContentsMode:
> +        if (token.name() == templateTag) {
> +            m_tree.openElements()->pop();
> +            resetInsertionModeAppropriately();
> +        }
> +        break;

Can you ASSERT here that the top element is a templateTag?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2578
> +    case TemplateContentsMode:
> +        // FIXME: What should we do here?

Probably pop the <template> element off the stack of open element, reset the insertion mode appropriately, and reprocess the token.  :)
Comment 11 Hajime Morrita 2012-02-22 17:22:12 PST
Comment on attachment 128299 [details]
Moar better, moar tests.

Don't you hide <template> behind the flag?
Comment 12 Dimitri Glazkov (Google) 2012-02-22 17:35:03 PST
(In reply to comment #11)
> (From update of attachment 128299 [details])
> Don't you hide <template> behind the flag?

Nah. This patch is just for me to learn what to write in the spec.
Comment 13 Hajime Morrita 2012-02-22 17:56:34 PST
> Nah. This patch is just for me to learn what to write in the spec.
Ah got it.
Comment 14 Dimitri Glazkov (Google) 2012-02-23 15:57:51 PST
Created attachment 128579 [details]
Faster, harder, stronger.
Comment 15 Rafael Weinstein 2012-11-28 12:13:04 PST
Dimtiri, can we close this now?
Comment 16 Dimitri Glazkov (Google) 2012-11-28 12:17:23 PST
(In reply to comment #15)
> Dimtiri, can we close this now?

Yes sir! Please dupe it against the real bug for future archeologists.
Comment 17 Rafael Weinstein 2012-11-28 12:30:25 PST

*** This bug has been marked as a duplicate of bug 86031 ***