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

Remove empty media queries #1423

Merged
merged 14 commits into from Nov 24, 2018

Conversation

3 participants
@korobochkin
Contributor

korobochkin commented Sep 11, 2018

After removal of the irrelevant CSS rules for the page, sometimes there are empty media queries left in the resulting page. In some cases those code bits may result in extra 4-5 KB of extra code that can be safely removed without any harm to page appearance.

For example, this code:

@media print,
screen and (min-width:40.0625em) and (min-width:40.0625em) {
  .medium-expand {
    -ms-flex: 1 1 0px;
    flex: 1 1 0px
  }
}

After tree shaking transforms into this:

@media print,screen and (min-width:40.0625em) and (min-width:40.0625em) {}

Since the selector inside this media query does not correspond to any elements on current page, we are left with empty query, that could be safely removed

To implement this we made changes in includes/sanitizers/class-amp-style-sanitizer.php.
As far as @media query is added on lines 2028-2031 we decided to add one more function which removes all empty media queries with preg_replace(). This happens after the final $stylesheet is concatenated from the small parts, but before these stylesheets size is calculated.

@korobochkin korobochkin force-pushed the korobochkin:feature/tree-shaking-improvements branch 2 times, most recently from 516a3fc to 4c8753d Sep 11, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 11, 2018

Thank you for working on this. It's something I saw as needing to be done but I haven't yet actioned on it.

Instead of removing the empty @media rules after concatenating all of the CSS, would it not be better do do the removal at:

https://github.com/Automattic/amp-wp/blob/b3e1b4d07dda70c5070d30f7426f849eb3a39823/includes/sanitizers/class-amp-style-sanitizer.php#L2027-L2031

You could check if $stylesheet_part matches something like /^@media[^{]*{\s*}$/ and skip concatenating it in the first place. This could be expanded to other @-rules as well. This would have the added advantage of not accidentally removing CSS that should be there. For example, if a stylesheet had per chance:

span:before {
   content:"@media foo {}"
}

This would currently cause the string content to be stripped out. Sure it's an unlikely thing to happen, but it seems better.

Please also add unit tests to verify that the empty rules are being removed.

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 11, 2018

I can see that my proposal won't work as is because the $stylesheet_part chunks could look line any of:

  • @supports ( object-fit: cover ){
  • @media print{
  • }
  • }@media screen and ( max-width: 48.875em ) and ( min-width: 48em ){

The last example is particularly tricky, since it contains the trailing } from a previous rule. This may indicate that a change should be made to how the stylesheet is broken up into strings in the first place to make sure it gets split into two.

@korobochkin korobochkin force-pushed the korobochkin:feature/tree-shaking-improvements branch 3 times, most recently from 63d88c2 to 83e4b59 Sep 11, 2018

@korobochkin

This comment has been minimized.

Contributor

korobochkin commented Sep 13, 2018

Thank you for the comments!
We’ve added unit tests and improved the regular expression, so it works now in the case you’ve mentioned when a stylesheet has:

span:before {
   content:"@media foo {}"
}

Your idea of improving by splitting the stylesheet seems very interesting, it needs profound investigation though, and we are researching it right now. We’ll need some time to finish it, and it’s hard to make a timing forecast at this point.

Do you think that our pull request could be approved in its current version? And as soon as we are done with the stylesheet splitting idea we’ll create another pull request for your review.

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 13, 2018

Happy to help. Thank you for contributing.

What about this example:

span:before {
   content:"this is a media query: @media foo {}"
}

The regex would incorrectly remove it here, wouldn't it? It's true that this is probably very unusual.

Your idea of improving by splitting the stylesheet seems very interesting, it needs profound investigation though, and we are researching it right now. We’ll need some time to finish it, and it’s hard to make a timing forecast at this point.

I'm pretty familiar with the CSS parser so I'll see if I can improve the splitting logic to make it easier to deal with instead of using regular expression replacements on the entire stylesheet.

westonruter added some commits Sep 13, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 13, 2018

@korobochkin there, I've updated the PHP-CSS-Parser to make sure that the stylesheet strings get split before and after CSS at rules. After doing composer install you should be able to see that when a $stylesheet_part contains an at-rule, it will always be at the beginning of the string. Likewise, the closing brace should now be isolated in a subsequent $stylesheet_part.

@korobochkin

This comment has been minimized.

Contributor

korobochkin commented Sep 20, 2018

So after running PHP Unit tests in Travis, we get 5 errors.
If we add the same CSS as in unit tests cases to the page the result of AMP_Style_Sanitizer is the same as expected and our “empty media query remover” works.

However, if we run tests locally we get 11 errors instead of 5.

We tried running PHP unit tests only and added var_dump() to see how parsed CSS in $pending_stylesheet[‘stylesheet’] looks like, but Travis exits if there are var_dump() calls. We cloned xwp-wp-dev-lib and tried to comment the PHPCS part out of Travis build (commented everything except the run_phpunit_travisci in travis.script.sh) but it didn’t help.

Can you point us in the right direction with those 5 errors?

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 22, 2018

@korobochkin sorry for the delay. I flushed the Travis build caches and the tests pass now. The old version of PHP-CSS-Parser was probably in the cache.

But the tests are passing on Travis, but you have 5 failing locally?

Removed method and PHP unit test which used in the first implementati…
…on of removing empty media query mechanism.

@westonruter westonruter added this to the v1.1 milestone Sep 27, 2018

@korobochkin

This comment has been minimized.

Contributor

korobochkin commented Sep 27, 2018

@westonruter Thank for the help with Travis tests. However, we have some difficulties with handling the CSS that comes from parsing. The problem is with the inconsistent handling of the rules that contain the '@' symbol.

The idea behind removing the empty media queries is to first look if there is any CSS inside and only merge them if this is true. Rules that contain '@' can be nested and it is important to take nested levels into account, but we notice that the parser works differently for different cases.

Ideally, we'd like to see the beginning of each rule that contains '@' marked as a separate element, and the end of the same rule marked with '}' also as a separate element.

@media screen and (min-width: 900px) {
    .foo { color: red; }

    @supports (color: red) {
        .foo { color: blue }
    }

    @font-face {
        font-family: dashicons;
        src: url(../fonts/dashicons.eot);
    }
}

@font-face {
    font-family: dashicons;
    src: url(../fonts/dashicons.eot);
}

@media screen and (min-width: 900px) {
    @counter-style foobar {
        system: cyclic;
        symbols: "";
        suffix: " ";
    }
}

@supports not (color: red) {
    .foo { color: red; }

    @media screen and (min-width: 900px) {
        .foo { color: red; }
    }
}

@supports (color: red) {
    @font-face {
        font-family: FooBar;
        src: local("FooBar");
    }
}

@supports (color: red) {
    @media screen and (min-width: 900px) {
        @font-face {
            font-family: FooBar;
            src: local("FooBar");
        }
    }
}

@supports (color: red) {
    @media screen and (min-width: 900px) {
        .foo { color: red; }
    }

    .foo { color: red; }

    @keyframes foobar {
        from { color: red; }
        to { color: blue; }
    }
}

@supports (color: red) {
    @font-feature-values FooBar {
        @bar {
            swishy: 1;
            flowing: 2;
        }
    }
}

And this is how this CSS was parsed.

array (
  0 => '',
  
  1 => '@media screen and (min-width: 900px){',
    2 => array (...),
    3 => '',
    4 => '@supports (color: red){',
      5 => array (...),
    6 => '}', //  We'd like to see '}' as a separate element
  7 => '@font-face{font-family:dashicons;src:url("../fonts/dashicons.eot")}}', // closing '}' should not be with @font-face
  
  8 => '@font-face{font-family:dashicons;src:url("../fonts/dashicons.eot")}',
  
  9 => '@media screen and (min-width: 900px){}', // empty @media in the single element with closing '}'
  10 => '',
  
  11 => '@supports not (color: red){',
    12 => array (...),
    13 => '',
    14 => '@media screen and (min-width: 900px){',
      15 => array (...),
    16 => '}',
  17 => '}',
  
  18 => '',
  19 => '@supports (color: red){@font-face{font-family:FooBar;src:local("FooBar")}}',
  20 => '',
  
  21 => '@supports (color: red){',
    22 => '@media screen and (min-width: 900px){@font-face{font-family:FooBar;src:local("FooBar")}}',
  23 => '}',
  
  24 => '',
  25 => '@supports (color: red){',
    26 => '@media screen and (min-width: 900px){',
      27 => array (),
    28 => '}',
    29 => '',
    30 => array (),
    31 => '@keyframes foobar{',
      32 => 'from{color:red}',
      33 => '',
      34 => 'to{color:blue}',
  35 => '}}', // @supports and @media are closed in the same element
  36 => '',
  37 => '@supports (color: red){}',
  38 => '',
);
@korobochkin

This comment has been minimized.

Contributor

korobochkin commented Oct 9, 2018

@westonruter We are still thinking how to solve the problem with the tests and we would like to hear your opinion.

@korobochkin

This comment has been minimized.

Contributor

korobochkin commented Oct 24, 2018

Hi, @westonruter ☺️ Just a small reminder about this PR. I am still highly interested in finishing this one. If nobody does not have a time for fixing configuration for CSS parser in near future I suppose I should ask for extra time and update parser configuration by myself. It would be great if someone could shed light on parser configuration. 🙏🏻

@westonruter

This comment has been minimized.

Member

westonruter commented Oct 24, 2018

Sorry for the delay on my feedback. I'm going to be pressed for time the next few weeks. I'm planning to return to this mid-November.

@korobochkin

This comment has been minimized.

Contributor

korobochkin commented Nov 23, 2018

@westonruter Hi! I would like to know about the progress of release 1.0 and do you have any news about our pull request? And happy Thanksgiving!

@westonruter

This comment has been minimized.

Member

westonruter commented Nov 23, 2018

This is near the top of my list to revisit now after the holidays.

The 1.0 release is currently scheduled for December 6th (which has shifted in part due to the WordPress 5.0 release date changes).

@westonruter westonruter reopened this Nov 23, 2018

@westonruter westonruter changed the base branch from develop to 1.0 Nov 24, 2018

westonruter added some commits Nov 24, 2018

@westonruter westonruter requested review from kienstra and amedina Nov 24, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Nov 24, 2018

Surprisingly, this ends up saving ~4KB in Twenty Nineteen's styles (8% of 50KB budget). These are the empty rules that get stripped: https://gist.github.com/westonruter/629f4b52e38db0c4b0ffc89e1b4d1b75

@westonruter

This comment has been minimized.

Member

westonruter commented Nov 24, 2018

@korobochkin Please give this a try. Make sure that your vendor/sabberworm/php-css-parser has successfully checked out the add/at-rule-before-after branch (xwp/PHP-CSS-Parser#1).

@westonruter westonruter modified the milestones: v1.1, v1.0 Nov 24, 2018

@westonruter westonruter added this to To do in v1.0 Nov 24, 2018

@westonruter westonruter moved this from To do to Definition in v1.0 Nov 24, 2018

@westonruter westonruter moved this from Definition to In progress in v1.0 Nov 24, 2018

@westonruter westonruter moved this from In progress to Ready for review in v1.0 Nov 24, 2018

@amedina

Ship it!

@westonruter westonruter merged commit 8f4deb2 into ampproject:1.0 Nov 24, 2018

1 check passed

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

@kienstra kienstra moved this from Ready for review to Ready for Merging in v1.0 Nov 27, 2018

@korobochkin

This comment has been minimized.

Contributor

korobochkin commented Nov 29, 2018

Thank you for your attention and work on this PR! We tested this updated version of code on few sites and it works as expected. 👍

@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