702939 - Split debugger.css between theme and content stylesheets

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

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 (he/him) [:past] (please ni?)]

Attachment: WIP

Still work in progress.

Comment 2

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

Attachment: 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]]

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 (he/him) [:past] (please ni?)]

Attachment: 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)]

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)]

Someone didn't read his history.  What I meant was "Joe's f+ is good enough for me".

Comment 7

[Joe Walker [:jwalker]]

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 (he/him) [:past] (please ni?)]

(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 (he/him) [:past] (please ni?)]

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]]

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 (he/him) [:past] (please ni?)]

Thank you Mihai. Rob will you review this, too, or should I land it in remote-debug?

Comment 12

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

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/90200db603e6