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

Improve handling of !important transformation specificity #1073

Closed
westonruter opened this Issue Apr 13, 2018 · 3 comments

Comments

4 participants
@westonruter
Copy link
Member

westonruter commented Apr 13, 2018

On https://scalewp.io/development-and-workflow/ there is an element:

<header id="header" style="display: none;">

In spite of the inline style, it gets shown because the stylesheet has:

#header {
    display: block !important;
    width: 100%;
    background: #FFF;
}

Nevertheless, when it is served as AMP the header is not displayed. The inline style is currently getting converted into a style rule as follows:

:root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-224b51a {
    display: none;
}

And the !important property gets extracted into a separate rule as:

:root:not(#_) #header {
    display: block;
}

Something is wrong in the logic because currently the header is not appearing in AMP where it is appearing in non-AMP. In other words, the generated selector for the inline style :root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-224b51a is winning over an !important property in an ID-selector style rule which gets transformed as :root:not(#_) #header.

Something needs to change in the collect_inline_styles and transform_important_qualifiers methods:

https://github.com/Automattic/amp-wp/blob/6861830b80e2d2cc17306f034feef976c35f6e22/includes/sanitizers/class-amp-style-sanitizer.php#L1064-L1102

and

https://github.com/Automattic/amp-wp/blob/6861830b80e2d2cc17306f034feef976c35f6e22/includes/sanitizers/class-amp-style-sanitizer.php#L981-L1048

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

@westonruter westonruter added this to To do in v1.0 May 2, 2018

@westonruter westonruter moved this from To do to Definition in v1.0 May 2, 2018

@postphotos postphotos moved this from Definition to To do in v1.0 May 2, 2018

@postphotos postphotos added the release label Jul 9, 2018

@westonruter westonruter changed the title Improve handling of !important transformation specificity Verify handling of !important transformation specificity Jul 24, 2018

@westonruter westonruter changed the title Verify handling of !important transformation specificity Improve handling of !important transformation specificity Jul 24, 2018

@hellofromtonya hellofromtonya self-assigned this Jul 30, 2018

@hellofromtonya hellofromtonya moved this from To do to In progress in v1.0 Jul 30, 2018

@hellofromtonya hellofromtonya moved this from In progress to To do in v1.0 Aug 1, 2018

@hellofromtonya hellofromtonya moved this from To do to In progress in v1.0 Aug 2, 2018

@hellofromtonya

This comment has been minimized.

Copy link
Contributor

hellofromtonya commented Aug 3, 2018

We are applying a base specificity multiplier of 5 to the inline styles:

$root  = ':root' . str_repeat( ':not(#_)', 5 );

We do this to ensure that inline styles take precedence over non !important style rulesets.

In order to have !important rulesets take precedence, we can apply that same base specificity multiplier + 1.

		// Compute the specificity multiplier.
		$specificity_multiplier = self::INLINE_SPECIFICITY_MULTIPLIER + 1 + floor( $old_selector->getSpecificity() / 100 );
		if ( $old_selector->getSpecificity() % 100 > 0 ) {
			$specificity_multiplier++;
		}
		if ( $old_selector->getSpecificity() % 10 > 0 ) {
			$specificity_multiplier++;
		}

		$selector_mod = str_repeat( ':not(#_)', $specificity_multiplier );
		$new_selector = $old_selector->getSelector();

		// Amend the selector mod to the first element in selector if it is already the root; otherwise add new root ancestor.
		if ( preg_match( '/^\s*(html|:root)\b/i', $new_selector, $matches ) ) {
			$new_selector = substr( $new_selector, 0, strlen( $matches[0] ) ) . $selector_mod . substr( $new_selector, strlen( $matches[0] ) );
		} else {
			$new_selector = sprintf( ':root%s %s', $selector_mod, $new_selector );
		}
		return new Selector( $new_selector );
	}

The net result is this:

:root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) #header {
    display: block;
}

screen shot 2018-08-03 at 6 16 19 pm

The transformed CSS ruleset now takes precedence.

PR is coming...tomorrow.

@hellofromtonya hellofromtonya moved this from In progress to Ready for review in v1.0 Aug 5, 2018

@westonruter westonruter moved this from Ready for review to Ready for QA in v1.0 Aug 5, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 18, 2018

Moving To 'Ready For Merging'

If it's alright, I'm moving this to 'Ready For Merging,' as I don't think it needs functional testing. Feel free to move it back.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Sep 18, 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
You can’t perform that action at this time.