1183280 - Remove preprocessor usage for devtools CSS

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

We should remove CSS preprocessor usage within the CSS directory, which would allow an easier path forward towards reloading the frontend without a rebuild.

One option for dealing with OS-specific styles would be to apply an attribute on the root for different OSes from within theme-switching.js or similar, then those styles could be targeted using a :root[os="win"], :root[os="linux"], :root[os="osx"] or similar.  If we have any styles that absolutely need to be preprocessed anyway for some reason, we should try to contain them within a single file separate from the main styling.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

This patch removes a bunch of preprocessed css.

I had to keep some preprocessed as they are being pulled from browser/
and do not have theme-switching.js evaluated in their scope :/
Same thing for webide.css, we don't evaluate theme-switching in it.
And also need to remove the star in jar.mn.

I would like to move forward with followups here as we can already
cleanup a lot of %if everywhere!

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8683699 [details] [diff] [review]
patch v1

Review of attachment 8683699 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/ruleview.css
@@ +49,1 @@
>    margin-top: 4px;

Looks like you need to remove this for the base case.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8683699 [details] [diff] [review]
patch v1

I'm going to rebase on top of bug 1183280 once it lands.
ntim wrote similar patch to this, but with more cleanups.
I just have splitview.css cleanup on top of his patch.

Comment 4

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaaa0116d974

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Rebased.
I fixed the floating scrollbars issue.
There is some namespace/:root issue with xul documents.
It seems to work fine with a "xul" named namespace.

This patch conflicts with bug 1196786,
but I would like to see the preprocessing being removed sooner than later.
This patch is much simplier and just rewrite the existing preprocessed rules
by using :root[platform=""].

Hopefully ntim can continue and cleanup our CSS from unecessary rules...

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Fix missing namespace in floating-scrollbar-light.css

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8702589 [details] [diff] [review]
patch v3

Review of attachment 8702589 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this !

::: devtools/client/themes/common.css
@@ +18,1 @@
>    --monospace-font-family: monospace;

bgrins asked me to undo this change in https://bugzilla.mozilla.org/show_bug.cgi?id=1196786#c9 , since this file is also loaded in browser.xul (although I think it's only for the toolbox splitter and iframe)

::: devtools/client/themes/floating-scrollbars.css
@@ +13,1 @@
>    border: 0px solid transparent;

Can you change this to border: none ?

@@ +26,5 @@
>    min-height: 10px;
>    max-height: 10px;
>  }
>  
> +:root[platform="mac"] xul|slider {

Are you sure it works on all docs without *|*:root[platform="mac"] ? (I haven't tested)
Also see bug 1196786 comment 9 and comment 10 on how to test this floating scrollbar stuff.

::: devtools/client/themes/widgets.css
@@ +355,5 @@
>  
>  /* SideMenuWidget container */
>  
>  .theme-dark .side-menu-widget-container:-moz-locale-dir(ltr),
>  .theme-dark .side-menu-widget-empty-text:-moz-locale-dir(ltr) {

You should remove .theme-dark, since this rule should now apply to all themes (now that you've converted this to CSS vars)

@@ +361,4 @@
>  }
>  
>  .theme-dark .side-menu-widget-container:-moz-locale-dir(rtl),
>  .theme-dark .side-menu-widget-empty-text:-moz-locale-dir(rtl) {

Same here

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

I moved floating scrollbar and common.css modifications to bug 1235780 and bug 1235781.
And I tried to addressed all comments.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8702883 [details] [diff] [review]
patch v4

Review of attachment 8702883 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work on splitview.css and widgets.css!  The code changes look good to me, although I haven't tested this locally across all platforms

Comment 10

[Brian Grinstead [:bgrins]]

Try push with rebased patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b23db8398c39

Comment 11

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b172e1c9e59e

Comment 12

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/eae9f44f7444407a28ab3b876032eceebe19310c
Bug 1183280 - Remove most devtools preprocessing rule in CSS files. r=bgrins

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eae9f44f7444

Comment 14

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing expected

Actual Results: 
As expected