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

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
8 years ago
6 months ago

People

(Reporter: rcampbell, Assigned: sonny)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [console-2])

Attachments

(2 obsolete attachments)

(Reporter)

Description

8 years ago
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

Updated

8 years ago
Whiteboard: [console-2]
(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
"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.
(Assignee)

Updated

7 years ago
Depends on: 603001
(Assignee)

Updated

7 years ago
No longer depends on: 603001
(Assignee)

Updated

7 years ago
Assignee: nobody → sonny.piers
(Assignee)

Updated

7 years ago
Depends on: 703938
(Assignee)

Updated

7 years ago
No longer depends on: 703938
(Assignee)

Updated

7 years ago
Depends on: 703938
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Duplicate of this bug: 603001
(Assignee)

Comment 4

7 years ago
Created attachment 576000 [details] [diff] [review]
patch v1
(Assignee)

Updated

7 years ago
Attachment #576000 - Flags: review?(mihai.sucan)
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!)
Attachment #576000 - Flags: review?(mihai.sucan)
Attachment #576000 - Flags: review?(dao)
Attachment #576000 - Flags: review+
Sonny: please also add the MPL trilicense boilerplate to the new webconsole.css file. See:

http://www.mozilla.org/MPL/boilerplate-1.1/
Component: Developer Tools → Developer Tools: Console
QA Contact: developer.tools → developer.tools.console
Version: unspecified → Trunk
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.
Attachment #576000 - Flags: review?(dao) → review+
(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!
(Assignee)

Comment 9

7 years ago
Created attachment 576694 [details] [diff] [review]
patch v1.1
(Assignee)

Updated

7 years ago
Attachment #576000 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #576000 - Attachment is obsolete: false
(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?
(Assignee)

Comment 11

7 years ago
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?

Updated

7 years ago
Blocks: 704110
This bug has r+'d patches. Any reason why it hasn't landed?
It seems we forgot. Will look into this. Thanks for your reminder!
(Reporter)

Comment 14

7 years ago
will incorporate this with bug 704110.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
Not WONTFIX, then!
No longer blocks: 704110
Status: RESOLVED → REOPENED
Depends on: 704110
Resolution: WONTFIX → ---
Attachment #576000 - Attachment is obsolete: true
Attachment #576694 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → WONTFIX

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.