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

Ignore admin bar for AMP validation purposes #1921

Closed
2 of 5 tasks
westonruter opened this issue Mar 2, 2019 · 16 comments
Closed
2 of 5 tasks

Ignore admin bar for AMP validation purposes #1921

westonruter opened this issue Mar 2, 2019 · 16 comments

Comments

@westonruter
Copy link
Member

@westonruter westonruter commented Mar 2, 2019

The inclusion of the admin bar was included in AMP via #1205. However, the amount of admin bar CSS is so large (20KB) that it takes up about 40% of the total allowed CSS in AMP (50KB). For this reason it very frequently causes excessive_css validation errors when an authenticated user is browsing the frontend. As opposed to providing this option or trying to automatically disable the admin bar when there is too much CSS (#1800), we could instead just exclude admin bar CSS (including any styles that depend on admin-bar) from being considered in the 50KB budget. This can mean that an authenticated user will often be served an invalid AMP page, but this is of little importance since the unauthenticated visitor (including crawlers) will only get the valid AMP pages for indexing. The admin bar has an indicator of AMP validity already, so users who have the AMP validator browser extension would just need to trust the admin bar status rather than the browser toolbar, or else open the URL in an incognito window to double-check. AMP now supports an explicit “dev mode” (ampproject/amphtml#20974) for when invalid markup is intentionally being included in the page. The AMP validator browser extension will explicitly be aware of this mode (ampproject/amphtml#24176) so there will not be concerns about it alerting the user of errors. Initial support for dev mode in the AMP plugin landed with #3084.

👉 For full list of changes, please refer to description in PR: #3187.

Done/Obsolete

  • Remove checkbox from settings screen to hide admin bar in AMP. If anyone wants to do this, they can easily do so via a show_admin_bar filter along with a check for ! is_amp_endpoint(). Most users will benefit from the admin bar. Decisions not options. (Done in #2346 & #2405.)
  • Suppress admin bar from being shown during validation requests. Done in #2500.
  • Add note in admin bar submenu for AMP that page is not valid due to admin bar, but otherwise it is OK.
  • Add link to AMP admin bar menu to navigate to the current URL without the admin bar. A query param like hide_admin_bar can be introduced. This URL can also automatically add #development=1.
  • Consider removing the amp attribute from the html element when the admin bar is showing to prevent the AMP Validator extension from screaming at the user.
@swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Mar 2, 2019

Sounds like a plan!

Loading

@westonruter westonruter added this to the v1.1 milestone Mar 14, 2019
@westonruter westonruter changed the title Exclude admin bar CSS from being included in amp-custom styles Ignore admin bar for AMP validation purposes Mar 15, 2019
@westonruter westonruter removed this from the v1.1 milestone Mar 23, 2019
@westonruter westonruter added this to the v1.2 milestone Mar 23, 2019
@westonruter
Copy link
Member Author

@westonruter westonruter commented May 15, 2019

For perhaps a better alternative to this, see #2322.

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented May 15, 2019

Also, this could be made obsolete especially for Native mode sites by #2326, as excessive CSS could be rejected from sanitization by default.

Loading

@felixarntz
Copy link
Collaborator

@felixarntz felixarntz commented May 15, 2019

I agree #2322 would be a better alternative - if there is excessive CSS, the admin bar CSS should be removed first. We would need to ensure though that we still keep the user informed about that. Also, what would happen to the admin bar markup (that would then appear broken)?

Loading

@swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented May 15, 2019

The suggestion here is to remove the admin bar as a whole in those cases, not just the admin bar CSS.

Loading

@amedina
Copy link
Member

@amedina amedina commented May 16, 2019

I agree. As a user I'd like to have the Admin bar in cases where there is no excess CSS since it provides easy access points that should be there if possible, and can automatically be disabled if needed. This way the prioritization of stylesheets would apply only for all the other site style sheets.

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented Jun 4, 2019

Something else that comes to mind here is that when performing a validation request, we should prevent the admin bar from being shown at all. In other words:

--- a/includes/validation/class-amp-validation-manager.php
+++ b/includes/validation/class-amp-validation-manager.php
@@ -212,6 +212,8 @@ class AMP_Validation_Manager {
 		}
 
 		if ( self::$should_locate_sources ) {
+			add_filter( 'show_admin_bar', '__return_false', PHP_INT_MAX );
+
 			self::add_validation_error_sourcing();
 		}
 	}

This would actually align the behavior of user-initiated validation requests with validation requests performed via wp amp validate-site, which makes requests as an unauthenticated user and thus the admin bar is not displayed.

With that in place, then what remains is to ignore validation errors caused by the admin bar when rendering the page outside the context of a validation error, and thus allowing the admin bar to be included even though otherwise it causes the page to be strictly invalid AMP.

Loading

@HelaGone
Copy link

@HelaGone HelaGone commented Aug 22, 2019

Hello all

What is the current status of this?

I've came to the point where my theme seems to have a lot of css and the #wpadminbar is being striped out.

Is there a way to keep showing it only for editors?

I've tried what @westonruter suggest in its first post here (show_admin_bar filter along with a check for ! is_amp_endpoint()). But no luck.

I'm using the 1.2.0 version

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented Aug 23, 2019

@HelaGone The status is what you have experienced. In #2346 the admin bar automatically gets removed when its CSS takes it over the 50KB limit.

We still have to decide exactly what to do in regards to allowing the admin bar take the user above the 50KB limit while logged in. This will likely relate closely to #2326.

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented Aug 23, 2019

We now have something in AMP tailored for this use case! There is a new data-ampdevmode attribute which can be added to any element that we know will be not valid AMP, and the validator will suppress validation errors about it: ampproject/amphtml#20974

I've inquired about having the AMP Validator extension account for this special case so that it doesn't show an error icon: ampproject/amphtml#24176.

So this means that when the user is logged-in and the admin bar is showing, we can take the total amount of tree-shaken CSS for the admin bar and increase the 50KB limit by that amount, while also adding data-ampdevmode to the root html element and also add this attribute to the style[amp-custom]. And the admin bar will no longer then be removed for a site that otherwise does not exceed the 50KB limit when the admin bar is not showing.

Loading

@westonruter westonruter removed this from the v1.3 milestone Aug 23, 2019
@westonruter westonruter added this to the v1.2.2 milestone Aug 23, 2019
@westonruter westonruter self-assigned this Aug 23, 2019
@westonruter westonruter added this to Backlog in Ongoing Aug 23, 2019
@westonruter
Copy link
Member Author

@westonruter westonruter commented Aug 23, 2019

The support for data-ampdevmode being added in #3084 should also be used to refactor the mechanism for how the sanitizer args for allow_dirty_styles and allow_dirty_scripts are handled.

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented Aug 23, 2019

Instead of passing the admin bar through through the sanitizers, now we can exempt it from sanitization (and adding data-ampdevmode attributes on its descendant elements as needed) and then also inject data-ampdevmode attributes on its stylesheets to exclude from processing. This could also extend to scripts that add UI to the admin bar (e.g. Site Kit). As long as they include the data-ampdevmode attribute, then they would not get removed.

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented Aug 24, 2019

Something else we also we can do here then is to eliminate bundling a forked admin-bar.css as well.

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented Sep 3, 2019

A good idea from @schlessera is that there should be a way for data-ampdevmode to mark an entire DOM subtree as having its validation errors ignored. This would avoid having to add the data-ampdevmode to each descendant element. This would require an update to AMP validator. Perhaps instead of data-ampdevmode being a boolean attribute, it could take an attribute value that defines the scope for ignoring the validation errors. Scope could be:

  • Exclude the element and its attributes only.
  • Exclude the element only.
  • Exclude certain attributes.
  • Exclude the entire tree from that element and below.

Loading

@westonruter
Copy link
Member Author

@westonruter westonruter commented Sep 11, 2019

Testing instructions:

  1. Activate the Twenty Nineteen theme.
  2. Create a post with a lot of blocks (e.g. via Block Unit Test plugin).
  3. Activate either Transitional or Standard modes, and access an AMP page.
  4. The admin bar should always be displayed while logged in regardless of how much CSS is on the page.
  5. The AMP validator extension in Chrome should only ever show one validation error related to the dev mode:

image

For more before/after screenshots for what to look for, see the description on #3187.

Loading

@westonruter westonruter moved this from Ready for Review to Ready for QA in Ongoing Sep 11, 2019
@westonruter westonruter assigned csossi and unassigned westonruter Sep 11, 2019
@csossi
Copy link

@csossi csossi commented Sep 12, 2019

Verified in QA
image

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants