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

Fix AMP violations caused by AMP dev mode regression #1142

Closed
felixarntz opened this issue Feb 18, 2020 · 2 comments
Closed

Fix AMP violations caused by AMP dev mode regression #1142

felixarntz opened this issue Feb 18, 2020 · 2 comments
Labels
P0 High priority Type: Bug Something isn't working
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Feb 18, 2020

Bug Description

#505 introduced support for AMP dev mode so that Site Kit scripts and stylesheets are not considered AMP violations by the plugin, since they are only loaded when the user is logged in anyway and should continue to work in these circumstances.

There was a regression recently (reported multiple times) by no longer using wp_add_inline_script() to print the window.googlesitekit object definition and instead relying on WP_Scripts::add_data() (because this is the only way to make it work as a standalone script). Since this script is standalone, we need to add back a mechanism to highlight it as compatible with AMP dev mode. We can use a /*googlesitekit*/ comment for that like before (unfortunately modifying the rendered script tag is not possible).

Another issue is that the AMP plugin filter for that, amp_dev_mode_element_xpaths, fires rather early, but we only hook it in during wp_enqueue_scripts which is too late. It can be generally hooked in during init, there is nothing expensive in there anyway.


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

Acceptance criteria

  • There should not be any AMP violations flagged by the AMP plugin when the Site Kit admin bar menu is loaded.

Implementation Brief

QA Brief

  • Activate AMP plugin and set to AMP standard mode.
  • Go to a page in the frontend for which the Site Kit admin bar menu shows.
  • Ensure the plugin does not report any validation errors, and that the menu opens as expected when hovering over it.

Changelog entry

  • Fix AMP violations when user is logged in and Site Kit admin bar menu is active.
@felixarntz felixarntz added Type: Bug Something isn't working P0 High priority labels Feb 18, 2020
@felixarntz felixarntz added this to the Sprint 17 milestone Feb 18, 2020
@aaemnnosttv
Copy link
Collaborator

IB + CR ✅

@felixarntz felixarntz removed their assignment Feb 21, 2020
@aaemnnosttv aaemnnosttv self-assigned this Feb 24, 2020
@aaemnnosttv
Copy link
Collaborator

✅ QA

Using AMP v1.4.0 and Site Kit 1.3.0

  1. Confirmed validation errors on home page
    image
  2. Hover intent and Site Kit global data
    image
  3. ✅ Rechecking shows no validation errors using Site Kit 3b532c9
    image

Using AMP v1.4.4 (current latest) and Site Kit 1.3.0

  1. Confirmed validation errors on home page
    image
  2. Site Kit global data only
    image
  3. ✅ Rechecking shows no validation errors using Site Kit 3b532c9
    image

✅ Menu opens as expected on hover
image

Moving to approval

@aaemnnosttv aaemnnosttv removed their assignment Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants