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

Ensure ampify_threaded_comments runs when in Strict sandboxing #6769

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Dec 7, 2021

Summary

Fixes #6443 (comment).

As discovered by @milindmore22, when a site is in the Strict level for Sandboxing, the ampification of threaded comments was failing to happen. This meant that the comment form lacked the expected on="submit-success:commentform.clear,AMP.setState({ampCommentThreading: {"replyTo":"","commentParent":"0"}})" attribute, and amp-bind was not being used for comment replies.

The issue is that in Strict mode, the script sanitizer will remove all scripts, including the comment-reply one:

'comment_reply_allowed' => 'never', // Can be 'never' , 'always', or 'conditionally'.

// Handle the comment-reply script, removing it if it's not never allowed, marking it as PX-verified if it is
// always allowed, or leaving it alone if it is 'conditionally' allowed since it will be dealt with later
// in the AMP_Comments_Sanitizer.
if ( $comment_reply_script ) {
if ( 'never' === $this->args['comment_reply_allowed'] ) {
$comment_reply_script->parentNode->removeChild( $comment_reply_script );
} elseif ( 'always' === $this->args['comment_reply_allowed'] ) {
// Prevent the comment-reply script from being removed later in the comments sanitizer.
ValidationExemption::mark_node_as_px_verified( $comment_reply_script );
}
}

However, the comments sanitizer was using the presence of this script to determine whether to ampify commenting:

$comment_reply_script = $this->get_comment_reply_script();
$should_ampify_comment_threading = false;
if ( $comment_reply_script instanceof Element ) {

So we just need to replace the false with a 'always' === $this->args['ampify_comment_threading'] condition so that even when the script is not on the page, the ampification will be done.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v2.2 milestone Dec 7, 2021
@westonruter westonruter added the Bug Something isn't working label Dec 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2021

Plugin builds for ce54a32 are ready 🛎️!

@westonruter
Copy link
Member Author

Please code review as part of QA.

@westonruter westonruter merged commit 3fbb1d0 into develop Dec 7, 2021
@westonruter westonruter deleted the fix/ampify-comment-threding-in-strict-level branch December 7, 2021 22:43
@milindmore22
Copy link
Collaborator

QA Passed
The fix resolved comment box not having on attribute in Sandboxing level Strict, Other Sandboxing levels, Moderate and loose also Looks good 👍🏼

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Sandboxing Experiment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce support for a more flexible AMP via Bento with levels of sandboxing enforcement
2 participants