Add support for CSS variables using the `PostCSS CSS Variables` package (issue 11462) #11567

Snuffleupagus · 2020-02-04T22:00:00Z

Having thought briefly about using css-vars-ponyfill, I'm no longer convinced that it'd be a good idea. The reason is that if we actually want to properly support CSS variables, then that functionality should be available in all of our CSS files.
Note in particular the pdf_viewer.css file that's built as part of the COMPONENTS target, in which case I really cannot see how a rewrite-at-the-client solution would ever be guaranteed to always work correctly and without accidentally touching other CSS in the surrounding application.

All-in-all, simply re-writing the CSS variables at build-time seems much easier and is thus the approach taken in this patch; courtesy of /~https://github.com/MadLittleMods/postcss-css-variables
By using its preserve option, the built files will thus include both a fallback and a modern var(...) format[1]. As a proof-of-concept this patch removes a couple of manually added fallback values, and converts an additional sidebar related property to use a CSS variable.

Fixes #11462

[1] Comparing the master branch with this patch, when using gulp generic, produces the following diff for the built web/viewer.css file:

@@ -408,6 +408,7 @@

 :root {
   --sidebar-width: 200px;
+  --sidebar-transition-duration: 200ms;
 }

 * {
@@ -550,27 +551,28 @@
   position: absolute;
   top: 32px;
   bottom: 0;
-  width: 200px; /* Here, and elsewhere below, keep the constant value for compatibility
-                   with older browsers that lack support for CSS variables. */
+  width: 200px;
   width: var(--sidebar-width);
   visibility: hidden;
   z-index: 100;
   border-top: 1px solid rgba(51, 51, 51, 1);
   -webkit-transition-duration: 200ms;
           transition-duration: 200ms;
+  -webkit-transition-duration: var(--sidebar-transition-duration);
+          transition-duration: var(--sidebar-transition-duration);
   -webkit-transition-timing-function: ease;
           transition-timing-function: ease;
 }
 html[dir='ltr'] #sidebarContainer {
   -webkit-transition-property: left;
   transition-property: left;
-  left: -200px;
+  left: calc(-1 * 200px);
   left: calc(-1 * var(--sidebar-width));
 }
 html[dir='rtl'] #sidebarContainer {
   -webkit-transition-property: right;
   transition-property: right;
-  right: -200px;
+  right: calc(-1 * 200px);
   right: calc(-1 * var(--sidebar-width));
 }

@@ -640,6 +642,8 @@
 #viewerContainer:not(.pdfPresentationMode) {
   -webkit-transition-duration: 200ms;
           transition-duration: 200ms;
+  -webkit-transition-duration: var(--sidebar-transition-duration);
+          transition-duration: var(--sidebar-transition-duration);
   -webkit-transition-timing-function: ease;
           transition-timing-function: ease;
 }

pdfjsbot · 2020-02-04T23:16:51Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2020-02-04T23:18:38Z

From: Bot.io (Linux m4)

Success

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

Total script time: 1.77 mins

Published

Viewer: http://54.67.70.0:8877/f5070d8c5daf49d/web/viewer.html

Snuffleupagus · 2020-02-04T23:28:27Z

Sign, it seems that IE11 cannot handle calc(-1 * 200px); correctly or specifically the -1 part here...

We could possibly use another PostCSS package to solve that, but then again maybe not since I just found postcss/postcss-calc#87 :-(

… element to enable IE11 compatibility As gross as this hack is, it nonetheless seem necessary to allow using CSS variables; see also mozilla#11567 (comment)

pdfjsbot · 2020-02-05T08:14:54Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2020-02-05T08:16:42Z

From: Bot.io (Linux m4)

Success

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

Total script time: 1.77 mins

Published

Viewer: http://54.67.70.0:8877/c176f1ec14d0963/web/viewer.html

… element to enable IE11 compatibility As gross as this hack is, it nonetheless seem necessary to allow using CSS variables; see also mozilla#11567 (comment)

pdfjsbot · 2020-02-05T08:43:47Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2020-02-05T08:45:29Z

From: Bot.io (Linux m4)

Success

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

Total script time: 1.68 mins

Published

Viewer: http://54.67.70.0:8877/ab9cdcfbd5d2c0b/web/viewer.html

…ge (issue 11462) Having thought *briefly* about using `css-vars-ponyfill`, I'm no longer convinced that it'd be a good idea. The reason is that if we actually want to properly support CSS variables, then that functionality should be available in *all* of our CSS files. Note in particular the `pdf_viewer.css` file that's built as part of the `COMPONENTS` target, in which case I really cannot see how a rewrite-at-the-client solution would ever be guaranteed to always work correctly and without accidentally touching other CSS in the surrounding application. All-in-all, simply re-writing the CSS variables at build-time seems much easier and is thus the approach taken in this patch; courtesy of /~https://github.com/MadLittleMods/postcss-css-variables By using its `preserve` option, the built files will thus include *both* a fallback and a modern `var(...)` format[1]. As a proof-of-concept this patch removes a couple of manually added fallback values, and converts an additional sidebar related property to use a CSS variable. --- [1] Comparing the `master` branch with this patch, when using `gulp generic`, produces the following diff for the built `web/viewer.css` file: ```diff @@ -408,6 +408,7 @@ :root { --sidebar-width: 200px; + --sidebar-transition-duration: 200ms; } * { @@ -550,27 +551,28 @@ position: absolute; top: 32px; bottom: 0; - width: 200px; /* Here, and elsewhere below, keep the constant value for compatibility - with older browsers that lack support for CSS variables. */ + width: 200px; width: var(--sidebar-width); visibility: hidden; z-index: 100; border-top: 1px solid rgba(51, 51, 51, 1); -webkit-transition-duration: 200ms; transition-duration: 200ms; + -webkit-transition-duration: var(--sidebar-transition-duration); + transition-duration: var(--sidebar-transition-duration); -webkit-transition-timing-function: ease; transition-timing-function: ease; } html[dir='ltr'] #sidebarContainer { -webkit-transition-property: left; transition-property: left; - left: -200px; + left: calc(-1 * 200px); left: calc(-1 * var(--sidebar-width)); } html[dir='rtl'] #sidebarContainer { -webkit-transition-property: right; transition-property: right; - right: -200px; + right: calc(-1 * 200px); right: calc(-1 * var(--sidebar-width)); } @@ -640,6 +642,8 @@ #viewerContainer:not(.pdfPresentationMode) { -webkit-transition-duration: 200ms; transition-duration: 200ms; + -webkit-transition-duration: var(--sidebar-transition-duration); + transition-duration: var(--sidebar-transition-duration); -webkit-transition-timing-function: ease; transition-timing-function: ease; } ```

… element to enable IE11 compatibility As gross as this hack is, it nonetheless seem necessary to allow using CSS variables; see also mozilla#11567 (comment)

pdfjsbot · 2020-02-05T19:19:46Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/7e46a92fbb7f6fb/output.txt

pdfjsbot · 2020-02-05T19:21:30Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/7e46a92fbb7f6fb/output.txt

Total script time: 1.70 mins

Published

Viewer: http://54.67.70.0:8877/7e46a92fbb7f6fb/web/viewer.html

Snuffleupagus · 2020-02-05T19:32:00Z

This seems to work now, even in IE11, without anything being obviously broken :-)

Once this's landed we should thus be free to use CSS variables to simplify/re-factor the existing rules (and obviously when adding new rules), however I don't intend to convert more rules in this PR since the changes are mostly for proof-of-concept reasons and to allow validation of the overall approach.

timvandermeij · 2020-02-06T22:21:15Z

Looks good! I'm glad that we can start using CSS variables properly now.

timvandermeij added other viewer labels Feb 4, 2020

Snuffleupagus force-pushed the css-variables branch from 2b1f65f to 1205e04 Compare February 4, 2020 23:14

Snuffleupagus force-pushed the css-variables branch from 704bcc7 to 84258f7 Compare February 5, 2020 08:10

Snuffleupagus force-pushed the css-variables branch from 84258f7 to b81054b Compare February 5, 2020 08:40

Snuffleupagus marked this pull request as ready for review February 5, 2020 08:48

Snuffleupagus added 2 commits February 5, 2020 20:13

Update the left/right CSS calculation for the sidebarContainer HTML…

1021425

… element to enable IE11 compatibility As gross as this hack is, it nonetheless seem necessary to allow using CSS variables; see also mozilla#11567 (comment)

Snuffleupagus force-pushed the css-variables branch from b81054b to 1021425 Compare February 5, 2020 19:15

timvandermeij approved these changes Feb 6, 2020

View reviewed changes

timvandermeij merged commit a5fec29 into mozilla:master Feb 6, 2020

Snuffleupagus deleted the css-variables branch February 6, 2020 22:36

Snuffleupagus mentioned this pull request Mar 13, 2022

Replace dir-dependent float with logical properties #14671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CSS variables using the `PostCSS CSS Variables` package (issue 11462) #11567

Add support for CSS variables using the `PostCSS CSS Variables` package (issue 11462) #11567

Snuffleupagus commented Feb 4, 2020 •

edited

Loading

pdfjsbot commented Feb 4, 2020

pdfjsbot commented Feb 4, 2020

Snuffleupagus commented Feb 4, 2020

pdfjsbot commented Feb 5, 2020

pdfjsbot commented Feb 5, 2020

pdfjsbot commented Feb 5, 2020

pdfjsbot commented Feb 5, 2020

pdfjsbot commented Feb 5, 2020

pdfjsbot commented Feb 5, 2020

Snuffleupagus commented Feb 5, 2020

timvandermeij commented Feb 6, 2020

Add support for CSS variables using the PostCSS CSS Variables package (issue 11462) #11567

Add support for CSS variables using the PostCSS CSS Variables package (issue 11462) #11567

Conversation

Snuffleupagus commented Feb 4, 2020 • edited Loading

pdfjsbot commented Feb 4, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 4, 2020

From: Bot.io (Linux m4)

Success

Published

Snuffleupagus commented Feb 4, 2020

pdfjsbot commented Feb 5, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 5, 2020

From: Bot.io (Linux m4)

Success

Published

pdfjsbot commented Feb 5, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 5, 2020

From: Bot.io (Linux m4)

Success

Published

pdfjsbot commented Feb 5, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 5, 2020

From: Bot.io (Linux m4)

Success

Published

Snuffleupagus commented Feb 5, 2020

timvandermeij commented Feb 6, 2020

Add support for CSS variables using the `PostCSS CSS Variables` package (issue 11462) #11567

Add support for CSS variables using the `PostCSS CSS Variables` package (issue 11462) #11567

Snuffleupagus commented Feb 4, 2020 •

edited

Loading