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

Unexpected page contents when previewing changes to state of invalid markup for a validation error #5369

Closed
westonruter opened this issue Sep 11, 2020 · 0 comments
Labels
Bug Something isn't working P2 Low priority WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

Bug Description

Here's the scenario:

In #5356 I'm working on fixing #771 which involves catching better handling of validation errors caused by including i-amphtml- in a stylesheet (instead of this causing the entire stylesheet to be removed, only the bad selector should be removed). I was trying to add this to the stylesheet and then seeing the behavior of forcibly keeping the invalid CSS. But when I changed the status from Removed to Kept and clicked Preview, I did not see the CSS being kept on the page. What's going on?

First of all, the validation error for a style element (currently) includes the full text of the element as one of the error properties (which probably shouldn't be done for amp-custom or amp-keyframes). Any change to the stylesheet causes a separate validation error to be created, since the validation error is identified by a hash of the error properties.

Secondly, I have Jetpack active locally, and it is configured to display the sharing buttons on the page. And herein lies the problem: Jetpack's Sharing module is configured to not show anything if is_preview() returns true, and when clicking the Preview button on the Validated URL screen, the URL includes the preview=1 query parameter. The result is different HTML is on the previewed page, and this results in different tree-shaken CSS, and thus different content for the style[amp-custom] element. This is unexpected.

In reality, I don't believe the inclusion of preview=1 when previewing a Validated URL serves any purpose. The fact that we want to preview changes to the validation error state is indicated by the presence of the validation_errors query param. Therefore, I believe that \AMP_Validation_Manager::override_validation_error_statuses() can be modified to remove the condition checking for isset( $_REQUEST['preview'] ):

isset( $_REQUEST['preview'] )
&&

In addition, clicking the Preview Changes button on the Validated URL screen needs to result in taking a user to the URL without the preview=1 query param:

image

Expected Behaviour

An AMP page should render with the same content whether previewing changes to the status of invalid markup or viewing the page on the frontend normally.

Steps to reproduce

  1. Enable Sharing module in Jetpack.
  2. Cause some validation error (e.g. via a script in a Custom HTML block)
  3. Validate the URL
  4. Toggle the status of the validation error from Removed to Kept.
  5. Click the Preview Changes button
  6. Notice the Sharing buttons are missing, when they are normally shown on an AMP page.

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 P2 Low priority labels Sep 11, 2020
@jwold jwold added the WS:Core Work stream for Plugin core label Sep 17, 2020
@westonruter westonruter closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working P2 Low priority WS:Core Work stream for Plugin core
Projects
Archived in project
Development

No branches or pull requests

2 participants