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

Reduce length of specificity-hacked CSS selectors #6312

Merged
merged 6 commits into from
Mar 3, 2022

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 23, 2021

Summary

I realized that we can compress the size of our CSS specificity hacks quite a bit by eliminating the redundant :not() pseudo selectors. So instead of :not(#_):not(#_):not(#_) we can refactor this to be the equivalent specificity :not(#_#_#_).

Additionally, instead of adding a :root ancestor selector, we can instead just appending the the :not() to the original selector. Lastly, the prefix of .amp-wp- can be reduced down to just .amp. This is kept as-is for now to prevent any unintentional regressions.

Given this example HTML from #1313:

<style>
p#para {
    color: red !important;
    font-weight: normal !important;
    text-decoration: underline;
}
</style>
<p id="para" style="color: green !important; font-weight: bold; text-decoration: overline;">I should still be green, not bold, and overlined!</p>

Before this would have resulted in the following which is 422 bytes after minification:

p#para {
	text-decoration: underline
}

:root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) p#para {
	color: red;
	font-weight: normal
}

:root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-27ad0ad {
	font-weight: bold;
	text-decoration: overline
}

:root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-27ad0ad {
	color: green
}

With the changes in this PR, the same CSS is reduced to 235 bytes after minification:

p#para {
	text-decoration: underline
}

p#para:not(#_#_#_#_#_#_#_#_#_) {
	color: red;
	font-weight: normal
}

.amp-wp-27ad0ad:not(#_#_#_#_#_) {
	font-weight: bold;
	text-decoration: overline
}

.amp-wp-27ad0ad:not(#_#_#_#_#_#_#_#_#_#_#_#_#_#_#_#_#_) {
	color: green
}

This is a ~44% reduction in the resulting CSS.

This also fixes issues with amp-next-page for CSS selectors that are targeting elements in shadow DOM. Fixes #6909.

This also adds a tooltip to the :not(#_…) pesudo-class selectors in the stylesheets metabox:

image

Checklist

  • Esure specificities are maintained.
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter
Copy link
Member Author

The lack of :root has one unintended effect, which may or may not be a problem. Using the Specificity Calculator, I can see the new selector without :root has one fewer pseudo-class for its specificity:

image

I don't think this is a problem because the specificity from the IDs has much greater impact. But it's something to be aware of when testing.

@westonruter westonruter modified the milestones: v2.3, v2.2.2 Feb 11, 2022
@westonruter westonruter marked this pull request as ready for review February 11, 2022 19:57
@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

Plugin builds for 70fcaeb are ready 🛎️!

@westonruter
Copy link
Member Author

@swissspidy Asking for your review as I know Web Stories uses a lot of inline styles which could be impacted by this, although perhaps not due to GoogleForCreators/web-stories-wp#6783.

@swissspidy
Copy link
Collaborator

@westonruter sorry for the late reply. Correct, I could not see a difference for Web Stories because of GoogleForCreators/web-stories-wp#6783

Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

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

This looks good to me. The savings in stylesheet sizes are amazing!

I was a bit concerned if it is okay to have selectors targeting elements having multiple ID attributes. However, according to the W3C docs, it is valid:

If an element has multiple ID attributes, all of them must be treated as IDs for that element for the purposes of the ID selector. Such a situation could be reached using mixtures of xml:id, DOM3 Core, XML DTDs, and namespace-specific knowledge.

One potential issue I noticed is that the new selector is not supported in IE11:

Screenshot 2022-03-02 at 19 30 19
Chrome to the left, IE11 to the right (via BrowserStack)

The Custom HTML block content is:

<p style="color:red!important;">This is red !important text</p>
<p class="test-1">This is red text from inline stylesheet using :not(#_#_#_) method</p>
<p class="test-2">This is red text from inline stylesheet using :not(#_):not(#_):not(#_) method</p>
<style>
.test-1:not(#_#_#_) { color: red; }
.test-2:not(#_):not(#_):not(#_) { color: red; }
</style>

Since, as of WordPress 5.8, IE11 is no longer supported and also since AMP is meant primarily for mobile devices, I wouldn't regard this as a show-stopper.

@westonruter
Copy link
Member Author

One potential issue I noticed is that the new selector is not supported in IE11:

Good find! And good to know. I agree that it isn't a show-stopper.

@westonruter westonruter merged commit 3b0b45a into develop Mar 3, 2022
@westonruter westonruter deleted the try/reduce-specificity-hack-size branch March 3, 2022 01:27
@dhaval-parekh
Copy link
Collaborator

✅ QA Passed.

After adding HTML markup given description in post content. it reduced the length of CSS and does not use the :root. With new changes style of the web page are consistent with the non AMP page. ( Tested with Twenty twenty one theme )

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS selectors with !important not working in next pages with amp-next-page component
4 participants