Last Comment Bug 702939 - Split debugger.css between theme and content stylesheets
: Split debugger.css between theme and content stylesheets
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on: 700639
Blocks: minotaur 697762
  Show dependency treegraph
 
Reported: 2011-11-16 04:53 PST by Panos Astithas [:past]
Modified: 2012-01-04 03:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (7.78 KB, patch)
2011-11-30 08:44 PST, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch (22.48 KB, patch)
2011-12-01 10:55 PST, Panos Astithas [:past]
jwalker: feedback+
Details | Diff | Splinter Review
Working patch v2 (23.86 KB, patch)
2011-12-02 05:07 PST, Panos Astithas [:past]
jwalker: review+
mihai.sucan: review+
rcampbell: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-11-16 04:53:35 PST
Theme-related stylesheets are placed in browser/themes/*stripe/browser/devtools/. See the way the style inspector stylesheets are split up for an example. We should do the same for the debugger UI, if there is content-related stuff, or move it to themes otherwise.
Comment 1 Panos Astithas [:past] 2011-11-30 08:44:44 PST
Created attachment 577978 [details] [diff] [review]
WIP

Still work in progress.
Comment 2 Panos Astithas [:past] 2011-12-01 10:55:49 PST
Created attachment 578317 [details] [diff] [review]
Working patch

I also fixed a few styling issues on all platforms while I was here. Tested on OS X/Win7/WinXP/Linux. Joe, I don't suppose you have tried the debugger yet, but can you take a look at the patch (what I put in theme css and what I left in content) and tell me if you see any issues?
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-02 02:25:52 PST
Comment on attachment 578317 [details] [diff] [review]
Working patch

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

Looks good to me.
Only 2 comments, and perhaps we need a second opinion on the second.

::: browser/devtools/debugger/debugger.css
@@ -83,5 @@
> -    background-color: #fff;
> -    border: 1px solid #bbb;
> -    box-shadow: 0 -5px 10px -3px #ccc;
> -    -moz-margin-end: -1px;
> -    cursor: default;

I think 'cursor', while visual, is mostly functional. It tells the user what the UI does, and using the wrong cursor isn't just visually annoying, it's misleading - so it belongs in content.

@@ -95,5 @@
> - * Debugger statusbar
> - */
> -
> -#dbg-statusbar {
> -    -moz-appearance: statusbar;

So this is probably contentious. What we're saying here is 'looks like a statusbar'. A status bar isn't mearly visual. It comes with expectations about how it functions. So I'm leaning to this being functional too.

Put it another way - is it reasonable to expect a theme to do -moz-appearance: treetwisty; at this point? I would say probably not.
Comment 4 Panos Astithas [:past] 2011-12-02 05:07:14 PST
Created attachment 578551 [details] [diff] [review]
Working patch v2

(In reply to Joe Walker from comment #3)
> Looks good to me.
> Only 2 comments, and perhaps we need a second opinion on the second.

Agreed on both counts. I was already on the fence regarding cursor, anyway.
Thank you!
Comment 5 Dave Camp (:dcamp) 2011-12-02 11:03:07 PST
Comment on attachment 578551 [details] [diff] [review]
Working patch v2

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

I'm going to hand this off to Joe Walker.
Comment 6 Dave Camp (:dcamp) 2011-12-02 14:48:42 PST
Someone didn't read his history.  What I meant was "Joe's f+ is good enough for me".
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-05 07:08:36 PST
Comment on attachment 578551 [details] [diff] [review]
Working patch v2

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

::: browser/devtools/debugger/debugger.css
@@ +141,5 @@
>      display: -moz-box;
>      -moz-box-orient: vertical;
>  }
>  
>  /**

Evidence that we might not have our opinions on the theme/content nature of -moz-appearance quite right: We just replaced something that was 'just into content' with something equivalent that is clearly theme.

I suspect the truth is that the rules are not clear and that we have to use our judgement.
Comment 8 Panos Astithas [:past] 2011-12-05 08:35:26 PST
(In reply to Joe Walker from comment #7)
> Comment on attachment 578551 [details] [diff] [review] [diff] [details] [review]
> Working patch v2
> 
> Review of attachment 578551 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger.css
> @@ +141,5 @@
> >      display: -moz-box;
> >      -moz-box-orient: vertical;
> >  }
> >  
> >  /**
> 
> Evidence that we might not have our opinions on the theme/content nature of
> -moz-appearance quite right: We just replaced something that was 'just into
> content' with something equivalent that is clearly theme.
> 
> I suspect the truth is that the rules are not clear and that we have to use
> our judgement.

Yeah, it doesn't help that treetwisties in particular don't work in windows, forcing us to move them to theme, although it's something that the debugger heavily depends on not being broken. Perhaps we should add a note in the wiki page that moving stuff to theme css is occasionally done for working around platform quirks and overriding behavior, and not just because theme add-ons might want to change it.
Comment 9 Panos Astithas [:past] 2011-12-13 11:23:39 PST
Comment on attachment 578551 [details] [diff] [review]
Working patch v2

We think this should be included when landing the debugger in m-c, so one of you guys (or both) should review it. This does not touch toolkit code.
Comment 10 Mihai Sucan [:msucan] 2011-12-14 10:31:44 PST
Comment on attachment 578551 [details] [diff] [review]
Working patch v2

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

Looks good to me.
Comment 11 Panos Astithas [:past] 2011-12-15 00:07:42 PST
Thank you Mihai. Rob will you review this, too, or should I land it in remote-debug?

Note You need to log in before you can comment on or make changes to this bug.