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.

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

Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes #1111

Merged
merged 4 commits into from May 3, 2018

Conversation

5 participants
@westonruter
Copy link
Member

westonruter commented May 3, 2018

This is a follow-up on #1048, and in particular this #1048 (comment) from @camelburrito:

usually the [class names] associated with amp-bind can be searchable in the dom (string search), even that would be a little risky because there could be string concat etc happening

I suspect that dynamic class names (constructed via concatenation) would be less likely than static ones, since dynamic class names would depend on having a correspondingly dynamic stylesheet. This doesn't seem likely.

With the changes in this PR, with the following markup:

<amp-state id="mySidebar">
	<script type="application/json">
		{"expanded": false}
	</script>
</amp-state>
<aside class="sidebar" [class]="mySidebar.expanded ? 'expanded' : ''">...</aside>

And given the following CSS:

.sidebar { display:none }
.sidebar.expanded { display:block }

The second style rule will no longer be removed when tree shaking is being performed.

This should greatly reduce the amount of times that someone has to manually inform style sanitizer of the dynamic_element_selectors, for example:

add_filter( 'amp_content_sanitizers', function( $sanitizers ) {
	if ( ! isset( $sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] ) ) {
		$sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] = array();
	}
	$sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'] = array_merge(
		$sanitizers['AMP_Style_Sanitizer']['dynamic_element_selectors'],
		array(
			'amp-list',
			'amp-live-list',
			'[submit-error]',
			'[submit-success]',
			'.expanded', // <= Added.
			'.sidebar-open', // <= Added.
		)
	);
	return $sanitizers;
} );

westonruter added some commits May 3, 2018

@westonruter westonruter added this to the v1.0 milestone May 3, 2018

@westonruter westonruter requested a review from amedina May 3, 2018

@westonruter westonruter changed the title Prevent CSS tree shaking from removing selectors with classes mentioned in [class] attrs Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes May 3, 2018

@westonruter westonruter force-pushed the update/dynamic-element-selectors branch from e90f5c3 to 8cc3fb7 May 3, 2018

@westonruter westonruter requested a review from kienstra May 3, 2018

@kienstra
Copy link
Collaborator

kienstra left a comment

Approved

Hi @westonruter,
Thanks, this looks good and works as expected. I also saw the issue of the .sidebar.expanded selector being stripped in the develop branch.

But with this PR, it appears as expected.

@amedina

amedina approved these changes May 3, 2018

Copy link
Member

amedina left a comment

LGTM

@camelburrito

This comment has been minimized.

Copy link

camelburrito commented May 3, 2018

Thank you!! This looks great!

@westonruter westonruter merged commit 857e403 into develop May 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the update/dynamic-element-selectors branch May 3, 2018

@postphotos postphotos added this to Definition in v1.0 May 22, 2018

@westonruter westonruter moved this from Definition to Ready for QA in v1.0 May 22, 2018

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Jun 6, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment