1246733 - Remove stylesheet references of 'devtools/skin/common.css' and instead import it in theme files

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

To simplify the boilerplate markup for tools and frames we should handle common.css imports from the theme level, rather than relying on it existing in markup.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1246733 - Remove stylesheet references of 'devtools/skin/common.css' and instead import it in theme files;r=ntim,r=ochameau

Review commit: https://reviewboard.mozilla.org/r/34045/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34045/

Comment 2

[Brian Grinstead [:bgrins]]

Screenshot comparisons: http://screenshots.mattn.ca/compare/?oldProject=try&oldRev=dc5a8fa40cf5&newProject=try&newRev=288bdb0dd9b4.  The 'debugger' differences there are fixed now - I had deleted an extra css file on accident and caught it through that UI.

Comment 3

[Brian Grinstead [:bgrins]]

Alex, this fits in with general css cleanup I think (one less thing for a new tool to include), and is now possible as of Bug 1235781.  Let me know if you see any issues with doing this.  Note there is still references to common.css in webide (since it's not loading theme-switching.js)

Tim, if you have a chance can you see if this causes any style issues for you locally?

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8717721 [details]
MozReview Request: Bug 1246733 - Remove stylesheet references of 'devtools/skin/common.css' and instead import it in theme files;r=ntim,r=ochameau

https://reviewboard.mozilla.org/r/34045/#review31037

Looks good for documents (ensure reviewing projecteditor.xul).
I have some comments about the tests.
May be that's fine and then we could potentially remove some other css imports.
Or may be we should ensure importing theme-switching.js in tests.

But if try is happy with that change, I'm ok to proceed.

::: devtools/client/projecteditor/chrome/content/projecteditor.xul
(Diff revision 1)
> -<?xml-stylesheet href="chrome://devtools/skin/common.css" type="text/css"?>

This file doesn't load theme-switching.
It pulls light-theme.css manually few lines before.
Isn't that an issue to remove common.css?

::: devtools/client/shared/test/browser_tableWidget_basic.js:9
(Diff revision 1)
>    "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" +

You are removing common.css in many tests.
I'm fine doing so, but then I'm wondering, what is the point of adding any css file in test if they prove to not be useful?
Is global.css, light-theme.css useful?
May be widgets.css isn't important?
Or may be we should inject theme-switching.js instead?

::: devtools/client/shared/test/doc_options-view.xul
(Diff revision 1)
> -<?xml-stylesheet href="chrome://devtools/skin/common.css" type="text/css"?>

This document doesn't import theme-switching, but that's a test. So some comment than the previous one.

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8717721 [details]
MozReview Request: Bug 1246733 - Remove stylesheet references of 'devtools/skin/common.css' and instead import it in theme files;r=ntim,r=ochameau

https://reviewboard.mozilla.org/r/34045/#review31317

Looks good to me ! We should probably consider cleaning up our stylesheets, and keep a single shared stylesheet, but that's better as a followup

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8717721 [details]
> MozReview Request: Bug 1246733 - Remove stylesheet references of
> 'devtools/skin/common.css' and instead import it in theme
> files;r=ntim,r=ochameau
> 
> https://reviewboard.mozilla.org/r/34045/#review31037
> 
> Looks good for documents (ensure reviewing projecteditor.xul).
> I have some comments about the tests.
> May be that's fine and then we could potentially remove some other css
> imports.
> Or may be we should ensure importing theme-switching.js in tests.
> 
> But if try is happy with that change, I'm ok to proceed.
> 
> ::: devtools/client/projecteditor/chrome/content/projecteditor.xul
> (Diff revision 1)
> > -<?xml-stylesheet href="chrome://devtools/skin/common.css" type="text/css"?>
> 
> This file doesn't load theme-switching.
> It pulls light-theme.css manually few lines before.
> Isn't that an issue to remove common.css?

Good catch, thanks.  Yes, I've re-added it.

> ::: devtools/client/shared/test/browser_tableWidget_basic.js:9
> (Diff revision 1)
> >    "<?xml-stylesheet href='chrome://devtools/skin/light-theme.css'?>" +
> 
> You are removing common.css in many tests.
> I'm fine doing so, but then I'm wondering, what is the point of adding any
> css file in test if they prove to not be useful?
> Is global.css, light-theme.css useful?
> May be widgets.css isn't important?
> Or may be we should inject theme-switching.js instead?
> 
> ::: devtools/client/shared/test/doc_options-view.xul
> (Diff revision 1)
> > -<?xml-stylesheet href="chrome://devtools/skin/common.css" type="text/css"?>
> 
> This document doesn't import theme-switching, but that's a test. So some
> comment than the previous one.

Removing it it tests doesn't cause anything to fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bda90a612a76&selectedJob=16795532 *shrug*

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/365535caf118

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/365535caf118