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

Failure to normalize documents with HTML comments after </body> #4282

Closed
westonruter opened this issue Feb 12, 2020 · 0 comments · Fixed by #4297
Closed

Failure to normalize documents with HTML comments after </body> #4282

westonruter opened this issue Feb 12, 2020 · 0 comments · Fixed by #4297
Assignees
@schlessera
Labels
Bug Something isn't working P0 High priority
Milestone
v1.5

Comments

@westonruter
Copy link
Member

Bug Description

When markup appears after </body> or </html>, the document's original <body> is lost.
There is a bug in \Amp\AmpWP\Dom\Document::normalize_document_structure() in particular with the pattern in \Amp\AmpWP\Dom\Document::HTML_STRUCTURE_BODY_END_TAG as used here:

amp-wp/src/Dom/Document.php

Lines 492 to 497 in 2026afc

} elseif ( ! preg_match( self::HTML_STRUCTURE_BODY_END_TAG, $content, $matches ) ) {
// Only <body> missing.
// @todo This is an expensive regex operation, look into further optimization.
$content = preg_replace( self::HTML_STRUCTURE_HEAD_TAG, '$0<body>', $content, 1 );
$content .= '</body>';
}

This is causing new <body> to be added to the page which is blowing away the existing one.

Expected Behaviour

The original <body> with all of its valid attributes should not be removed during normalization when there is markup or HTML comments appearing after </body> or </html>. Any markup appearing after </body> should get moved inside during normalization, although beware of moving HTML comment nodes as this may cause problems with validation. This came up before previously in #4104.

Steps to reproduce

Modify the footer.php of Twenty Twenty to end with:

	</body>
</html>
<!-- a comment! -->

or

	</body>
<!-- a comment! -->
</html>

When then look at an AMP page in Standard/Transitional mode. Notice that the body element of the page is now just:

<body id="body">

Whereas it should be:

<body class="home blog logged-in admin-bar enable-search-modal has-no-pagination showing-comments show-avatars footer-top-visible customize-support" id="body">

Screenshots

Additional context

  • WordPress version:
  • Plugin version:
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode:
  • PHP version:
  • OS:
  • Browser: [e.g. chrome, safari]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry
@westonruter westonruter added Bug Something isn't working P0 High priority labels Feb 12, 2020
@westonruter westonruter added this to the v1.5 milestone Feb 12, 2020
@kmyram kmyram added Size: S and removed Size: XS labels Feb 13, 2020
@schlessera schlessera mentioned this issue Feb 15, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@schlessera schlessera

Labels
Bug Something isn't working P0 High priority
Projects
None yet
Milestone
v1.5
Development

Successfully merging a pull request may close this issue.

Support content after HTML end tag ampproject/amp-wp
3 participants
@schlessera @westonruter @kmyram