Closed Bug 1246733 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

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.
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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 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.
Attachment #8717721 - Flags: review?(poirot.alex) → review+
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
Attachment #8717721 - Flags: review?(ntim.bugs) → review+
(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*
https://hg.mozilla.org/mozilla-central/rev/365535caf118
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.