Implement autoprefixer #9629

wojtekmaj · 2018-04-03T22:36:50Z

Hey there!
I've implemented autoprefixer as a proof of concept. It was a breeze, actually!

Likely will help with: #6685

The only caveat worth mentioning is that if there were some browser-specific hacks like adding !important to :-ms-something only. See comments in the actual code for details.

Action:
I need to ask you for help regarding specific configuration on which browsers to target. For now, it's all on default settings and we don't configure anything.

I carefully compared the files using Mergely, and most of the differences make sense, but you will want to hardcode the configuration so that you'll be sure there are no changes without your knowledge.

This config would be in form of .browserslistrc file. Sample file could look like this:

# Browsers that we support 
 
> 1%
Last 2 versions
IE 10

which means that we would like to support anything with more than 1%, market share, all last 2 versions of everything, no exceptions, and IE10.

Here's all the fun you can do: https://www.npmjs.com/package/browserslist

I checked Allow edits from maintainers and you can feel invited to use this option :)

wojtekmaj · 2018-04-03T22:43:28Z

web/viewer.css

-  cursor: none;
-  -moz-user-select: none;
-}
-
 #viewerContainer.pdfPresentationMode:-ms-fullscreen {
  top: 0px !important;


This is the line I was mentioning in the description. The thing is, now that we have separate rules:

:-ms-fullscreen

:fullscreen

they get expanded by autoprefixer to:

:-ms-fullscreen

:-webkit-full-screen

:-moz-full-screen

:-ms-fullscreen

:fullscreen

As you can see, 1. was untouched, but 2 got expanded into all rules for all the browsers. So now, -ms-specific overwrites are before all the others, so if any other !important would occur in generic code, we can get these rules overwritten. This is not the case, thankfully, but it's something we should be aware of for the future.

Rob--W

Can you show the diff between the CSS files generated before, and after this PR?

Rob--W · 2018-04-04T12:07:09Z

gulpfile.js

@@ -766,6 +772,7 @@ gulp.task('chromium-pre', ['buildnumber', 'locale'], function () {
    preprocessHTML('web/viewer.html', defines)
        .pipe(gulp.dest(CHROME_BUILD_CONTENT_DIR + 'web')),
    preprocessCSS('web/viewer.css', 'chrome', defines, true)
+        .pipe(postcss([autoprefixer()]))


This version only needs to be compatible with Chrome, so use:

autoprefixer({ browsers: ['chrome >= 49'], })

(Chrome 49, because it is the last supported Chrome version by WIndows XP).

Rob--W · 2018-04-04T12:08:31Z

gulpfile.js

@@ -719,6 +724,7 @@ gulp.task('mozcentral-pre', ['buildnumber', 'locale'], function () {
    preprocessHTML('web/viewer.html', defines)
        .pipe(gulp.dest(MOZCENTRAL_CONTENT_DIR + 'web')),
    preprocessCSS('web/viewer.css', 'mozcentral', defines, true)
+        .pipe(postcss([autoprefixer()]))


This is only used in the latest version of Firefox, so use:

autoprefixer({ browsers: ['last 1 firefox versions'], })

wojtekmaj · 2018-04-04T13:00:05Z

Absolutely, here are the files:

Happy to hear your comments.

Rob--W · 2018-04-04T13:32:02Z

Generated viewer.css for chromium and mozcentral looks good to me.
I haven't checked the generic build yet.

The browser support table is at https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support . Can you add a browserlist so that the autoprefixer generates the expected prefixes?

wojtekmaj · 2018-04-04T13:37:47Z

We went for config passed directly to autoprefixer() function approach, so I'd follow that approach and do it again for gulp generic for consistency and to avoid any side effects.

This table is a little too general for me to build a config as it doesn't state which browser versions we definitely support. Here's my best guess. How does that sound?

last 2 versions
Chrome 49 # last supported on Windows XP
IE 11
Safari 8 # desktop Safari

Rob--W · 2018-04-04T13:50:32Z

Chrome 49 is OK because it is only listed because of Windows XP support (if XP support is desired, perhaps it's worth including "Firefox 52" too - this is currently Firefox ESR, and the last to support Win XP: https://support.mozilla.org/en-US/kb/end-support-windows-xp-and-vista ).
IE 11 is OK because it is the last IE version.
Safari >= 8 should be used to account for newer versions of Safari; similarly for ios.

Firefox ESR is missing from the list. Perhaps you can use defaults (see https://github.com/browserslist/browserslist#queries) and extend that with the explicitly supported browser versions.

wojtekmaj · 2018-04-04T14:34:04Z

Safari 8 <==> Safari >= 8 unless Safari would regress in development _{^{(I wouldn't be surprised 🙄)}}
iOS is covered by last 2 versions and in my opinion taking their fabulous update distribution speed I wouldn't personally bother much more.
Regarding Windows XP support, I added Chrome 49 only because of chromium-pre config, maybe that was a mistake. I'm certainly not the one deciding on this ;)

So to sum up, we could go for:

Option 1: Old iOS support (>99,5% market share), XP support

last 2 versions
Firefox 52 # last supported on Windows XP
Chrome 49 # last supported on Windows XP
IE 11
iOS last 3 versions
Safari 8 # desktop Safari

Option 2: iOS default (>95% market share), XP support

last 2 versions
Firefox 52 # last supported on Windows XP
Chrome 49 # last supported on Windows XP
IE 11
Safari 8 # desktop Safari

Option 3: No XP support, iOS default

last 2 versions
Firefox ESR
IE 11
Safari 8 # desktop Safari

Rob--W · 2018-04-04T16:04:58Z

Safari 8 <==> Safari >= 8 unless Safari would regress in development _{^{(I wouldn't be surprised 🙄)}}

The intention is to support Safari 8 and later, so Safari >= 8 is more accurate. I haven't checked whether there would be any difference (I would not be surprised either way), but it is possible to have a new prefix in a later version. A historical example of prefix madness is Flexbox.

How about this:

last 2 versions
Chrome 49 # Last supported on Windows XP
Firefox 52 # Last supported on Windows XP
Firefox ESR
IE 11
Safari >= 8
> 0.5%
not dead

(based on the defaults as documented at https://github.com/browserslist/browserslist#queries)
(92.16% coverage according to http://browserl.ist/?q=last+2+versions%2C+Chrome+49%2C+Firefox+52%2C+Firefox+ESR%2C+IE+11%2C+Safari+%3E%3D+8%2C+%3E+0.5%25%2C+not+dead)

How did you determine the browser support? I'm checking browserl.ist and the results are different. E.g. option 1 has 76.64% coverage according to http://browserl.ist/?q=last%202%20versions,Firefox%2052,Chrome%2049,IE%2011,last%203%20iOS%20versions,Safari%208 (note: I fixed the syntax by changing "iOS last 3 versions" to "last 3 iOS versions").

wojtekmaj · 2018-04-04T16:36:39Z

Browser support was only applying to iOS versions, I thought I made it clear, sorry if it wasn't. This config looks great to me!

timvandermeij · 2018-04-04T20:47:52Z

Nice work! The integration indeed looks easier than I would have expected. I'll review this in the coming days (mainly checking the various diffs), but quickly looking at the diffs tells me that this is a good idea; it cleans up the CSS quite a bit and it's much less error-prone since I'm seeing some (un)prefixed rules that we currently just miss.

Edit: I'll also dive into the .browserlistrc file during the review, so please add the one you think is best to this PR.

wojtekmaj · 2018-04-04T20:56:00Z

I'm adding what @Rob--W suggested (no reason not to, it doesn't cost us anything to support some more prefixes), but in a form of autoprefixer() config in gulpfile to have it consistent, if you don't mind. Another reason is that .browserlistrc can be read by several other libraries, too, and that could introduce some side effects I can't predict.

timvandermeij · 2018-04-04T20:58:02Z

Yes, that sound fine to me.

timvandermeij · 2018-04-08T11:19:40Z

/botio-linux preview

pdfjsbot · 2018-04-08T11:19:41Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/21e593bfeae8a81/output.txt

pdfjsbot · 2018-04-08T11:22:01Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/21e593bfeae8a81/output.txt

Total script time: 2.32 mins

Published

Viewer: http://54.67.70.0:8877/21e593bfeae8a81/web/viewer.html

timvandermeij · 2018-04-08T11:22:31Z

/botio test

pdfjsbot · 2018-04-08T11:22:31Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/24cf616f40ebf3a/output.txt

pdfjsbot · 2018-04-08T11:22:32Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/79f71dc9faf0d9c/output.txt

pdfjsbot · 2018-04-08T11:40:36Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/24cf616f40ebf3a/output.txt

Total script time: 18.06 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/24cf616f40ebf3a/reftest-analyzer.html#web=eq.log

pdfjsbot · 2018-04-08T11:47:04Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/79f71dc9faf0d9c/output.txt

Total script time: 24.53 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/79f71dc9faf0d9c/reftest-analyzer.html#web=eq.log

In `rasterizeAnnotationLayer` we load the source CSS files directly, so these are not processed by Autoprefixer. Since the prefixed rules have now been removed from the source CSS files, we must manually provide one prefixed rule that Chrome needs in the overrides CSS file for checkbox and radio button rendering to work in the reference tests.

timvandermeij · 2018-04-08T12:05:07Z

/botio test

pdfjsbot · 2018-04-08T12:05:07Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/e0fa6c49410b608/output.txt

pdfjsbot · 2018-04-08T12:05:08Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/afa5de509a23cf2/output.txt

pdfjsbot · 2018-04-08T12:23:07Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/e0fa6c49410b608/output.txt

Total script time: 17.99 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2018-04-08T12:29:39Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/afa5de509a23cf2/output.txt

Total script time: 24.52 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2018-04-08T13:12:20Z

Thank you for implementing this! It cleans up the CSS files quite a bit.

wojtekmaj · 2018-04-09T10:52:18Z

Super glad I could help. ❤️ to Mozilla!

prohtex · 2018-04-13T22:14:05Z

Any chance we could address #9392 as well?

Implement autoprefixer

wojtekmaj added 2 commits April 4, 2018 00:25

Clean up browser-specific CSS

dbb0e88

Introduce autoprefixer to gulp process

ed67c1d

wojtekmaj commented Apr 3, 2018

View reviewed changes

wojtekmaj mentioned this pull request Apr 3, 2018

Offically release version 2.0 #9521

Closed

3 tasks

Rob--W reviewed Apr 4, 2018

View reviewed changes

Rob--W requested a review from timvandermeij April 4, 2018 16:53

timvandermeij added viewer other labels Apr 4, 2018

Configure Autoprefixer

acd1fa4

timvandermeij approved these changes Apr 8, 2018

View reviewed changes

timvandermeij merged commit 1a634f9 into mozilla:master Apr 8, 2018

timvandermeij mentioned this pull request Apr 8, 2018

Default viewer doesn't use 'retina' toolbar icons on iOS #6685

Closed

timvandermeij removed the other label Apr 8, 2018

wojtekmaj deleted the add-autoprefixer branch April 9, 2018 20:44

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#9629 from wojtekmaj/add-autoprefixer

20bd46c

Implement autoprefixer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement autoprefixer #9629

Implement autoprefixer #9629

wojtekmaj commented Apr 3, 2018 •

edited

Loading

wojtekmaj Apr 3, 2018

Rob--W left a comment

Rob--W Apr 4, 2018

Rob--W Apr 4, 2018

wojtekmaj commented Apr 4, 2018 •

edited

Loading

Rob--W commented Apr 4, 2018

wojtekmaj commented Apr 4, 2018 •

edited

Loading

Rob--W commented Apr 4, 2018

wojtekmaj commented Apr 4, 2018 •

edited

Loading

Rob--W commented Apr 4, 2018 •

edited

Loading

wojtekmaj commented Apr 4, 2018

timvandermeij commented Apr 4, 2018 •

edited

Loading

wojtekmaj commented Apr 4, 2018

timvandermeij commented Apr 4, 2018

timvandermeij commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

timvandermeij commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

timvandermeij commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

timvandermeij commented Apr 8, 2018

wojtekmaj commented Apr 9, 2018

prohtex commented Apr 13, 2018

Implement autoprefixer #9629

Implement autoprefixer #9629

Conversation

wojtekmaj commented Apr 3, 2018 • edited Loading

wojtekmaj Apr 3, 2018

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Rob--W Apr 4, 2018

Choose a reason for hiding this comment

Rob--W Apr 4, 2018

Choose a reason for hiding this comment

wojtekmaj commented Apr 4, 2018 • edited Loading

gulp generic

gulp mozcentral-pre

gulp chromium-pre

Rob--W commented Apr 4, 2018

wojtekmaj commented Apr 4, 2018 • edited Loading

Rob--W commented Apr 4, 2018

wojtekmaj commented Apr 4, 2018 • edited Loading

Rob--W commented Apr 4, 2018 • edited Loading

wojtekmaj commented Apr 4, 2018

timvandermeij commented Apr 4, 2018 • edited Loading

wojtekmaj commented Apr 4, 2018

timvandermeij commented Apr 4, 2018

timvandermeij commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 8, 2018

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 8, 2018

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 8, 2018

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Apr 8, 2018

From: Bot.io (Windows)

Failed

timvandermeij commented Apr 8, 2018

pdfjsbot commented Apr 8, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 8, 2018

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 8, 2018

From: Bot.io (Linux m4)

Success

pdfjsbot commented Apr 8, 2018

From: Bot.io (Windows)

Success

timvandermeij commented Apr 8, 2018

wojtekmaj commented Apr 9, 2018

prohtex commented Apr 13, 2018

wojtekmaj commented Apr 3, 2018 •

edited

Loading

wojtekmaj commented Apr 4, 2018 •

edited

Loading

`gulp generic`

`gulp mozcentral-pre`

`gulp chromium-pre`

wojtekmaj commented Apr 4, 2018 •

edited

Loading

wojtekmaj commented Apr 4, 2018 •

edited

Loading

Rob--W commented Apr 4, 2018 •

edited

Loading

timvandermeij commented Apr 4, 2018 •

edited

Loading