635359 - move functional rules in toolkit/themes/*.../webConsole.css to content/

Status: RESOLVED WONTFIX

(DevTools :: Console, defect)

Description

[Rob Campbell [:rc] (:robcee)]

There are a number of functional rules in the various webConsole.css files that could be moved to content/ to improve separation of static vs. functional styling.

see:
https://bugzilla.mozilla.org/show_bug.cgi?id=611795#c44

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to comment #0)
> There are a number of functional rules in the various webConsole.css files
> that could be moved to content/ to improve separation of static vs.
> functional styling.

I think you meant toolkit/ here, right? I don't see any css files in content/ besides tests.

Also, here is another mention of the work ideally required:

https://bugzilla.mozilla.org/show_bug.cgi?id=664788#c11

Comment 2

[:Gavin Sharp [email: [e-mail]]]

"content/" as in "toolkit/components/console/hudservice/content/", except that doesn't exist now because hudservice doesn't have any traditional "content" (XUL/CSS/JS files). Doesn't need to be in an actual "content/" subdirectory, that's just a convention.

Comment 4

[Sonny Piers [:sonny]]

Attachment: patch v1

Comment 5

[Mihai Sucan [:msucan]]

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

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

Patch looks good to me, but I believe we should move more stuff into the content css.

See:
https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS

Dão: what do you think?

(Sonny, thank you very much for your contribution!)

Comment 6

[Mihai Sucan [:msucan]]

Sonny: please also add the MPL trilicense boilerplate to the new webconsole.css file. See:

http://www.mozilla.org/MPL/boilerplate-1.1/

Comment 7

[Dão Gottwald [:dao]]

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

> <?xml-stylesheet href="chrome://browser/skin/devtools/webconsole.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://browser/content/devtools/webconsole.css " type="text/css"?>

Flip this around so that content precedes skin.

Looks good otherwise. I'm not sure what other stuff Mihai had in mind, but I'll note that https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS doesn't seem quite correct... I should probably edit it.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 576000 [details] [diff] [review] [diff] [details] [review]
> patch v1
> 
> > <?xml-stylesheet href="chrome://browser/skin/devtools/webconsole.css" type="text/css"?>
> >+<?xml-stylesheet href="chrome://browser/content/devtools/webconsole.css " type="text/css"?>
> 
> Flip this around so that content precedes skin.
> 
> Looks good otherwise. I'm not sure what other stuff Mihai had in mind, but
> I'll note that
> https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS doesn't seem
> quite correct... I should probably edit it.

Please do, thank you very much!

Comment 9

[Sonny Piers [:sonny]]

Attachment: patch v1.1

Comment 10

[Joe Walker [:jwalker]]

(In reply to Sonny Piers [:sonny] from comment #9)
> Created attachment 576694 [details] [diff] [review] [diff] [details] [review]
> patch v1.1

I took a look through the rules to see what should be content CSS, and I think the following properties should move:

-moz-transition
white-space
text-align
-moz-box-orient
-moz-box-direction
overflow
display
position

I'm of the opinion that fundamental layout is not something we should be encouraging themes to change because it's too hard to get right. Certainly padding is theme CSS, but absolute positioning values are probably part of content CSS. So I think the following could be moved too.

height
width
top

I think it would make sense for Dão to comment before you go and make further changes, though.

Also, I'd be interested to know what you think. The best docs we have are: https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS. Dão said he had an issue with this page, but he didn't change anything so I'm assuming he was just referring to the typo in where the files were stored that I fixed.

While we're at it, this looks funny to me:

.jsterm-output-node {
  overflow: auto;
  overflow-x: auto;
}

Is that redundant or am I missing something?

Comment 11

[Sonny Piers [:sonny]]

Joe, thanks for the feedback. I'll trust you on this, distinguishing themes or content related CSS rules is too subtle for me :-) 

Dao, can we have your opinion on this?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

This bug has r+'d patches. Any reason why it hasn't landed?

Comment 13

[Mihai Sucan [:msucan]]

It seems we forgot. Will look into this. Thanks for your reminder!

Comment 14

[Rob Campbell [:rc] (:robcee)]

will incorporate this with bug 704110.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Not WONTFIX, then!