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.

Sign up for GitHub

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

Jump to bottom

The parent tag of tag 'amp-sidebar' is 'amp-bind-macro', but it can only be 'body'. #20368

Closed
AcidRaZor opened this issue Jan 16, 2019 · 9 comments
Closed

The parent tag of tag 'amp-sidebar' is 'amp-bind-macro', but it can only be 'body'. #20368

AcidRaZor opened this issue Jan 16, 2019 · 9 comments
Assignees
@Gregable
Labels
Type: Bug WG: caching
Milestone
Pending Triage

Comments

@AcidRaZor
Copy link

What's the issue?

When using amp-bind-macro, amp-sidebar causes a validation issue with the following error message:

The parent tag of tag 'amp-sidebar' is 'amp-bind-macro', but it can only be 'body'.

How do we reproduce the issue?

I use the following macros to format currency:


<amp-bind-macro id="getLog10" arguments="num" expression="num.length - 1" />
    <amp-bind-macro id="numShouldHaveComma" arguments="log10" expression="log10%3==0 && log10!=0" />
    <amp-bind-macro id="formatNumber_map" arguments="char, ind, numStr" expression="(numShouldHaveComma(getLog10(numStr.substr(ind)))) ? char+',' : char" />
    <amp-bind-macro id="formatNumber" arguments="num, numStr" expression="(num < 9999999) ? 'R ' + numStr.split('').map((char,ind)=>formatNumber_map(char,ind, numStr)).join('') : 'P.O.A'" />
    <amp-bind-macro id="formatNumberPM" arguments="num, numStr, retail_num" expression="(round(retail_num) > 9999999) ? 'P.O.A' : 'R ' + numStr.split('').map((char,ind)=>formatNumber_map(char,ind, numStr)).join('') + 'pm*'" />
    <amp-bind-macro id="formatMoney" arguments="val" expression="formatNumber(round(val), round(val).toString())" />
    <amp-bind-macro id="formatMoneyPM" arguments="val, val2" expression="formatNumberPM(round(val), round(val).toString(), val2)" />

What have you used to check?

https://search.google.com/test/amp
https://validator.ampproject.org

Which AMP version is affected?

New issue as far as I am aware, I don't always check validity of the pages I generate, but did notice several months ago that amp-bind-macro wraps everything within the body tag for some reason.
@AcidRaZor
Copy link
Author

I'm also wondering if this is the reason why, when using mobile preview or testing facilities that shows you a screenshot of the site, why it shows all text (html) and not the actual site.

@aghassemi
Copy link
Contributor

@AcidRaZor Is amp-sidebar actually a child of body? Can you post your HTML?

I quick try with the following code did not generate any issues for me:

<!doctype html>
<html ⚡>
<head>
  <meta charset="utf-8">
  <link rel="canonical" href="self.html" />
  <meta name="viewport" content="width=device-width,minimum-scale=1">
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  <script async src="https://cdn.ampproject.org/v0.js"></script>
  <script async custom-element="amp-sidebar" src="https://cdn.ampproject.org/v0/amp-sidebar-0.1.js"></script>
  <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
</head>
<body>Hello, AMP world.
  <amp-sidebar layout=nodisplay ></amp-sidebar>
  <amp-bind-macro id="circleArea" arguments="radius" expression="3.14 * radius * radius" />
</body>
</html>

@aghassemi aghassemi self-assigned this Jan 16, 2019
@aghassemi aghassemi added this to the Pending Triage milestone Jan 16, 2019
@aghassemi
Copy link
Contributor

I can reproduce this if amp-sidebar is AFTER amp-bind-macro AND amp-bind-macro is self-closing.
e.g.

<body>Hello, AMP world.
    <amp-bind-macro id="getLog10" arguments="num" expression="num.length - 1"/>
    <amp-sidebar layout=nodisplay ></amp-sidebar>
</body>

@AcidRaZor For a quick work-around, instead of using a self-closing amp-bind-macro (e.g. <amp-bind-macro id="getLog10" arguments="num" expression="num.length - 1"/>) use a full explicit end tag (e.g. <amp-bind-macro id="getLog10" arguments="num" expression="num.length - 1"></amp-bind-macro>

/to @Gregable, looks like there is a parsing issue in parent detection of elements which are after self-closing tags.

@aghassemi aghassemi assigned Gregable and unassigned aghassemi Jan 16, 2019
@Gregable
Copy link
Member

There is no such thing as a self-closing tag in html5. There are about a dozen void elements which have no associated closing tag, but self-closing tag syntax is not a thing.

So, a browser (and the amp validator) will see the above example as the same parse tree as:

<body>Hello, AMP world.
    <amp-bind-macro id="getLog10" arguments="num" expression="num.length - 1">
      <amp-sidebar layout=nodisplay ></amp-sidebar>
    </amp-bind>
</body>

@Gregable
Copy link
Member

Technically, there are self-closing tags within other namespaces within HTML. The only such namespace allowed by AMP is SVG. This isn't relevant for the question, but to avoid confusion.

@Gregable
Copy link
Member

Closing as working as intended.

@aghassemi
Copy link
Contributor

I see. I will update the examples then.

aghassemi added a commit that referenced this issue Jan 16, 2019
@aghassemi
🐛 amp-bind: close amp-bind-macro tag in example
5952b54 
Fixes #20368
@aghassemi aghassemi mentioned this issue Jan 16, 2019
Merged
dreamofabear pushed a commit that referenced this issue Jan 16, 2019
@aghassemi
🐛 amp-bind: close amp-bind-macro tag in example (#20382)
049f005 
Fixes #20368
@AcidRaZor
Copy link
Author

Thanks guys!

@AcidRaZor
Copy link
Author

Just as another FYI, I did have the macro's after the amp-sidebar and it still did the same, but the explanation given makes sense. Thanks again for the quick response

This was referenced Feb 8, 2019
Closed
Closed
noranazmy pushed a commit to noranazmy/amphtml that referenced this issue Mar 22, 2019
@aghassemi
🐛 amp-bind: close amp-bind-macro tag in example (ampproject#20382)
118fca9 
Fixes ampproject#20368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Gregable Gregable

Labels
Type: Bug WG: caching
Projects
None yet
Milestone
Pending Triage
Development

No branches or pull requests
4 participants
@Gregable @aghassemi @AcidRaZor @dreamofabear