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

1094: Transform CSS selectors according to sanitizer HTML element to AMP component conversions #1175

Merged
merged 17 commits into from May 31, 2018

Conversation

Projects
None yet
3 participants
@miina
Copy link
Collaborator

miina commented May 25, 2018

Modify AMP_Style_Sanitizer so that it would convert non-amp CSS selectors to amp component selectors in case it seems to be necessary. Fixes issue with potential broken design, for example with Twenty Ten header.

Fixes #1094.

miina added some commits May 25, 2018

$selectors[] = $amp_selectors_parsed['selector'];
} else {
$selectors = array_keys( $selectors_parsed );
}

This comment has been minimized.

@miina

miina May 25, 2018

Collaborator

@westonruter Started adding the logic within this method since it seemed to be related with tree shaking in terms of adding/not adding the selector if it's not found after replacing. That's still WIP (e.g. currently works only in case ! $should_tree_shake and is missing most of the mappings) but let me know if there are any concerns with this approach.

This comment has been minimized.

@miina

miina May 25, 2018

Collaborator

Hmm, maybe one potential issue could be that the size of the style could exceed the limit if doing the replacement here.

This comment has been minimized.

@miina

miina May 28, 2018

Collaborator

Moving the logic to process_css_declaration_block to avoid issues with style size changing after the size check.

*
* @param Selector $selector CSS Selector.
*/
private function ampify_selector( $selector ) {

This comment has been minimized.

@miina

miina May 28, 2018

Collaborator

@westonruter Reworked the logic to replace the components before tree shaking instead (compared to the previous commit). Wondering if it's OK to just replace the selectors or is it possible that this way we'll be replacing a selector that's actually in use.

Also, in case of img it can be replaced with either amp-img or amp-anim, thinking if we should add an additional selector for potential amp-anim, too.

This comment has been minimized.

@westonruter

westonruter May 30, 2018

Member

It would be ideal if the selector mappings could be added to each sanitizer. For example, on AMP_Img_Sanitizer there could be a get_selector_mapping() method which returns:

array(
    'img' => array( 'amp-img', 'amp-anim' )
)

Whereas on the AMP_Audio_Sanitizer class it would return:

array(
    'video' => array( 'amp-video ),
)

In other words, there could be multiple AMP components that could be converted so the selector should do replacements for each one, potentially duplicating a selector however many times is needed.

As for how to get the AMP_Style_Sanitizer to be able to invoke such a get_selector_mapping method on the other sanitizers, it could be done similar to as proposed for passing embed handlers to an embed sanitizer: https://github.com/Automattic/amp-wp/pull/1128/files#diff-2585472f207548f626a43a7ff3cab922R150

I have a concern with that approach of passing class instances as sanitizer args, however. It could cause problems with the response caching introduced in #1156, since the cache key is computed by passing the sanitizer args:

https://github.com/Automattic/amp-wp/blob/ab7204af35a8eb68bd8840a2e3531df8ae73c21f/includes/class-amp-theme-support.php#L1073-L1078

/cc @juanchaur1

@@ -1,4 +1,4 @@
.amp-wp-unknown-size img {
.amp-wp-unknown-size .i-amphtml-replaced-content {

This comment has been minimized.

@westonruter

westonruter May 30, 2018

Member

It is illegal in AMP to use i-amphtml-replaced-content class names. See:

Internal AMP class names prefixed with -amp- and i-amp- are disallowed in AMP HTML.

https://www.ampproject.org/docs/fundamentals/spec#classes

This comment has been minimized.

@miina

miina May 30, 2018

Collaborator

Yes, think I even saw a todo note recently somewhere as well to remove these from CSS within style sanitizer and realized that it's illegal. Will look into which selector to use.

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

Since tree shaking isn't done for attribute values, I suggest the way to fix this is to use an attribute selector like [src].

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented May 30, 2018

@miina it's also important to note that <amp-img> wraps a normal <img> tag, however this tag is inserted after the page renders which means that the element didn't exist when the tree shaking takes place. So the style .amp-wp-unknown-size img would have been removed since it was not pointing to an existing element.

A suggestion would be to maybe have a rule that any class starting with amp-wp-* be exempted.

@miina

This comment has been minimized.

Copy link
Collaborator

miina commented May 30, 2018

@DavidCramer Do you mean "exempted" as excluded from sanitizing and never removed from CSS?

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented May 30, 2018

@miina yes, but only if there is an element with the class.
example:

.amp-wp-unknown-size img {
....
}

if it finds an element with class="amp-wp-unknown-size" it will be left in as it can be assumed that the img will exist but doesn't currently.

@DavidCramer DavidCramer referenced this pull request May 30, 2018

Merged

Add sanitizer to fixup issues with core themes #1074

14 of 16 tasks complete

westonruter and others added some commits May 31, 2018

* @param Selector $selector CSS Selector.
*/
private function ampify_selector( $selector ) {
// @todo What about amp-anim, amp-playbuzz?

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

I've got an idea I'm working on for this…

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented May 31, 2018

@miina there, I feel pretty happy with 7a218aa as something to iterate on. I left some todos for some additional changes, other than the ones you already have outstanding.

// Let the sanitizers.
foreach ( $sanitizers as $sanitizer ) {
$sanitizer->after_sanitize( $sanitizers );
}

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

Since we're not using this, I think we should just kill after_sanitize.

// Let the sanitizers know about each other prior to sanitizing.
foreach ( $sanitizers as $sanitizer ) {
$sanitizer->before_sanitize( $sanitizers );

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

I do think we should rename this to init

$replacements = 0;
foreach ( $ruleset->getSelectors() as $old_selector ) {
$edited_selectors = array( $old_selector->getSelector() );
foreach ( $this->selector_mappings as $html_selector => $amp_selectors ) { // Note: The $selector_mappings array contains ~6 items.

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

These notes are here because 4 level of loop nesting is normally something that is a big red flag for performance.

@@ -1,4 +1,4 @@
.amp-wp-unknown-size img {
.amp-wp-unknown-size .i-amphtml-replaced-content {

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

Since tree shaking isn't done for attribute values, I suggest the way to fix this is to use an attribute selector like [src].

miina added some commits May 31, 2018

Merge branch 'add/1094-convert_css_selectors' of github.com:Automatti…
…c/amp-wp into add/1094-convert_css_selectors
@@ -514,6 +524,8 @@ public function filter_attachment_layout_attributes( $node, $new_attributes, $la
/**
* Add <amp-image-lightbox> element to body tag if it doesn't exist yet.
*
* @todo Move this to AMP_Img_Sanitizer?
*/
public function maybe_add_amp_image_lightbox_node() {

This comment has been minimized.

@miina

miina May 31, 2018

Collaborator

Note that this is also used by AMP_Gallery_Block_Sanitizer.

if ( isset( $parent_attributes['data-amp-lightbox'] ) && true === filter_var( $parent_attributes['data-amp-lightbox'], FILTER_VALIDATE_BOOLEAN ) ) {
$attributes['data-amp-lightbox'] = '';
$attributes['on'] = 'tap:' . self::AMP_IMAGE_LIGHTBOX_ID;

This comment has been minimized.

@miina

miina May 31, 2018

Collaborator

Note that self:AMP_IMAGE_LIGHTBOX_ID is also used by AMP_Gallery_Block_Sanitizer, that's why it to AMP_Base_Sanitizer.

@miina miina changed the title [WIP] 1094: Transform CSS selectors according to sanitizer HTML element to AMP component conversions 1094: Transform CSS selectors according to sanitizer HTML element to AMP component conversions May 31, 2018

westonruter and others added some commits May 31, 2018

Merge branch 'add/1094-convert_css_selectors' of github.com:Automatti…
…c/amp-wp into add/1094-convert_css_selectors

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

@westonruter westonruter merged commit 70b81ae into develop May 31, 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 add/1094-convert_css_selectors branch May 31, 2018

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