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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Mar 16, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8100ff6
Don't tree-shake allowed child classes of amp-date-picker
kienstra Feb 27, 2020
28d037e
Check for exact class matches
kienstra Feb 27, 2020
bd9313d
Use preg_match() to be more precise
kienstra Feb 27, 2020
686e742
Simplify foreach loop into in_array() call
kienstra Mar 3, 2020
36cba7d
Commit Weston's alternative to preg_match()
kienstra Mar 4, 2020
afab46f
At least for now, revert "Commit Weston's alternative to preg_match()"
kienstra Mar 4, 2020
5bffd65
Commit Weston's suggestion, using full class names
kienstra Mar 4, 2020
eea565f
Commit Weston's alternative to `in_array()`
kienstra Mar 4, 2020
01a8e8f
Make a variable static, to prevent recreating the array
kienstra Mar 5, 2020
d83354e
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/date-…
westonruter Mar 14, 2020
b6f1a23
Optimize check for React Dates class names by checking if upper-case
westonruter Mar 14, 2020
19507af
Use elseif for amp-date-picker class name check
westonruter Mar 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,12 @@ private function has_used_class_name( $class_names ) {
}
continue 2;
}
} 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;
}

if ( ! isset( $this->used_class_names[ $class_name ] ) ) {
Expand Down Expand Up @@ -756,6 +762,32 @@ private function has_used_attributes( $attribute_names ) {
return true;
}

/**
* Whether a given class is allowed to be styled in <amp-date-picker>.
*
* 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 <amp-date-picker>.
*/
private function is_class_allowed_in_amp_date_picker( $class ) {
static $class_prefixes = [
'CalendarDay',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@kienstra kienstra Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

amp-date-picker

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'CalendarMonth',
'CalendarMonthGrid',
'DayPicker',
kienstra marked this conversation as resolved.
Show resolved Hide resolved
'DayPickerKeyboardShortcuts',
'DayPickerNavigation',
'KeyboardShortcutRow',
Comment on lines +779 to +785
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great function to get the class names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

];

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

/**
* Run logic before any sanitizers are run.
*
Expand Down
40 changes: 40 additions & 0 deletions tests/php/test-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,46 @@ static function( $preempt, $request, $url ) {
*/
public function get_amp_selector_data() {
return [
'amp-date-picker-allowed-child-class-not-tree-shaken' => [
'<div><amp-date-picker id="baz" type="single" mode="overlay" layout="container" format="YYYY-MM-DD" src="/example.json" input-selector="#src-input"></amp-date-picker></div>',
'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' => [
'<div><amp-date-picker id="baz" type="single" mode="overlay" layout="container" format="YYYY-MM-DD" src="/example.json" input-selector="#src-input"></amp-date-picker></div>',
'.DayPicker_weekHeaders {border-bottom: 10px}',
'.DayPicker_weekHeaders{border-bottom:10px}',
],
'amp-date-picker-allowed-container-child-class-not-tree-shaken' => [
'<div><amp-date-picker id="baz" type="single" mode="overlay" layout="container" format="YYYY-MM-DD" src="/example.json" input-selector="#src-input"></amp-date-picker></div>',
'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' => [
'<div class="foo-baz"><amp-date-picker id="baz" type="single" mode="overlay" layout="container" format="YYYY-MM-DD" src="/example.json" input-selector="#src-input"></amp-date-picker></div>',
'.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' => [
'<div><span>AMP Date Picker Not Present</span></div>',
'amp-date-picker .CalendarMonth_caption{border-bottom: 10px}',
'',
],
'amp-date-picker-similar-disallowed-child-class-tree-shaken' => [
'<div><amp-date-picker id="baz" type="single" mode="overlay" layout="container" format="YYYY-MM-DD" src="/example.json" input-selector="#src-input"></amp-date-picker></div>',
'amp-date-picker .CalendarFoo {border-bottom: 10px}',
'',
],
'amp-date-picker-disallowed-child-class-tree-shaken' => [
'<div><amp-date-picker id="baz" type="single" mode="overlay" layout="container" format="YYYY-MM-DD" src="/example.json" input-selector="#src-input"></amp-date-picker></div>',
'amp-date-picker .random-class{border-bottom: 10px}',
'',
],
'amp-date-picker-non-children-tree-shaken' => [
'<div><amp-date-picker id="example" type="single" mode="static" layout="fixed-height" height="150" format="YYYY-MM-DD"></amp-date-picker></div>',
'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( '<div><img class="logo" src="%s" width="200" height="100"></div>', admin_url( 'images/wordpress-logo.png' ) ),
'div img.logo{border:solid 1px red}',
Expand Down