Don't tree-shake allowed child classes of `amp-date-picker` if it's in the DOM #4339

kienstra · 2020-02-27T08:00:28Z

Summary

Prevents tree-shaking of valid child classes of amp-date-picker, if that tag is in the document.

When adding the markup and styling that Weston shared to a Custom HTML block:

<style>
amp-date-picker .CalendarMonth_caption {  
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;
}
amp-date-picker .amp-date-picker-calendar-container .CalendarMonth_caption {
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;
}
.amp-date-picker-calendar-container .CalendarMonth_caption {
  background-image: linear-gradient(#b8b8b8, #5e5e5e);
  padding: 12px 0px;	
}
</style>
<amp-date-picker layout="fixed-height" height="360"></amp-date-picker>

...it's not tree-shaken:

Fixes #4334

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

Since this is JS-driven, child classes won't be in the document during tree-shaking. So if amp-date-picker is in the tags and the class is an allowed child of it, don't tree-shake it.

kienstra · 2020-02-27T08:05:09Z

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

+	private function is_class_allowed_in_amp_date_picker( $class ) {
+		// Some prefixes, some full class names.
+		$allowed_class_beginnings = [
+			'CalendarDay',


This might be missing some classes, though I tried to be thorough.

I looked at the components that have styles.

This doesn't include components that are just an .svg, like CalendarIcon.

Do any of these component names get carried over to be class names in the DOM? If so, they should be included in this list.

Yes, all of these appear as names in the DOM as classes, except for DateInput, DateRangePicker, DateRangePicker:

For those 3 that didn't appear, I couldn't find them, when using most of the snippets in the amp-date-picker documentation. I should probably remove those, though maybe I just missed a case where they are used.

The 3 that I couldn't find in the DOM as class names are removed in 5bffd65

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

Some classes should have exact matches, like amp-date-picker-calendar-contrainer

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

The component name is expected to be followed by a letter or _ For example, CalendarDay could be CalendarDay_table. Also, DayPicker could be DayPickerNavigation.

kienstra · 2020-02-27T08:55:17Z

Hi @westonruter,
No hurry, could you please review this when you have a chance? Thanks!

There's probably no need to iterate through the classses, this could simply return in_array().

westonruter · 2020-03-03T21:41:27Z

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

+			'SingleDatePicker',
+		];
+
+		if ( preg_match( '#^(' . implode( '|', $class_prefixes ) . ')[a-zA-Z_]+#', $class ) ) {


This can be made simpler/faster like so:

Suggested change

if ( preg_match( '#^(' . implode( '|', $class_prefixes ) . ')[a-zA-Z_]+#', $class ) ) {

if ( in_array( strtok( $class, '_' ), $class_prefixes, true ) ) ) {

Thanks, committed in 36cba7d

Reverted, then re-committed in 5bffd65

westonruter · 2020-03-03T21:43:06Z

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

+			return true;
+		}
+
+		return in_array( $class, [ 'amp-date-picker-calendar-container', 'amp-date-picker-resize-bug' ], true );


Suggested change

return in_array( $class, [ 'amp-date-picker-calendar-container', 'amp-date-picker-resize-bug' ], true );

return 'amp-date-picker-' === substr( $class, 0, 16 );

Committed in eea565f

westonruter · 2020-03-03T21:44:15Z

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

+	private function is_class_allowed_in_amp_date_picker( $class ) {
+		// Some prefixes, some full class names.
+		$allowed_class_beginnings = [
+			'CalendarDay',


Do any of these component names get carried over to be class names in the DOM? If so, they should be included in this list.

Co-Authored-By: Weston Ruter <westonruter@google.com>

This reverts commit 36cba7d.

This now searches for something like DayPickerNavigation_, instead of just verifying that it starts with DayPicker.

Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter · 2020-03-04T01:50:50Z

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

+			'CalendarDay',
+			'CalendarMonth',
+			'CalendarMonthGrid',
+			'DayPicker',
+			'DayPickerKeyboardShortcuts',
+			'DayPickerNavigation',
+			'KeyboardShortcutRow',


I'm using this function in the console:

function gatherClassNames( elements ) { const classNames = new Set(); const add = ( className ) => { if ( !/^i-amphtml-/.test( className ) ) { classNames.add( className.replace( /_.+/, '' ) ); } }; for ( const element of elements ) { element.classList.forEach( add ); gatherClassNames( element.children ).forEach( add ); } return classNames; }

And I'm invoking it via:

gatherClassNames(document.querySelectorAll('amp-date-picker'))

I'm not able to find any other class names in use when looking at the amp-date-picker examples.

Thanks, great function to get the class names.

The KeyboardShortcutRow class only appears on clicking the ? icon in the bottom right. But so far, I couldn't find more classes that appear via user interaction.

westonruter · 2020-03-04T01:57:18Z

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

+			return true;
+		}
+
+		return 'amp-date-picker-' === substr( $class, 0, 16 );


Actually, is this not redundant with:

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

Lines 650 to 655 in eea565f

// Class names for amp-date-picker, see <https://www.ampproject.org/docs/reference/components/amp-date-picker>.

case 'amp-date-picker-':

if ( ! $this->has_used_tag_names( [ 'amp-date-picker' ] ) ) {

return false;

}

continue 2;

Good point, I think it is redundant.

What do you think of deleting that snippet you shared?

Otherwise, it might be confusing if that's there, and there's a call below of ->is_class_allowed_in_amp_date_picker()

I'd defer to @schlessera for his recommendation, since he highly optimized this method.

I'd probably rather change this method to:

return in_array( strtok( $class, '_' ), $class_prefixes, true );

Indeed, as explained in my comment above, the logic needs to be integrated into the switch statement that already has the datepicker included, otherwise this is getting a big performance hit again.

This would then take care of the 'amp-date-picker-' === substr( $class, 0, 16 ) bit, which would be a given once you enter here.

Keep in mind that this logic runs 20k to 30k times on a single request for a regular page. It is important to keep this highly optimized when making changes.

@schlessera what do you think of this approach: b6f1a23?

@westonruter See b6f1a23#r37853546

schlessera · 2020-03-04T06:47:14Z

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

@@ -662,6 +662,12 @@ private function has_used_class_name( $class_names ) {
 				}
 			}

+			// If the document has an amp-date-picker tag, check if this class is an allowed child of it.
+			// That component's child classes won't be present yet in the document, so prevent tree-shaking valid classes.
+			if ( $this->has_used_tag_names( [ 'amp-date-picker' ] ) && $this->is_class_allowed_in_amp_date_picker( $class_name ) ) {


This needs to be integrated into the switch logic for the datepicker above that starts at line 651. Otherwise, we will always run the optimized loop, and then always run another check for the datepicker, suddenly making the entire loop work twice as much (and thus much slower).

Thanks, good point that this would be a big performance cost always running this other check.

Maybe I'm not understanding your suggestion.

But on moving this into the switch for the datepicker at line 651:`

diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index e9e2b4be6..5f807791a 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -649,7 +649,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { switch ( substr( $class_name, 0, 16 ) ) { // Class names for amp-date-picker, see <https://www.ampproject.org/docs/reference/components/amp-date-picker>. case 'amp-date-picker-': - if ( ! $this->has_used_tag_names( [ 'amp-date-picker' ] ) ) { + if ( ! $this->has_used_tag_names( [ 'amp-date-picker' ] ) || ! $this->is_class_allowed_in_amp_date_picker( $class_name ) ) { // Though this conditional will probably never be false. return false; } continue 2; @@ -662,12 +662,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { } } - // If the document has an amp-date-picker tag, check if this class is an allowed child of it. - // That component's child classes won't be present yet in the document, so prevent tree-shaking valid classes. - if ( $this->has_used_tag_names( [ 'amp-date-picker' ] ) && $this->is_class_allowed_in_amp_date_picker( $class_name ) ) { - continue; - } - if ( ! isset( $this->used_class_names[ $class_name ] ) ) { return false; }

...this won't allow the classes in is_class_allowed_in_amp_date_picker()

static $class_prefixes = [ 'CalendarDay', 'CalendarMonth', 'CalendarMonthGrid', 'DayPicker', 'DayPickerKeyboardShortcuts', 'DayPickerNavigation', 'KeyboardShortcutRow', ];

...as they don't begin with amp-date-picker-.

But maybe I'm not understanding your point. And your point about the performance cost is well taken.

Here's an example <amp-date-picker>

schlessera · 2020-03-04T06:49:51Z

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

+			return true;
+		}
+
+		return 'amp-date-picker-' === substr( $class, 0, 16 );


Indeed, as explained in my comment above, the logic needs to be integrated into the switch statement that already has the datepicker included, otherwise this is getting a big performance hit again.

This would then take care of the 'amp-date-picker-' === substr( $class, 0, 16 ) bit, which would be a given once you enter here.

Keep in mind that this logic runs 20k to 30k times on a single request for a regular page. It is important to keep this highly optimized when making changes.

schlessera · 2020-03-04T06:50:42Z

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

+	 * @return bool Whether the class is allowed as a child of <amp-date-picker>.
+	 */
+	private function is_class_allowed_in_amp_date_picker( $class ) {
+		$class_prefixes = [


This should be either a const array, or a static variable. Otherwise, you're creating and filling up a fresh new array every single time this method runs.

Sure, this is applied in 01a8e8f

On Alain's suggestion, as this would have to be recreacted every time.

…picker-tree-shaking * 'develop' of github.com:ampproject/amp-wp: (285 commits) Add PHPStan config files to libraries Remove erroneous unset Use US spelling analyze instead of analyse Remove static analysis step from common scripts section in travis.yml Fix missing unset Readd condition for running php-stan Raise PHPStan level in plugin to 2 Raise PHPStan level in plugin to 1 Fix conditional running of test steps Remove wrong psot-update action in ampproject/common again Better attempt at normalizing Composer file Normalize Composer file Update Composer lock file Run PHPStan in Travis Fix PHPCS issues Fix weird *NEVER* array unset error Fix stub sanitizer Typehint array being unset Skip analysing view template Fix bug in AMP_DOM_Utils referencing inexistent property ...

kienstra · 2020-03-16T18:30:05Z

Nice, thanks!

Don't tree-shake allowed child classes of amp-date-picker

8100ff6

Since this is JS-driven, child classes won't be in the document during tree-shaking. So if amp-date-picker is in the tags and the class is an allowed child of it, don't tree-shake it.

googlebot added the cla: yes Signed the Google CLA label Feb 27, 2020

kienstra commented Feb 27, 2020

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Show resolved Hide resolved

kienstra commented Feb 27, 2020

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved

Check for exact class matches

28d037e

Some classes should have exact matches, like amp-date-picker-calendar-contrainer

kienstra commented Feb 27, 2020

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved

Use preg_match() to be more precise

bd9313d

The component name is expected to be followed by a letter or _ For example, CalendarDay could be CalendarDay_table. Also, DayPicker could be DayPickerNavigation.

kienstra requested a review from westonruter February 27, 2020 08:55

Simplify foreach loop into in_array() call

686e742

There's probably no need to iterate through the classses, this could simply return in_array().

westonruter requested changes Mar 3, 2020

View reviewed changes

kienstra and others added 4 commits March 3, 2020 18:25

Commit Weston's alternative to preg_match()

36cba7d

Co-Authored-By: Weston Ruter <westonruter@google.com>

At least for now, revert "Commit Weston's alternative to preg_match()"

afab46f

This reverts commit 36cba7d.

Commit Weston's suggestion, using full class names

5bffd65

This now searches for something like DayPickerNavigation_, instead of just verifying that it starts with DayPicker.

Commit Weston's alternative to in_array()

eea565f

Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter reviewed Mar 4, 2020

View reviewed changes

schlessera requested changes Mar 4, 2020

View reviewed changes

kienstra and others added 3 commits March 4, 2020 20:32

Make a variable static, to prevent recreating the array

01a8e8f

On Alain's suggestion, as this would have to be recreacted every time.

Optimize check for React Dates class names by checking if upper-case

b6f1a23

westonruter requested a review from schlessera March 14, 2020 15:25

westonruter approved these changes Mar 14, 2020

View reviewed changes

Use elseif for amp-date-picker class name check

19507af

schlessera approved these changes Mar 16, 2020

View reviewed changes

westonruter merged commit c6b1328 into develop Mar 16, 2020

westonruter deleted the fix/date-picker-tree-shaking branch March 16, 2020 17:03

westonruter added this to the v1.5 milestone Mar 22, 2020

westonruter added the Changelogged label Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't tree-shake allowed child classes of `amp-date-picker` if it's in the DOM #4339

Don't tree-shake allowed child classes of `amp-date-picker` if it's in the DOM #4339

kienstra commented Feb 27, 2020 •

edited

Loading

kienstra Feb 27, 2020

westonruter Mar 3, 2020

kienstra Mar 4, 2020 •

edited

Loading

kienstra Mar 4, 2020

kienstra commented Feb 27, 2020 •

edited

Loading

westonruter Mar 3, 2020 •

edited

Loading

kienstra Mar 4, 2020 •

edited

Loading

kienstra Mar 4, 2020

westonruter Mar 3, 2020

kienstra Mar 4, 2020

westonruter Mar 3, 2020

westonruter Mar 4, 2020

kienstra Mar 4, 2020

kienstra Mar 4, 2020

westonruter Mar 4, 2020

kienstra Mar 4, 2020

westonruter Mar 4, 2020

schlessera Mar 4, 2020

westonruter Mar 14, 2020

schlessera Mar 16, 2020

schlessera Mar 4, 2020

kienstra Mar 5, 2020 •

edited

Loading

kienstra Mar 5, 2020

schlessera Mar 4, 2020

schlessera Mar 4, 2020

kienstra Mar 5, 2020

kienstra commented Mar 16, 2020

	if ( preg_match( '#^(' . implode( '\|', $class_prefixes ) . ')[a-zA-Z_]+#', $class ) ) {
	if ( in_array( strtok( $class, '_' ), $class_prefixes, true ) ) ) {

	return in_array( $class, [ 'amp-date-picker-calendar-container', 'amp-date-picker-resize-bug' ], true );
	return 'amp-date-picker-' === substr( $class, 0, 16 );

	// Class names for amp-date-picker, see <https://www.ampproject.org/docs/reference/components/amp-date-picker>.
	case 'amp-date-picker-':
	if ( ! $this->has_used_tag_names( [ 'amp-date-picker' ] ) ) {
	return false;
	}
	continue 2;

Don't tree-shake allowed child classes of amp-date-picker if it's in the DOM #4339

Don't tree-shake allowed child classes of amp-date-picker if it's in the DOM #4339

Conversation

kienstra commented Feb 27, 2020 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Feb 27, 2020 • edited Loading

westonruter Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

kienstra Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Mar 16, 2020

Don't tree-shake allowed child classes of `amp-date-picker` if it's in the DOM #4339

Don't tree-shake allowed child classes of `amp-date-picker` if it's in the DOM #4339

kienstra commented Feb 27, 2020 •

edited

Loading

kienstra Mar 4, 2020 •

edited

Loading

kienstra commented Feb 27, 2020 •

edited

Loading

westonruter Mar 3, 2020 •

edited

Loading

kienstra Mar 4, 2020 •

edited

Loading

kienstra Mar 5, 2020 •

edited

Loading