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

Integrate admin bar with AMP dev mode #3187

Merged
merged 23 commits into from Sep 11, 2019

Conversation

@westonruter
Copy link
Member

commented Sep 5, 2019

See full writeup: Integrating with AMP Dev Mode in WordPress.

  • Skip sanitizing admin bar (and removing it) in favor of ignoring (in dev mode).
  • Introduce amp_is_dev_mode() function along with an amp_dev_mode_enabled filter; this returns true by default when the user is logged-in and the admin bar is showing.
  • Introduce an AMP_Dev_Mode_Sanitizer filter which is registered among the sanitizers when amp_is_dev_mode() returns true. Takes as an argument an array of XPath expressions for elements to add data-ampdevmode to; this array can be filtered via the amp_dev_mode_element_xpaths hook. When the admin bar is shown, it will at least include the XPath expressions for excluding admin bar.
  • Eliminate need for keeping forked version of admin-bar.css in plugin.
  • Automatically add data-ampdevmode attributes to scripts and styles that depend on the admin bar, excluding them from being sanitized out of the document. This includes Dashicons, when the stylesheet is not being enqueued as part of the admin bar.
  • Skip processing stylesheets and converting elements that have data-ampdevmode attributes.

Fixes #1921.

Reverts or partially-undoes #2346, #2972, #2500, #2405.

Integrates with Dev Mode introduced in ampproject/amphtml#20974 via #3084.

Before

When themes have too much CSS for the admin bar to fit, the admin bar was removed from the page:

image

When there was not too much CSS, the admin bar was included in post-processing, converting all elements in the admin bar to AMP (including the Gravatar to amp-img) and processing/tree-shaking the admin-bar.css and dashicons.css; the admin-bar.js was dequeued and any plugins that tried to add JS to put interactivity in the admin bar were blocked as validation errors:

image

After

The admin bar element tree, the admin bar stylesheets (admin-bar.css, its inline style, and dashicons.css if not directly enqueued by a theme/plugin) will all be marked with the dev-ampdevmode attribute, in addition to this the data-ampdevmode attribute being added to the root element. The AMP validator extension will show one single error related to dev mode, but the extension will soon be updated to change this from an error badge to some other indicator specific dev mode:

image

The root HTML element, the admin bar element tree, the styles for the admin bar (admin-bar.css, inline style, dashicons.css), and now even the admin-bar.js all get the data-ampdevmode attribute added and thus are excluded from sanitization:

image

Now plugins can exempt JS administrative functionality in the admin bar from AMP validation. For example, with Automattic/jetpack#13450 the Notifications module in Jetpack now works on AMP pages:

image

The required scripts and styles all get the data-ampdevmode attribute:

image

Even when there is excessive CSS on the page, the admin bar will still be included:

image

Todo

  • When admin bar is showing, add data-ampdevmode to the html element.
  • Ensure that data-ampdevmode is added to all admin bar assets, both scripts and styles, and any related dependencies.
  • Exclude processing stylesheets that have data-ampdevmode attributes.
  • Remove #wpadminbar from the page prior to running the sanitizers, and then restore it at the end, but with data-ampdevmode attributes being added to every element in the tree.
  • Add #wpadminbar to dynamic_element_selectors when admin bar is shown. May require #1478.
  • Add filter for controlling whether dev mode is enabled.
  • Add filter for the XPath queries for elements to decorate with data-ampdevmode attributes.
  • Make dev-mode for admin bar conditional based on user being authenticated, since the admin bar can technically be forced for unauthenticated users? ( is_user_logged_in() && is_admin_bar_showing() ) But then we'd still need the forked admin bar CSS. In reality, only a very very small number of sites would force the admin bar to be shown for unauthenticated users, so this is probably not worth worrying about. But perhaps we should still keep in place the stylesheet prioritization logic.
  • Add support for declaring certain script/style handles as being for dev mode? (Maybe later.)
  • Make use of data-ampdevmode to allow Customizer scripts and styles in the preview, as opposed to the sanitizer args allow_dirty_styles and allow_dirty_scripts. (To be investigated later; Customizer integration is less important than the admin bar, and the current approach of just allowing all scripts in the Customizer preview works fine.)
  • Skip converting elements that have data-ampdevmode?
  • Skip processing style attributes on elements that have data-ampdevmode?
  • Add tests.
  • Try implementing support in Site Kit.
  • Try implementing support in Jetpack.
  • Implement support in Pantheon HUD.
  • Implement support in WordPress SEO.

Build for testing: amp.zip - v1.3-alpha-20190910T210727Z-e15bbe33

westonruter added 2 commits Sep 4, 2019
This reverts commit d83bb25 from #2500.
…dev mode)

* Eliminate need for keeping forked version of admin-bar.css in plugin.
* Automatically add data-ampdevmode attributes to scripts and styles that depend on the admin bar, excluding them from being sanitized out of the document.
* Skip processing stylesheets that have data-ampdevmode attributes.
@westonruter westonruter added this to the v1.3 milestone Sep 5, 2019
@googlebot googlebot added the cla: yes label Sep 5, 2019
westonruter added 6 commits Sep 6, 2019
Also add missing dynamic_element_selectors arg to stylesheet cache key.
…s for dev mode elements
…y for unauthenticated admin bars
@westonruter westonruter changed the title Integrate admin bar and Customizer with AMP dev mode Integrate admin bar with AMP dev mode Sep 7, 2019
westonruter added 2 commits Sep 7, 2019
Refactor methods to improve readability and organization
Add todo comments for where dev mode should be used instead of allow_dirty_*.
Make closures static.
@westonruter westonruter requested a review from schlessera Sep 7, 2019
@westonruter westonruter marked this pull request as ready for review Sep 7, 2019
@westonruter westonruter marked this pull request as ready for review Sep 7, 2019
@westonruter

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

I'm going to add some more tests, but the scope of this I believe is complete.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

I'm also going to open PRs for Jetpack and Site Kit to ensure the API allows for the plugins to successfully exempt admin bar integrations from AMP validation.

Copy link
Collaborator

left a comment

Some typos in comments and a few optional remarks from my side.

@@ -233,6 +233,17 @@ function() {
add_action( 'amp_story_head', 'wp_site_icon', 99 );
add_action( 'amp_story_head', 'wp_oembed_add_discovery_links' );
// Disable admin bar from even trying to be output, since wp_head and wp_footer hooks are not on the template.

This comment has been minimized.

Copy link
@schlessera

schlessera Sep 9, 2019

Collaborator

Would it make sense (in a later PR) to hook the admin bar into 'amp_story_head'/'amp_story_footer' ?

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 9, 2019

Author Member

That's a good question. I'm not sure if the styles used in AMP Stories would even be compatible with the styles used for the admin bar. Since AMP Stories are a full-screen experience, I suspect that the admin bar would clash. But it would be something useful to investigate at a later time, yes.

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
westonruter and others added 6 commits Sep 9, 2019
Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
…ode-support

* 'develop' of github.com:ampproject/amp-wp: (28 commits)
  Exclude development files from production build ZIPs
  Add filter to allow more video types in AMP story background se… (#3171)
  Update dependency @babel/cli to v7.6.0 (#3203)
  Update dependency @babel/core to v7.6.0 (#3204)
  Remove deprecated AMP_WP_Utils class
  Refresh Composer lock file
  Run build in a temporary folder
  Add shell script to clean up the current folder and then trigger a build
  Adapt Gruntfile to fetch optimized Composer dependencies within the build folder
  Retrieve Sabberworm patch directly from the GitHub PR
  Adding regression test for vertical text alignment issues
  Fixing minor errors in test descriptions
  Only restrict height inside non-fit text blocks
  Rewritten block size tests
  Limit height declaration to only direct descendant
  Remove now obsolete withEnforcedVideoUploadType HOC
  Update eslint-plugin-jest and apply fixes (#3192)
  Update dependency postcss to v7.0.18 (#3200)
  Update dependency terser-webpack-plugin to v2 (#3195)
  Update dependency webpack-cli to v3.3.8 (#3194)
  ...
westonruter added 2 commits Sep 10, 2019
@westonruter westonruter requested a review from schlessera Sep 10, 2019
@@ -899,6 +935,33 @@ function amp_get_content_sanitizers( $post = null ) {
*/
$sanitizers = apply_filters( 'amp_content_sanitizers', $sanitizers, $post );
if ( amp_is_dev_mode() ) {

This comment has been minimized.

Copy link
@schlessera

schlessera Sep 10, 2019

Collaborator

I think it would be preferable to have this piece of logic be part of the AMP_Dev_Mode_Sanitizer class instead, and then make that sanitizer decide when it acts or not, instead of hardcoding it in here.

It should be the responsibility of the AMP_Dev_Mode_Sanitizer to know when to do something and what to do, not some helper function collection.

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 10, 2019

Author Member

I did think about that, but I think it's preferable here because this function is inherently connected directly with WordPress as it applies a filter. There are two reasons for why this should not be in the sanitizer:

  1. It is important that the array of sanitizer args returned by amp_get_content_sanitizers() be deterministic in what the resulting sanitized document be. In other words, we cannot apply filters inside of a sanitizer because then we cannot use the sanitizer args as a source of truth for computing a cache key for the post-processed output, as we can now.
  2. We should be trending the sanitizers toward being pure PHP classes that are do not connect with WordPress APIs (especially the plugin API per above) as this is the direction needed for abstracting the sanitizers onto a CMS-agnostic package.

Additionally, this amp_is_dev_mode() function would be useful outside of the context of a sanitizer. For example, a theme/plugin may want to call this function during template generation to determine whether it can output some dev code (e.g. Query Monitor).

The amp_is_dev_mode() is gatekeeping whether or not the AMP_Dev_Mode_Sanitizer should be used in the first place. So it is operating at a higher level.

Aside: should it rather be named is_amp_dev_mode()?

This comment has been minimized.

Copy link
@schlessera

schlessera Sep 10, 2019

Collaborator

Aside: should it rather be named is_amp_dev_mode()?

I'm seeing the amp_ as a prefix here, as this could just as well be used outside of the AMP plugin, so I think the name is fine.

tests/php/test-amp-helper-functions.php Show resolved Hide resolved
tests/php/test-amp-helper-functions.php Show resolved Hide resolved
@westonruter

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Just found an issue. When the theme has a lot of CSS and the admin bar is showing up, the admin-bar.css could be included but dashicons.css could get excluded, resulting in:

image

We should probably mark the link element for loading Dashicons with data-ampdevmode, though there is a chance that a theme/plugin is enqueueing it for use on the frontend for use on pages even without the admin bar.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

What comes to mind is that we could mark Dashicons as being in dev mode only if it was not directly enqueued or a dependency of any enqueued stylesheet other than admin-bar.

westonruter added 2 commits Sep 10, 2019
…min bar
@westonruter westonruter requested a review from schlessera Sep 10, 2019
@westonruter westonruter referenced this pull request Sep 10, 2019
2 of 5 tasks complete
schlessera added 2 commits Sep 11, 2019
…cons
@googlebot

This comment has been minimized.

Copy link

commented Sep 11, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 11, 2019
@schlessera

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2019

@googlebot I consent.

@googlebot

This comment has been minimized.

Copy link

commented Sep 11, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 11, 2019
@westonruter westonruter merged commit dd5a130 into develop Sep 11, 2019
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@westonruter westonruter deleted the add/dev-mode-support branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.