Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

amp-mustache: Mustache tags don't work in tables #11205

Closed
zhangsu opened this issue Sep 7, 2017 · 9 comments
Closed

amp-mustache: Mustache tags don't work in tables #11205

zhangsu opened this issue Sep 7, 2017 · 9 comments

Comments

@zhangsu
Copy link
Member

zhangsu commented Sep 7, 2017

What's the issue?

With the following AMP HTML:

<amp-list width="auto" height="200" layout="fixed-height" credentials="include"
    [state]="comments" src="https://example.com">
  <template type="amp-mustache">
    <table>
      <tr>
        <td>Comment:</td>
        <td>{{content}}</td>
      </tr>
      {{#replies}}
        <tr>
          <td>Reply:</td>
          <td>{{content}}</td>
        </tr>
      {{/replies}}
    </table>
  </template>
</amp-list>
<button on="tap:AMP.setState({comments: [{content: 'Hello', replies: [{content: 'Hi'}]}, {content: 'Howdy', replies: []}]})">
  Set state
</button>

I expect the rendered tables to be

Comment: Hello
Reply: Hi
Comment: Howdy

Because the first comment in the comments state has one item in the replies list, but the second comment doesn't.

However, the actual rendered tables are

Comment: Hello
Reply: Hello
Comment: Howdy
Reply: Howdy

The {{#replies}} section is evaluated to the wrong value for the first comment (should be Reply: Hi instead of Reply: Hello); The {{#replies}} section is surprisingly evaluated for the second comment even though the second comment has an empty replies list.

If I change the template to the following, then the presentation becomes okay (although not quite the DOM structure I was hoping for):

  <template type="amp-mustache">
    <table>
      <tr>
        <td>Comment:</td>
        <td>{{content}}</td>
      </tr>
    </table>
    {{#replies}}
      <table>
        <tr>
          <td>Reply:</td>
          <td>{{content}}</td>
        </tr>
      </table>
    {{/replies}}
  </template>

Presentationally this looks the same as my expectation (without table/cell borders), but structurally it renders 3 tables instead of 2.

What browsers are affected?

Only tested on Chrome.

Which AMP version is affected?

Tested with AMP version 1504040004635.

@dreamofabear
Copy link

Tried this out on the mustache demo and looks like this is WAI.

Mustache:

<table>
   <tr>
     <td>Comment:</td>
     <td>{{content}}</td>
   </tr>
   {{#replies}}
     <tr>
       <td>Reply:</td>
       <td>{{content}}</td>
     </tr>
   {{/replies}}
</table>

JSON:

{
   "content": "Howdy",
   "replies": []
}

Rendered HTML:

<table>
<tr>
<td>Comment:</td>
<td>Howdy</td>
</tr>
</table>

Is it possible to use a different variable name for the reply text? I.e. something other content to avoid clashing with comment's content?

@zhangsu
Copy link
Member Author

zhangsu commented Sep 7, 2017

Hmm, it looks like the Mustache engine itself is working as intended. The second row of the table is correctly not rendered because replies is empty.

However, rendering the same Mustache template with the same JSON in AMP HTML (after clicking the button):

<amp-list width="auto" height="200" layout="fixed-height" credentials="include"
    [state]="comments" src="https://example.com">
  <template type="amp-mustache">
    <table>
      <tr>
        <td>Comment:</td>
        <td>{{content}}</td>
      </tr>
      {{#replies}}
        <tr>
          <td>Reply:</td>
          <td>{{content}}</td>
        </tr>
      {{/replies}}
    </table>
  </template>
</amp-list>
<button on="tap:AMP.setState({comments: [{content: 'Howdy', replies: []}]})">
  Set state
</button>

Gives me this:

<table role="listitem">
  <tr>
    <td>Comment:</td>
    <td>Howdy</td>
  </tr>
  <tr>
    <td>Reply:</td>
    <td>Howdy</td>
  </tr>
</table>

The second row of the table shouldn't be rendered because replies is empty.

Did using the same variable name in the reply text cause this? I can certainly use a different name, but I was under the impression that the {{#replies}} section shouldn't even be rendered when replies in the JSON is empty. Also, shouldn't using the same variable name in different "sections" of the template be allowed? I thought {{content}} in the {{#replies}} section should always evaluate to the content property of each item in the replies list...

@dreamofabear
Copy link

Sorry, you're right. 😛 Looks like innerHTML returns a mixed up version of the template markup:

> $0.innerHTML
"
      
        {{#replies}}
          
        {{/replies}}
      <table>
        <tbody><tr>
          <td>Comment:</td>
          <td>{{content}}</td>
        </tr><tr>
            <td>Reply:</td>
            <td>{{content}}</td>
          </tr></tbody></table>
    "

Not related to AMP code. Investigating.

@dreamofabear
Copy link

dreamofabear commented Sep 7, 2017

The issue is that the browser parses the children of <template> elements into a document fragment at page load, and text nodes are not valid children of <table> elements.

This results in the {{#replies}} and {{/replies}} text nodes being foster parented. A good example of this here: https://www.w3.org/TR/html5/single-page.html#unexpected-markup-in-tables

A workaround is to use elements that have looser semantics, e.g. <div> and <p>. I've tried the following which works:

<template type="amp-mustache">
  <div>
    <div>
      <p>Comment:</p>
      <p>{{content}}</p>
    </div>
    {{#replies}}
      <div>
        <p>Reply:</p>
        <p>{{content}}</p>
      </div>
    {{/replies}}
  </div>
</template>

Another method is to support a way to specify a template as an unparsed string, e.g. as a <script type="text/plain"> child.

Thanks for reporting this.

@dreamofabear dreamofabear changed the title AMP Mustache template section behaves weird when there's bare <tr> inside amp-mustache: Mustache tags don't work in tables Sep 7, 2017
@dreamofabear dreamofabear added this to Unprioritized in Runtime Sep 11, 2017
@dreamofabear dreamofabear moved this from Unprioritized to Nice to have in Runtime Sep 11, 2017
@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @choumx Do you have any updates?

@ericlindley-g ericlindley-g added this to Bugs in UI Jan 9, 2018
@meronym
Copy link

meronym commented Jan 24, 2018

I managed to work this out (although in a not very elegant way) by commenting out the section placeholders (so the browser's parser won't foster parent them):

  <template type="amp-mustache">
    <table>
      <!-- {{#replies}} -->
        <tr>
          <td>Reply:</td>
          <td>{{content}}</td>
        </tr>
      <!-- {{/replies}} -->
    </table>
  </template>

@dreamofabear
Copy link

Another workaround: triple mustache now supports rendering table elements.

Closing since there are a few ways to solve this.

@jaygray0919
Copy link

@choumx said:

A workaround is to use elements that have looser semantics, e.g. <div> and <p>.

<p> also generates a role="lisitem" and additional element. see: #17509

Further, if there are 2+ span structures nested in <p>, the output generates a new <p> structure for each span.

That created havoc for elements that are intended to be inline - instead, we get multiple block level output: one for each new <p>

@dreamofabear
Copy link

Re: @jaygray0919's comment, see #17509 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Runtime
Email
UI
  
Bugs
Development

No branches or pull requests

5 participants