Add stylesheet information to validated URL screen by westonruter · Pull Request #4026 · ampproject/amp-wp

westonruter · 2020-01-05T07:03:03Z

Summary

Add “CSS Usage” information to AMP admin bar menu item. The total percentage of the 50KB limit is shown, with a ⚠️ if the percentage is ≥80% and 🚫 if greater than 100%. Fixes Show notice on frontend when there is excessive CSS #1801.
CSS Usage information is also shown in the Validated URL post list table.
Add “Stylesheets” metabox to the validated URL screen which includes an expandable list of all stylesheets with details including;
- Original size
- Minified size
- Minification rate
- Relative size compared to the rest of the CSS on the page
- Priority (e.g. theme stylesheets are more important than plugin stylesheets)
- Status: 🚫 when excluded, ⚠️ when included but over limit (in Standard mode)
- Original markup which added the stylesheet.
- Sources, whether plugin, theme, or core. Full source stack available after expanding.
- CSS code with checkbox to reveal which code was removed after tree shaking.
The CSS manifest HTML comment is removed (since the validated URL screen has all the same information and more), thus the include_manifest_comment argument to the style sanitizer has been eliminated.
The format of the parsed CSS stylesheet is improved to strip braces from the stored strings and to store properties as an array rather than a serialized string.
The validation response data (also obtainable via wp amp validation check-url) includes the raw stylesheet information. To facilitate this, a new get_validate_response_data method has been added to the base sanitizer which is used during a validation response for sanitizers to augment the response with additional sanitizer-specific data (such as stylesheets).

Note 1: The work here was done within the architecture of the current codebase, which makes the implementation not as ideal as possible, especially for the newly added UI. This will be addressed when the UI is revamped to be more modern.

Note 2: The storage of the stylesheet data alongside the validation errors and the queried object are highlighting deficiencies of the amp_validated_url post type data format and the API of AMP_Validated_URL_Post_Type::store_validation_errors(). These will need to be addressed with a later refactor.

See screenshots in comments below.

Todo

Verify memory usage.
Ensure sources each appear on a separate line (missing display:block)
Trim needless braces from stored declaration blocks.
For style attributes and other style rules, add explanation for why :not(#_) shows up so much.
Add meter or percentage usage column in Validated URL post list table.
Show stylesheets with warnings even when in standard mode, when they go over 50KB.
Add AMP admin bar item for CSS budget used along with notice (see Show notice on frontend when there is excessive CSS #1801); link to stylesheets metabox on validated URL screen.
Remove CSS manifest comment entirely since obsolete.
Ensure that all information for the stylesheet is presented in the table, including the handle (and ideally the dependencies).

For Later

Fix attribution of inline styles added via wp_add_inline_style(), like for Twenty Twenty. See Scripts and styles enqueued before wp_enqueue_scripts fail to get sources identified #4133.
Stylesheets enqueued at init do not get sourcing information. Stylesheets registered at wp_default_styles likewise are not identified. See Scripts and styles enqueued before wp_enqueue_scripts fail to get sources identified #4133.
Add usage percentage to WP-CLI output. (Also change percentage to indicate new status or kept status.)

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

westonruter · 2020-01-08T01:25:10Z

Latest changes:

Showing what CSS was removed during tree-shaking ant what CSS remains:

Showing where a stylesheet came from (even when it didn't cause an excessive CSS error):

…e_document method Add tests for AMP_Content_Sanitizer, removing obsolete AMP_Test_World_Sanitizer

…alidation_Manager::get_validate_response_data()

…a to validate responses

…heet

…udget but still included Also improve message

westonruter · 2020-01-19T02:35:45Z

includes/validation/class-amp-validated-url-post-type.php

+						<?php
+						$origin_abbr_text = '?';
+						if ( 'link_element' === $stylesheet['origin'] ) {
+							$origin_abbr_text = '<link&nbsp;&hellip;>'; // @todo Consider adding the basename of the CSS file.


Including the handle would be useful as well.

pierlon · 2020-01-21T07:19:40Z

assets/css/src/amp-validation-error-taxonomy.css

+.shaken-stylesheet {
+	display: block;
+	white-space: pre-wrap;
+	overflow-x: scroll;


Is this needed? In my (brief) testing I was not able to find a stylesheet that overflowed the edges, and instead rendered a disabled horizontal scroll bar and a vertical scroll bar:

I'm not seeing that horizontal scrollbar in either Chrome or Firefox.

You're right, though. This should not be necessary because of white-space: pre-wrap; and also because each property is now on a separate line after e0055b5.

Removed in da85b8f.

…wsers

westonruter · 2020-01-24T00:38:20Z

FYI: I used the branch for this PR to do the analysis of all themes on WordPress.org.

kienstra

Looks good!

Hi @westonruter,
This is a huge improvement, having a UI for this. The percentage in the admin bar is great, also.

There's only one comment here related to logic. The rest are UI-related.

kienstra · 2020-01-24T07:26:16Z

includes/class-amp-theme-support.php

 		}

-		$assets = AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args );
+		$sanitization_results = AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args );


Nice renaming

kienstra · 2020-01-24T07:43:41Z

includes/validation/class-amp-validated-url-post-type.php

+		if ( empty( $stylesheets ) ) {
+			printf(
+				'<p><em>%s</em></p>',
+				esc_html__( 'No stylesheet data available. Please try re-checking this URL.', 'amp' )


Nice handling of this case:

kienstra · 2020-01-24T07:47:47Z

includes/validation/class-amp-validated-url-post-type.php

+								<?php AMP_Validation_Error_Taxonomy::render_sources( $stylesheet['sources'] ); ?>
+							</dd>
+
+							<dt><?php esc_html_e( 'CSS Code', 'amp' ); ?></dt>


You've probably considered this, but this section can have a lot of code, like from twentynineteen-style-css:

Maybe a <details><summary>, similar to 'Sources'?

I'm not seeing that. Can you please try clearing/disabling your cache?

Here's what I see:

Sorry, it looks fine after a hard refresh:

kienstra · 2020-01-24T07:53:57Z

includes/validation/class-amp-validated-url-post-type.php

+									if ( $del_count > 0 ) {
+										printf(
+											'<p><label><input type="checkbox" class="show-removed-styles"> %s</label></p>',
+											esc_html__( 'Show styles removed during tree-shaking', 'amp' )


You should be seeing this:

Only when the checkbox is checked should the styles be displayed:

Please try disabling/clearing cache.

Sorry, you're right that it looks fine after a hard refresh:

kienstra · 2020-01-24T08:02:49Z

assets/src/amp-validation/amp-validated-url-post-edit-screen.js

+		const showRemovedStylesCheckbox = stylesheetDetailsElements.querySelector( '.show-removed-styles' );
+		if ( showRemovedStylesCheckbox ) {
+			showRemovedStylesCheckbox.addEventListener( 'click', () => {
+				shakenStylesheetContainer.classList.toggle( 'removed-styles-shown', showRemovedStylesCheckbox.checked );


This doesn't look to be working in my local:

I didn't see any console error.

This is working fine now with a hard refresh.

kienstra · 2020-01-24T08:06:58Z

includes/validation/class-amp-validated-url-post-type.php

+							<dt><?php esc_html_e( 'Original Markup', 'amp' ); ?></dt>
+							<dd><code class="stylesheet-origin-markup"><?php echo esc_html( $origin_html ); ?></code></dd>
+
+							<dt><?php esc_html_e( 'Sources', 'amp' ); ?></dt>


This looks really good:

kienstra · 2020-01-24T08:15:41Z

assets/css/src/amp-validation-error-taxonomy.css

+	font-weight: bold;
+	text-align: right;
+}
+


Maybe adding something like this would help:

.amp-stylesheet-list th { width: 15%; }

Without it, the columns are pretty narrow on a 16" display:

The stylesheet actually already has:

Please try clearing/disabling cache and check again.

Ah, sorry. The styling looks fine with a hard refresh:

kienstra · 2020-01-24T08:24:12Z

includes/sanitizers/class-amp-style-sanitizer.php

+		}
+
+		$css_usage_percentage = ceil( ( $total_size / $this->style_custom_cdata_spec['max_bytes'] ) * 100 );
+		$menu_item_text       = __( 'CSS Usage', 'amp' ) . ': ';


includes/validation/class-amp-validated-url-post-type.php

pierlon · 2020-01-24T09:11:41Z

includes/validation/class-amp-validated-url-post-type.php

+						</button>
+					</td>
+					<td class="column-stylesheet_order">
+						<?php echo (int) $row; ?>


$row is already an int here.

Yes. This is done purely as a way to explicitly mark the variable as safe for printing. So it's an alternative to doing esc_html().

Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

amedina

In the "Stylesheets" section part of the Validate URL screen, the status column can be seen as redundant. Since there is so much info on that table, I propose combining the Percentage column and the Status column into one. The semantics of the Status column can be conveyed by either just the color of the Percentage bar, or adding the Status symbol next to the Percentage bar.

amedina

Similarly, The Minified and Final Size columns can be combined, effectively removing another column and making the table less crowded. This can be done by Displaying Final Size (Minified %). For example: 220B (-40.5%).

<

amedina · 2020-01-24T18:49:08Z

In the "Stylesheets" section part of the Validate URL screen, the status column can be seen as redundant. Since there is so much info on that table, I propose combining the Percentage column and the Status column into one. The semantics of the Status column can be conveyed by either just the color of the Percentage bar, or adding the Status symbol next to the Percentage bar.

Discussed offline. These two columns play an important role for developers. We will keep them and will further the discussion to devise improvements afterwards.

amedina

Discussed comments offline. Ship it!

westonruter · 2020-01-25T07:22:49Z

I should have added more tests. See #4175.

westonruter added this to the v1.5 milestone Jan 5, 2020

googlebot added the cla: yes Signed the Google CLA label Jan 5, 2020

westonruter mentioned this pull request Jan 6, 2020

Further compress CSS by renaming long class names to short versions #1518

Closed

westonruter added 26 commits January 8, 2020 22:42

Pass back instantiated sanitizers from AMP_Content_Sanitizer::sanitiz…

61a8f65

…e_document method Add tests for AMP_Content_Sanitizer, removing obsolete AMP_Test_World_Sanitizer

Pass results of AMP_Content_Sanitizer::sanitize_document() into AMP_V…

e797fb2

…alidation_Manager::get_validate_response_data()

Remove unused pending_stylesheets key

8444e42

Introduce get_validate_response_data method on sanitizers to pass dat…

b06be31

…a to validate responses

Capture original_size of CSS before processing

f8e8d2b

Splice empty strings into array instead of removing them

dfbcbda

Avoid saving empty stylesheet parts

f4c8dbb

Advance outer loop to last token when removing empty at-rules

155031f

Prevent loss of tree-shaken selector data

82d19aa

Store hash with processed stylesheet; temporarily re-construct styles…

9673e49

…heet

Include import_front_matter in initial included_size

351e7be

Improve stylesheet sanitizer method names and terminology

939bc1d

Rename stylesheet array key to tokens

d883b55

Further improve terminology of variables

586d1df

Store the parse time with each parsed stylesheet; capture shake time

68cca45

Store shaken_tokens in pending_stylesheets, capture origin, and element

3979809

Use 'final_size' as array key instead of 'size'

e14c749

Make explicit that size is final in variable names

dc144ce

Augment validated URL screen with stylesheet details

9987f53

Ensure sources appear separately on each line

102a5ff

Rename 'Origin' column to 'Markup'

8770d26

Move expand button to first column

856cf01

Rename and move Delta column to Minified column

7ea970e

Add meter percentage

3250a82

Improve status column with symbol indicating warning when exceeding b…

e6a19f4

…udget but still included Also improve message

Add CSS information to admin bar

ebb0d07

westonruter mentioned this pull request Jan 18, 2020

Improve check-url command output #3929

Closed

3 tasks

Prevent JS error when show-removed-styles checkbox is omitted

32592b3

westonruter mentioned this pull request Jan 18, 2020

Scripts and styles enqueued before wp_enqueue_scripts fail to get sources identified #4133

Closed

westonruter marked this pull request as ready for review January 19, 2020 00:15

westonruter requested a review from pierlon January 19, 2020 00:16

westonruter commented Jan 19, 2020

View reviewed changes

westonruter mentioned this pull request Jan 19, 2020

Add source information for script/style dependencies and inline scripts/styles #4135

Merged

6 tasks

pierlon reviewed Jan 21, 2020

View reviewed changes

Remove unneeded overflow-x:scroll which caused scrollbars in some bro…

da85b8f

…wsers

westonruter requested review from kienstra and pierlon January 23, 2020 17:20

westonruter requested a review from amedina January 24, 2020 07:10

kienstra approved these changes Jan 24, 2020

View reviewed changes

pierlon reviewed Jan 24, 2020

View reviewed changes

Use static closure since $this is not needed

e51f4b3

Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

kienstra approved these changes Jan 24, 2020

View reviewed changes

amedina reviewed Jan 24, 2020

View reviewed changes

amedina approved these changes Jan 24, 2020

View reviewed changes

westonruter merged commit 50d76db into develop Jan 24, 2020

This was referenced Jan 24, 2020

I2I: Relax CSS Limit from 50000 bytes to 75000 bytes ampproject/amphtml#26466

Closed

Restore PHPUnit code coverage to 60%+ #4175

Closed

westonruter deleted the add/sanitizer-augmented-validate-response-data branch January 25, 2020 07:22

This was referenced Feb 11, 2020

Fix division by zero to calculate minification ratio when original size is zero #4275

Merged

Fix discrepancy when reporting CSS usage percentage #4283

Merged

westonruter added the Changelogged label Mar 23, 2020

Conversation

westonruter commented Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Todo

For Later

Checklist

Uh oh!

westonruter commented Jan 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jan 24, 2020

Uh oh!

kienstra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amedina left a comment

Choose a reason for hiding this comment

Uh oh!

amedina left a comment

Choose a reason for hiding this comment

Uh oh!

amedina commented Jan 24, 2020

Uh oh!

amedina left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jan 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

westonruter commented Jan 5, 2020 •

edited

Loading

kienstra left a comment •

edited

Loading

kienstra Jan 24, 2020 •

edited

Loading