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

bug 1487403: upgrade cleancss to version 4.x #5135

Merged
merged 2 commits into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@davidflanagan
Copy link
Collaborator

davidflanagan commented Nov 26, 2018

This patch updates package.json to upgrade from clean-css 3.x
to clean-css-cli 4.x. It makes other modifications to add and
correctly process the command-line arguments that are required
for the new version of cleancss.

Test plan:

  • spot check the files in static/build/styles to make sure they
    have not changed too dramatically. In particular, pay attention
    to relative paths for images and fonts.
  • run locally to verify that there is no obvious CSS breakage.
bug 1487403: upgrade cleancss to version 4.x
This patch updates package.json to upgrade from clean-css 3.x
to clean-css-cli 4.x. It makes other modifications to add and
correctly process the command-line arguments that are required
for the new version of cleancss.

Test plan:
- spot check the files in static/build/styles to make sure they
  have not changed too dramatically. In particular, pay attention
  to relative paths for images and fonts.
- run locally to verify that there is no obvious CSS breakage.
.travis.yml Outdated
@@ -22,6 +22,7 @@ env:
- ES_VERSION=5.6.10
- ES_DOWNLOAD_URL=https://artifacts.elastic.co/downloads/elasticsearch/elasticsearch-5.6.10.tar.gz
- PIPELINE_CLEANCSS_BINARY=$TRAVIS_BUILD_DIR/node_modules/.bin/cleancss
- PIPELINE_CLEANCSS_ARGUMENTS="-O2 --skip-rebase"

This comment has been minimized.

@davidflanagan

davidflanagan Nov 26, 2018

Collaborator

I don't know what uses travis.yml, so I don't know how to test that this works. In particular I don't know whether the double quotes here are the right thing.

This comment has been minimized.

@davidflanagan

davidflanagan Nov 26, 2018

Collaborator

Well, the travis ran automatically when I created the PR, so presumably that means that this worked.

command = (binary, args) if args else (binary,)
# If the arguments ever include quoted arguments with spaces then
# the simple split() call here is not going to be good enough.
command = (binary,) + tuple(args.split())

This comment has been minimized.

@davidflanagan

davidflanagan Nov 26, 2018

Collaborator

The existing code worked for a single argument but was not written to work with more than one.

@jwhitlock
Copy link
Member

jwhitlock left a comment

Thanks @davidflanagan, this works in my development environment. Good job finding the --skip-rebase command, that seemed to make the paths consistent between the new and old versions.

I tested with the production-style assets. The generated files are about the same size, and ordered differently, so the minified versions are impossible to compare visually. I pushed dashboard.css through a pretty-printer, and there appear to be some differences:

The old version has dual size declarations, maybe for IE9 compatibility.

.banned,.dashboard-slug,td.dashboard-comment{
    font-style:italic
}
.banned{
    color:#e66465;
    font-size:16px;
    font-size:.88889rem
}

The new version drops font-size:16px; and pulls the italic in:

.banned{
    color:#e66465;
    font-size:.88889rem;
    font-style:italic
}

The old version has a double-entry for color, one with a traditional color and one with an rgba color:

.dashboard-table tbody tr{
    cursor:pointer;
    border-top-width:1px;
    border-top-style:solid;
    border-top-color:#adadad;
    border-top-color:rgba(51,51,51,.4)
}

The new version drops rgba it and combines into one declaration:

.dashboard-table tbody tr{
    cursor:pointer;
    border-top:1px solid #adadad
}

These changes don't appear to make a lot of difference when viewing in Firefox.

I get a different package-lock.json when I run make npmrefresh. I'm also not sure if it matters. Do you recall how you updated that file?

I'd be OK merging this, or we can put it on the staging server first, if anyone wants to poke around and look for styling issues.

@davidflanagan

This comment has been minimized.

Copy link
Collaborator

davidflanagan commented Nov 27, 2018

@jwhitlock I think I updated the package-lock file with npmrefresh. I don't really understand those files, but I never see a small diff in a package lock file, so I gather that they have non-deterministic ordering of the entries, unfortunately.

I had noticed that the new version was removing redundant font sizes and combining styles where possible. But I had not noticed the color changes you point out. Devtools indicate that our dashboard tools are using the rgba version of that border color, so I'm surprised to see that that is the one that is being removed rather than the hex code. Maybe CSS specifies that borders are never actually translucent, and in that case maybe the two colors are actually equivalent. If not, then I'm worried that we will have visual changes...

@davidflanagan

This comment has been minimized.

Copy link
Collaborator

davidflanagan commented Nov 27, 2018

So that is totally a bug in cleancss. I'm updating this PR to add another command-line argument to work around it.

I found a bug in one of the cleancss -O2 optimizations and
the package is not very actively maintained right now, so
playing it safe even though this makes our CSS files a little
bigger than they were.
@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 28, 2018

This one looks better. cleancss 3.x looks like it was a little more efficient. Here's a rule from wiki.css:

.reviews{
    background:#fff3d4;
    border-color:#f6b73c;
    color:#333
}
/* Some space */
.quick-links,.reviews{
    font-size:16px;
    overflow:hidden
}
/* Some space */
.reviews{
    box-sizing:border-box;
    max-width:42rem;
    margin-bottom:20px;
    padding:10px;
    font-size:.88889rem;
    border-left-width:5px;
    border-left-style:solid
}

The same rule from cleancss 4.x at -01:

.reviews{
    background:#fff3d4;
    border-color:#f6b73c;
    color:#333
}
/* Some space */
.reviews{
    box-sizing:border-box;
    max-width:42rem;
    overflow:hidden;
    margin-bottom:20px;
    padding:10px;
    font-size:16px;
    font-size:.88889rem
}
.reviews{
    border-left-width:5px;
    border-left-style:solid
}

The rules are the same but in a different order. Maybe that -O2 can do better when it is working.

I looked at some pages locally, and they appear the same:

This looks ready to me. And our CSS could use some cleanup.

@jwhitlock jwhitlock requested a review from schalkneethling Nov 28, 2018

@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 28, 2018

@schalkneethling said he'd like to take a look as well. I've duplicated this branch to https://github.com/mozilla/kuma/tree/cleancss-1487403 (to build a docker image) and force-pushed to stage-push, so now it is deployed to https://developer.allizom.org. Hopefully this makes the review a bit easier.

@schalkneethling, feel free to merge if you feel it is ready.

@schalkneethling

This comment has been minimized.

Copy link
Collaborator

schalkneethling commented Nov 29, 2018

This looks ready to me. And our CSS could use some cleanup.

Oh boy can it ever ;)

Poked around on staging, all looks good. Merging, Thx @davidflanagan 🌮

@schalkneethling schalkneethling merged commit a30e0fd into mozilla:master Nov 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No new, high severity issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment