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

Open P tag causes JS validator to fail and cache to pass. #4654

Closed
dknecht opened this issue Aug 22, 2016 · 4 comments
Closed

Open P tag causes JS validator to fail and cache to pass. #4654

dknecht opened this issue Aug 22, 2016 · 4 comments
Assignees
@Gregable @powdercloud
Labels
WG: caching
Milestone
Backlog Bugs

Comments

@dknecht
Copy link
Contributor

dknecht commented Aug 22, 2016 •
edited

This code fails JS validator and passes cache validator. Which is the correct behavior?

<body>
<p>I am not closed!
<p>I am closed!</p>
<amp-sidebar id='sidebar' layout='nodisplay' side="right">
</amp-sidebar>
</body>
</html>
@dknecht
Copy link
Contributor Author

dknecht commented Aug 22, 2016 •
edited by Gregable

and similarly...

This pass both of them. With the missing closing tag on <li> should it?

<body>
<amp-sidebar id='sidebar' layout='nodisplay' side="right">
    <amp-accordion>
      <section>
        <h2>News</h2>
        <ul>
            <li><a href="http://example.com">Foo</a>
        </ul>
      </section>
    </amp-accordion>
</amp-sidebar>
</body>

@erwinmombay
Copy link
Member

/to @Gregable @powdercloud

@powdercloud
Copy link
Contributor

I think the relevant snippet from https://www.w3.org/TR/html5/grouping-content.html#the-p-element is:

A p element's end tag may be omitted if the p element is immediately followed by an address, article, aside, blockquote, div, dl, fieldset, footer, form, h1, h2, h3, h4, h5, h6, header, hgroup, hr, main, nav, ol, p, pre, section, table, or ul, element, or if there is no more content in the parent element and the parent element is not an a element.

And similar for li: https://www.w3.org/TR/html5/grouping-content.html#the-li-element
So I think the cache validator is correct and we may need to fix htmlparser.js to get the proper nesting for the first example (the amp-sidebar is a direct child of the body, but the parser thinks it's a child of the first p tag).
Thank you for the test case!

@ericlindley-g ericlindley-g added this to the Backlog milestone Aug 31, 2016
@Gregable Gregable mentioned this issue Sep 13, 2016
Merged
@Gregable
Copy link
Member

This is fixed everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Gregable Gregable

@powdercloud powdercloud

Labels
WG: caching
Projects
None yet
Milestone
Backlog Bugs
Development

No branches or pull requests
6 participants
@Gregable @dknecht @erwinmombay @honeybadgerdontcare @powdercloud @ericlindley-g