Amp-wp: Ignore admin bar for AMP validation purposes

[westonruter]

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” (https://github.com/ampproject/amphtml/issues/20974) for when invalid markup is intentionally being included in the page. The AMP validator browser extension will explicitly be aware of this mode (https://github.com/ampproject/amphtml/issues/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 https://github.com/ampproject/amp-wp/pull/3084.

👉 For full list of changes, please refer to description in PR: https://github.com/ampproject/amp-wp/pull/3187.

Done/Obsolete

• [x] 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.)
• [x] 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]

Sounds like a plan!

[westonruter]

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

[westonruter]

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

[felixarntz]

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)?

[swissspidy]

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

[amedina]

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.

[westonruter]

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.

[HelaGone]

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

[westonruter]

@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.

[westonruter]

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: https://github.com/ampproject/amphtml/issues/20974

I've inquired about having the AMP Validator extension account for this special case so that it doesn't show an error icon: https://github.com/ampproject/amphtml/issues/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.

[westonruter]

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.

[westonruter]

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.

[westonruter]

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

[westonruter]

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.

[westonruter]

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:

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

[csossi]

Verified in QA