From 8100ff6be7cbe8c93f7abb9cf08cb2e28d9a3490 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 27 Feb 2020 01:46:23 -0600 Subject: [PATCH 01/11] Don't tree-shake allowed child classes of amp-date-picker 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. --- .../sanitizers/class-amp-style-sanitizer.php | 41 +++++++++++++++++++ tests/php/test-amp-style-sanitizer.php | 40 ++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 1b99ade22bc..bddd51d73d8 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/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 ) ) { + continue; + } + if ( ! isset( $this->used_class_names[ $class_name ] ) ) { return false; } @@ -755,6 +761,41 @@ private function has_used_attributes( $attribute_names ) { return true; } + /** + * Whether a given class is allowed to be styled in . + * + * That component has child classes that won't be present in the document yet. + * So get whether a class is an allowed child. + * + * @since 1.5.0 + * @link https://github.com/airbnb/react-dates/tree/05356/src/components + * + * @param string $class The name of the class to evaluate. + * @return bool Whether the class is allowed as a child of . + */ + private function is_class_allowed_in_amp_date_picker( $class ) { + // Some prefixes, some full class names. + $allowed_class_beginnings = [ + 'CalendarDay', + 'CalendarMonth', + 'DateInput', + 'DateRangePicker', + 'DayPicker', + 'KeyboardShortcutRow', + 'SingleDatePicker', + 'amp-date-picker-calendar-container', + 'amp-date-picker-resize-bug', + ]; + + foreach ( $allowed_class_beginnings as $allowed_class ) { + if ( 0 === strpos( $class, $allowed_class ) ) { + return true; + } + } + + return false; + } + /** * Run logic before any sanitizers are run. * diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 5e2bd74520a..3a1a5546549 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -624,6 +624,46 @@ static function( $preempt, $request, $url ) { */ public function get_amp_selector_data() { return [ + 'amp-date-picker-allowed-child-class-not-tree-shaken' => [ + '
', + 'amp-date-picker .CalendarMonth_caption{border-bottom: 10px} div amp-date-picker .amp-date-picker-selecting{margin-right:10px}', + 'amp-date-picker .CalendarMonth_caption{border-bottom:10px}div amp-date-picker .amp-date-picker-selecting{margin-right:10px}', // This class is an allowed child, so it shouldn't be removed. + ], + 'amp-date-picker-single-allowed-child-class-not-tree-shaken' => [ + '
', + '.DayPicker_weekHeaders {border-bottom: 10px}', + '.DayPicker_weekHeaders{border-bottom:10px}', + ], + 'amp-date-picker-allowed-container-child-class-not-tree-shaken' => [ + '
', + 'amp-date-picker .amp-date-picker-calendar-container{border-bottom: 10px} div amp-date-picker .amp-date-picker-selecting{margin-right:10px}', + 'amp-date-picker .amp-date-picker-calendar-container{border-bottom:10px}div amp-date-picker .amp-date-picker-selecting{margin-right:10px}', + ], + 'valid-class-wrapping-amp-date-picker-not-tree-shaken' => [ + '
', + '.foo-baz amp-date-picker .CalendarMonth_caption{border-bottom: 10px} div amp-date-picker .amp-date-picker-selecting{margin-right:10px}', + '.foo-baz amp-date-picker .CalendarMonth_caption{border-bottom:10px}div amp-date-picker .amp-date-picker-selecting{margin-right:10px}', + ], + 'amp-date-picker-valid-child-tree-shaken-if-component-not-in-document' => [ + '
AMP Date Picker Not Present
', + 'amp-date-picker .CalendarMonth_caption{border-bottom: 10px}', + '', + ], + 'amp-date-picker-similar-disallowed-child-class-tree-shaken' => [ + '
', + 'amp-date-picker .CalendarFoo {border-bottom: 10px}', + '', + ], + 'amp-date-picker-disallowed-child-class-tree-shaken' => [ + '
', + 'amp-date-picker .random-class{border-bottom: 10px}', + '', + ], + 'amp-date-picker-non-children-tree-shaken' => [ + '
', + 'amp-date-picker .CalendarMonth_caption{border-bottom: 10px} .unrelated{color:#fff}', + 'amp-date-picker .CalendarMonth_caption{border-bottom:10px}', // Non-children of the amp-date-picker should still be tree-shaken if they're not in the DOM. + ], 'img' => [ sprintf( '
', admin_url( 'images/wordpress-logo.png' ) ), 'div img.logo{border:solid 1px red}', From 28d037e1d92c5f1404e53adbaa951f237622ac75 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 27 Feb 2020 02:15:21 -0600 Subject: [PATCH 02/11] Check for exact class matches Some classes should have exact matches, like amp-date-picker-calendar-contrainer --- .../sanitizers/class-amp-style-sanitizer.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index bddd51d73d8..f9c99e77481 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -774,8 +774,7 @@ private function has_used_attributes( $attribute_names ) { * @return bool Whether the class is allowed as a child of . */ private function is_class_allowed_in_amp_date_picker( $class ) { - // Some prefixes, some full class names. - $allowed_class_beginnings = [ + $class_prefixes = [ 'CalendarDay', 'CalendarMonth', 'DateInput', @@ -783,12 +782,21 @@ private function is_class_allowed_in_amp_date_picker( $class ) { 'DayPicker', 'KeyboardShortcutRow', 'SingleDatePicker', + ]; + + foreach ( $class_prefixes as $prefix ) { + if ( 0 === strpos( $class, $prefix ) ) { + return true; + } + } + + $allowed_exact_classes = [ 'amp-date-picker-calendar-container', 'amp-date-picker-resize-bug', ]; - foreach ( $allowed_class_beginnings as $allowed_class ) { - if ( 0 === strpos( $class, $allowed_class ) ) { + foreach ( $allowed_exact_classes as $exact_class ) { + if ( $exact_class === $class ) { return true; } } From bd9313d462aca486423006824ff6725beac8df0d Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 27 Feb 2020 02:43:02 -0600 Subject: [PATCH 03/11] Use preg_match() to be more precise The component name is expected to be followed by a letter or _ For example, CalendarDay could be CalendarDay_table. Also, DayPicker could be DayPickerNavigation. --- includes/sanitizers/class-amp-style-sanitizer.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index f9c99e77481..d864d43ee70 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -784,10 +784,8 @@ private function is_class_allowed_in_amp_date_picker( $class ) { 'SingleDatePicker', ]; - foreach ( $class_prefixes as $prefix ) { - if ( 0 === strpos( $class, $prefix ) ) { - return true; - } + if ( preg_match( '#^(' . implode( '|', $class_prefixes ) . ')[a-zA-Z_]+#', $class ) ) { + return true; } $allowed_exact_classes = [ From 686e74252a28be71c77ba347321e1ed11802ef35 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 3 Mar 2020 12:12:01 -0600 Subject: [PATCH 04/11] Simplify foreach loop into in_array() call There's probably no need to iterate through the classses, this could simply return in_array(). --- includes/sanitizers/class-amp-style-sanitizer.php | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index d864d43ee70..5f8e3010180 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -788,18 +788,7 @@ private function is_class_allowed_in_amp_date_picker( $class ) { return true; } - $allowed_exact_classes = [ - 'amp-date-picker-calendar-container', - 'amp-date-picker-resize-bug', - ]; - - foreach ( $allowed_exact_classes as $exact_class ) { - if ( $exact_class === $class ) { - return true; - } - } - - return false; + return in_array( $class, [ 'amp-date-picker-calendar-container', 'amp-date-picker-resize-bug' ], true ); } /** From 36cba7d67a34f61ef97f0e93322ec00567251529 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 3 Mar 2020 18:25:53 -0600 Subject: [PATCH 05/11] Commit Weston's alternative to preg_match() Co-Authored-By: Weston Ruter --- includes/sanitizers/class-amp-style-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 5f8e3010180..b6d518325c0 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -784,7 +784,7 @@ private function is_class_allowed_in_amp_date_picker( $class ) { 'SingleDatePicker', ]; - if ( preg_match( '#^(' . implode( '|', $class_prefixes ) . ')[a-zA-Z_]+#', $class ) ) { + if ( in_array( strtok( $class, '_' ), $class_prefixes, true ) ) ) { return true; } From afab46f84a09c179e3add0d6a5f9aaae05394fef Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 3 Mar 2020 18:45:19 -0600 Subject: [PATCH 06/11] At least for now, revert "Commit Weston's alternative to preg_match()" This reverts commit 36cba7d67a34f61ef97f0e93322ec00567251529. --- includes/sanitizers/class-amp-style-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index b6d518325c0..5f8e3010180 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -784,7 +784,7 @@ private function is_class_allowed_in_amp_date_picker( $class ) { 'SingleDatePicker', ]; - if ( in_array( strtok( $class, '_' ), $class_prefixes, true ) ) ) { + if ( preg_match( '#^(' . implode( '|', $class_prefixes ) . ')[a-zA-Z_]+#', $class ) ) { return true; } From 5bffd65ecba76846919095ea883c93d6df20954a Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 3 Mar 2020 19:09:17 -0600 Subject: [PATCH 07/11] Commit Weston's suggestion, using full class names This now searches for something like DayPickerNavigation_, instead of just verifying that it starts with DayPicker. --- includes/sanitizers/class-amp-style-sanitizer.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 5f8e3010180..ef6172ae79d 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -777,14 +777,14 @@ private function is_class_allowed_in_amp_date_picker( $class ) { $class_prefixes = [ 'CalendarDay', 'CalendarMonth', - 'DateInput', - 'DateRangePicker', + 'CalendarMonthGrid', 'DayPicker', + 'DayPickerKeyboardShortcuts', + 'DayPickerNavigation', 'KeyboardShortcutRow', - 'SingleDatePicker', ]; - if ( preg_match( '#^(' . implode( '|', $class_prefixes ) . ')[a-zA-Z_]+#', $class ) ) { + if ( in_array( strtok( $class, '_' ), $class_prefixes, true ) ) { return true; } From eea565f5a78d441428e049073354b40ae94d6399 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 3 Mar 2020 19:16:26 -0600 Subject: [PATCH 08/11] Commit Weston's alternative to `in_array()` Co-Authored-By: Weston Ruter --- includes/sanitizers/class-amp-style-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index ef6172ae79d..89e1ecfecaa 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -788,7 +788,7 @@ private function is_class_allowed_in_amp_date_picker( $class ) { return true; } - return in_array( $class, [ 'amp-date-picker-calendar-container', 'amp-date-picker-resize-bug' ], true ); + return 'amp-date-picker-' === substr( $class, 0, 16 ); } /** From 01a8e8fc265543052b4ab1858236a12855c4bedd Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 4 Mar 2020 20:32:31 -0600 Subject: [PATCH 09/11] Make a variable static, to prevent recreating the array On Alain's suggestion, as this would have to be recreacted every time. --- includes/sanitizers/class-amp-style-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 89e1ecfecaa..e9e2b4be6bb 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -774,7 +774,7 @@ private function has_used_attributes( $attribute_names ) { * @return bool Whether the class is allowed as a child of . */ private function is_class_allowed_in_amp_date_picker( $class ) { - $class_prefixes = [ + static $class_prefixes = [ 'CalendarDay', 'CalendarMonth', 'CalendarMonthGrid', From b6f1a2360c84a82882f12c1f0f1126e4ee2a3450 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 14 Mar 2020 08:24:58 -0700 Subject: [PATCH 10/11] Optimize check for React Dates class names by checking if upper-case --- includes/sanitizers/class-amp-style-sanitizer.php | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index f21bcc7f95e..2216322d15d 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -665,7 +665,9 @@ 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 ) ) { + // The ctype_upper() check is an optimization since we know up front that all class names in React Dates are + // in CamelCase form, thus we can short-circut if the first character of the class name is not upper-case. + if ( ctype_upper( $class_name[0] ) && $this->has_used_tag_names( [ 'amp-date-picker' ] ) && $this->is_class_allowed_in_amp_date_picker( $class_name ) ) { continue; } @@ -785,11 +787,7 @@ private function is_class_allowed_in_amp_date_picker( $class ) { 'KeyboardShortcutRow', ]; - if ( in_array( strtok( $class, '_' ), $class_prefixes, true ) ) { - return true; - } - - return 'amp-date-picker-' === substr( $class, 0, 16 ); + return in_array( strtok( $class, '_' ), $class_prefixes, true ); } /** From 19507af1ab96650cb546e703c64e1fe3fe3e81ec Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 16 Mar 2020 09:44:52 -0700 Subject: [PATCH 11/11] Use elseif for amp-date-picker class name check --- includes/sanitizers/class-amp-style-sanitizer.php | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 2216322d15d..91198e09edf 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -661,13 +661,11 @@ private function has_used_class_name( $class_names ) { } continue 2; } - } - - // 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. - // The ctype_upper() check is an optimization since we know up front that all class names in React Dates are - // in CamelCase form, thus we can short-circut if the first character of the class name is not upper-case. - if ( ctype_upper( $class_name[0] ) && $this->has_used_tag_names( [ 'amp-date-picker' ] ) && $this->is_class_allowed_in_amp_date_picker( $class_name ) ) { + } elseif ( ctype_upper( $class_name[0] ) && $this->has_used_tag_names( [ 'amp-date-picker' ] ) && $this->is_class_allowed_in_amp_date_picker( $class_name ) ) { + // 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. + // The ctype_upper() check is an optimization since we know up front that all class names in React Dates are + // in CamelCase form, thus we can short-circut if the first character of the class name is not upper-case. continue; }