Closed Bug 1183280 Opened 5 years ago Closed 5 years ago

Remove preprocessor usage for devtools CSS

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox42 affected, firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox42 --- affected
firefox46 --- fixed

People

(Reporter: bgrins, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: [devtools-css])

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 1196786
Whiteboard: [devtools-css]
Depends on: 1211190
Depends on: 1219613
Attached patch patch v1 (obsolete) — Splinter Review
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 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 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.
Attachment #8683699 - Attachment is obsolete: true
Depends on: 1229328
Attached patch patch v2 (obsolete) — Splinter Review
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...
Attachment #8702576 - Flags: review?(bgrinstead)
Attached patch patch v3 (obsolete) — Splinter Review
Fix missing namespace in floating-scrollbar-light.css
Attachment #8702589 - Flags: review?(bgrinstead)
Attachment #8702576 - Attachment is obsolete: true
Attachment #8702576 - Flags: review?(bgrinstead)
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
Depends on: 1235780
Depends on: 1235781
Attached patch patch v4Splinter Review
I moved floating scrollbar and common.css modifications to bug 1235780 and bug 1235781.
And I tried to addressed all comments.
Attachment #8702883 - Flags: review?(bgrinstead)
Attachment #8702589 - Attachment is obsolete: true
Attachment #8702589 - Flags: review?(bgrinstead)
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
Attachment #8702883 - Flags: review?(bgrinstead) → review+
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/eae9f44f7444407a28ab3b876032eceebe19310c
Bug 1183280 - Remove most devtools preprocessing rule in CSS files. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/eae9f44f7444
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.