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.

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
Merged
Changes from 5 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+36 −1
Diff settings

Always

Just for now

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

This comment has been minimized.

Copy link
@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.

Copy link
@miina

miina May 30, 2018

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

Copy link
@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].

/** Worst case scenario when we can't figure out dimensions for an image. **/
/** Force the image into a box of fixed dimensions and use object-fit to scale. **/
object-fit: contain;
@@ -942,6 +942,12 @@ private function real_path_urls( $urls, $stylesheet_url ) {
private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_list, $options ) {
$validation_errors = array();
if ( method_exists( $ruleset, 'getSelectors' ) ) {
foreach ( $ruleset->getSelectors() as $selector ) {
$this->ampify_selector( $selector );
}
}
// Remove disallowed properties.
if ( ! empty( $options['property_whitelist'] ) ) {
$properties = $ruleset->getRules();
@@ -1435,6 +1441,35 @@ private function finalize_styles() {
}
}
/**
* Ampify CSS selectors.
*
* @param Selector $selector CSS Selector.
*/
private function ampify_selector( $selector ) {

This comment has been minimized.

Copy link
@miina

miina May 28, 2018

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

Copy link
@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

// @todo What about amp-anim, amp-playbuzz?

This comment has been minimized.

Copy link
@westonruter

westonruter May 31, 2018

Member

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

$mappings = array(
'audio' => 'amp-audio',
'iframe' => 'amp-iframe',
'img' => 'amp-img',
'video' => 'amp-video',
);
$new_selector = $selector->getSelector();
foreach ( $mappings as $tag => $amp_tag ) {
if ( 0 === strpos( $selector->getSelector(), $tag ) ) {
$new_selector = substr_replace( $new_selector, $amp_tag, 0, strlen( $tag ) );
}
if ( false !== strpos( $selector->getSelector(), ' ' . $tag ) ) {
$new_selector = str_replace( ' ' . $tag, ' ' . $amp_tag, $new_selector );
}
}
if ( $selector->getSelector() !== $new_selector ) {
$selector->setSelector( $new_selector );
}
}
/**
* Finalize a stylesheet set (amp-custom or amp-keyframes).
*
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.