Closed Bug 692409 (darkdebug) Opened 8 years ago Closed 8 years ago

Add a pretty resizer and the dark theme to the debugger

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 15

People

(Reporter: past, Assigned: rcampbell)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(4 files, 31 obsolete files)

220.99 KB, image/png
Details
81.82 KB, image/png
Details
34.24 KB, image/png
Details
8.42 KB, patch
Details | Diff | Splinter Review
Instead of a splitter, we should be using the same pretty resizer as the highlighter toolbar, from bug 691712.
On this note, maybe some elements from the debugger UI should also be resizable, like the stackframes list or property view.
Indeed, one option is to do that in bug 687095. Cedric had suggested that AdaptiveSplitView is or would be soon supporting such interactions.

Another option is using XUL widgets, after we convert debugger.xhtml to XUL (bug 700639).

Otherwise we should file a new bug to fix it in the current UI with CSS flexbox and some mouse event handlers, as we had discussed at one point with Rob.
(In reply to Panos Astithas [:past] from comment #0)
> Instead of a splitter, we should be using the same pretty resizer as the
> highlighter toolbar, from bug 691712.

Another instance of the new resizer is in the console bug 704110. In that bug we can also find styling changes to get the devtools standard dark theme that we need in the debugger as well.
Component: Developer Tools → Developer Tools: Debugger
QA Contact: developer.tools → developer.tools.debugger
Summary: Add a pretty resizer to the debugger → Add a pretty resizer and the dark theme to the debugger
Depends on: 708257
Blocks: 708257
No longer depends on: 708257
Bug 687702 adds a devtools/common.css stylesheet with the dark theme.
Priority: -- → P2
The resizers of the Inspector are not shared (still Inspector-specific). Some work will be needed to share the resizers across all the devtools.

Can we get some more information about where the resizers will be? A screenshot + big-red-arrows would help :)
(In reply to Paul Rouget [:paul] from comment #5)
> The resizers of the Inspector are not shared (still Inspector-specific).
> Some work will be needed to share the resizers across all the devtools.
> 
> Can we get some more information about where the resizers will be? A
> screenshot + big-red-arrows would help :)

For the debugger I'm thinking of copying the HTML panel's behavior, where there seems to be an invisible splitter across the top and a visible one on the right of the toolbar.
So just a resizer at the top and one the top-right corner. I'd like to help here. Because we also want that for the Web Console.
(In reply to Paul Rouget [:paul] from comment #7)
> So just a resizer at the top and one the top-right corner. I'd like to help
> here. Because we also want that for the Web Console.

Sold!
In bug 704110, the "common" patch implements the dark theme for a couple of new widgets (devtools-closebutton, toolbarbutton type=menu-button, toolbarbutton type=menu).
You might want to start with that.

We got rid of the on-the-side resizers.

You will probably need a horizontal resizer like we have in the Style Editor, right?

Let me know what you need.
Attached patch common.css (obsolete) — Splinter Review
common.css from paul's webconsole refactoring.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attached patch darkdebug WIP (obsolete) — Splinter Review
todo: update winstripe and gnomestripe. Based on common.css.
Attached patch darkdebug WIP alternate (obsolete) — Splinter Review
Inverted UI, toolbar on the bottom. See screen.
Attached image screenshot, inverted UI (obsolete) —
(In reply to Rob Campbell [:rc] (robcee) from comment #13)
> Created attachment 601791 [details]
> screenshot, inverted UI

http://bit.ly/ycIu92
(In reply to Victor Porof from comment #14)
> (In reply to Rob Campbell [:rc] (robcee) from comment #13)
> > Created attachment 601791 [details]
> > screenshot, inverted UI
> 
> http://bit.ly/ycIu92

Does that mean it's good or bad? I can't tell.
(In reply to Rob Campbell [:rc] (robcee) from comment #15)
> Does that mean it's good or bad? I can't tell.

Means it's awesome.
Attached patch darkdebug WIP 2 (obsolete) — Splinter Review
toolbar back on top per señor horlander. Text color on label matches buttons now.
Attachment #601785 - Attachment is obsolete: true
Attachment #601790 - Attachment is obsolete: true
Love it! My eyes don't hurt any more.
Attached patch darkdebug WIP 3 (obsolete) — Splinter Review
version 3. copied theme over to winstripe and gnome.
Attachment #601791 - Attachment is obsolete: true
Attachment #601967 - Attachment is obsolete: true
Attached patch common.css (obsolete) — Splinter Review
a slight rebase
Attachment #601783 - Attachment is obsolete: true
Attached patch darkdebug WIP 3.1 (obsolete) — Splinter Review
I spent some time last night struggling with the menulist and our toolbarbutton styling. That's not really going to work. I think using some descendent selectors like,

menulist.devtools-menulist (for the top level "button")

menulist.devtools-menulist > .menulist-label-box > .menulist-label (for the label)

menulist.devtools-menulist > .menulist-dropmarker for the dropmarker.

I tried various combinations of these last night without much success. I'll take a look again today after splitting out the changes from the darkdebug patch that belong in common.css.
Attachment #602103 - Attachment is obsolete: true
Blocks: 732451
Blocks: 732452
Attached patch paul pass (obsolete) — Splinter Review
update of common.css and fixed some XUL stuff. Comes on top of darkdebug WIP 3.1
Attached patch windows attempt (obsolete) — Splinter Review
applied over paul pass patch.

An attempt at windows styling. Got the sizing right, can't seem to get the background fixed on the dropmarker. Also, focused menulist button looks funky.
Attached patch windows attempt (rebased) (obsolete) — Splinter Review
Attachment #603472 - Attachment is obsolete: true
Attachment #608287 - Attachment description: rebased → windows attempt (rebased)
Attached patch paul-pass (rebased) (obsolete) — Splinter Review
Attachment #602460 - Attachment is obsolete: true
Attached patch darkdebug (updated) (obsolete) — Splinter Review
rebased
Attachment #602358 - Attachment is obsolete: true
Attached patch common (rebased) (obsolete) — Splinter Review
rebased
Attachment #602355 - Attachment is obsolete: true
Attached patch updated pinstripe (obsolete) — Splinter Review
Attached patch dark-debug (folded and rebased) (obsolete) — Splinter Review
Attachment #608288 - Attachment is obsolete: true
Attachment #608289 - Attachment is obsolete: true
Attachment #608330 - Attachment is obsolete: true
(In reply to Rob Campbell [:rc] (:robcee) from comment #30)
> mac screenshot:
> http://dl.dropbox.com/u/9927208/Screen%20Shot%202012-03-30%20at%2017.18.49.
> png

There's padding problem in your toolbarbuttons. And I think the label on the very right is expanding vertically the toolbarbuttons (which are probably align:stretched), which make the toolbar a little too high.
yeah, those toolbarbuttons are going to become icons in bug 723059 which I'm hoping will land simultaneously.

We've also got a patch (without a bug?) which updates the state of the Pause/Resume button which should do away with the need for a status label altogether. I'll get rid of it and get that bug filed.

thanks!
Attached patch dark-debug (updated pinstripe) (obsolete) — Splinter Review
need to update gnomestripe and windows, but this should address paul's feedback.
Attachment #611017 - Attachment is obsolete: true
Attached image screenshot os x
showing checked pause button, removed status label, proper toolbar button margins.
Attached patch windows (updated winstripe) (obsolete) — Splinter Review
working well, but missing a black border on the toolbar splitter and have an odd focus styling issue on the menulist. Focused menulist has a hard blue background (similar to the pushed state on our buttons).
Attachment #608287 - Attachment is obsolete: true
Attachment #611606 - Attachment is obsolete: true
Attachment #611570 - Attachment is obsolete: true
Attachment #608290 - Attachment is obsolete: true
Attachment #612224 - Flags: review?(paul)
Attachment #612223 - Flags: review?(paul)
Attachment #612219 - Flags: review?(paul)
Still working on the review, but I have one question: what is the point of using HTML in debugger.xul? Can we use XUL everywhere and get read of the XHTML namespace and get the XUL one by default? Is it easy to do? (not asking to get that in this patch)
First pass, more to come:

(no need to split the patch in 3 patches)

------

Use class="devtools-horizontal-splitter" for the HTML Tree here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/TreePanel.jsm#166
and remove the related splitter code from theme/*/browser.css.

> +++ b/browser/devtools/debugger/debugger.xul
> +      <xul:menulist id="scripts"
> +                    class="devtools-menulist"/>

Useless carriage return.

> +    <div id="dbg-content" class="hbox flex">
> +      <div id="stack" class="vbox">
> +        <div id="stackframes" class="vbox flex"></div>
> +      </div>
> +      <div id="script" class="vbox flex">
> +        <div id="editor" class="vbox flex"></div>
> +      </div>
> +      <div id="properties" class="vbox">
> +        <div id="variables" class="vbox flex"></div>
> +      </div>

<div class"vbox flex">? Why not <vbox flex="1">? (not blocking, see comment 40)

browser/themes/winstripe/devtools/common.css/common.css
>  /* Search input */
>  
>  .devtools-searchinput {
>    -moz-appearance: none;
>    margin: 0 3px;
>    border: 1px solid hsla(211,68%,6%,.6);

In (gnome|pin)stipe, we fix a searchinput typo:
> -  background-position: 4px 3px, top left, top left;
> +  background-position: 4px center, top left, top left;

Can you do that here to please?

> +/* Close button */
> +
> +/* copied viciously from winstripe/browser.css::highlighter-closebutton; */

It should be removed from browser.css.
Second pass, more to come:

Focus Handling:

We jump for the web content to the menulist directly (then to the iframe).
Buttons should be tabbable (just add tabbindex=0 to the buttons).

+++ b/browser/themes/gnomestripe/devtools/common.css
>  /* Toolbar and Toolbar items */
>  
>  .devtools-toolbar {
>    -moz-appearance: none;
>    padding: 4px 3px;
>    box-shadow: 0 1px 0 0 hsla(210, 16%, 76%, .2) inset;
>    background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
> +  color: hsl(210,30%,85%);
> +  text-shadow: 0 -1px 0 hsla(210,8%,5%,.45);
>  }

(pin|win|gnome)stripe: This is not needed since there's no label in the toolbar anymore.

----
+++ b/browser/themes/pinstripe/devtools/common.css

> +.devtools-menulist,
>  .devtools-toolbarbutton {
>    -moz-appearance: none;
>    min-width: 78px;
>    min-height: 22px;
>    color: hsl(210,30%,85%);
>    text-shadow: 0 -1px 0 hsla(210,8%,5%,.45);
>    border: 1px solid hsla(210,8%,5%,.45);
>    border-radius: @toolbarbuttonCornerRadius@;
>    background: -moz-linear-gradient(hsla(212,7%,57%,.35), hsla(212,7%,57%,.1)) padding-box;
>    box-shadow: 0 1px 0 hsla(210,16%,76%,.15) inset, 0 0 0 1px hsla(210,16%,76%,.15) inset, 0 1px 0 hsla(210,16%,76%,.15);
> +  padding: 0 1px;
> +  margin: 0 3px;
>  }

Is that really needed? (pinstripe and winstripe)

---

> +/* Same look for:
> + *  <toolbarbutton type=menu>, <toolbarbutton type=menu-button>, <menulist>
> + */

Add this comment to (gnome|win)stripe too.

---

+++ b/browser/themes/winstripe/devtools/common.css

> +.devtools-menulist,
>  .devtools-toolbarbutton {
>    -moz-appearance: none;
>    min-width: 78px;
>    min-height: 22px;
>    color: hsl(210,30%,85%);
>    text-shadow: 0 -1px 0 hsla(210,8%,5%,.45);
>    border: 1px solid hsla(211,68%,6%,.5);
>    border-radius: 3px;
>    background: -moz-linear-gradient(hsla(209,13%,54%,.35), hsla(209,13%,54%,.1) 85%, hsla(209,13%,54%,.2)) padding-box;
>    box-shadow: 0 1px 0 hsla(209,29%,72%,.15) inset, 0 0 0 1px hsla(209,29%,72%,.1) inset, 0 0 0 1px hsla(209,29%,72%,.1), 0 1px 0 hsla(210,16%,76%,.1);
> -  -moz-margin-end: 3px;
> +  padding: 0 1px;
> +  margin: 0 3px;
>  }

Is that really needed? It has an impact on the Inspector toolbar too.

> -.devtools-toolbarbutton > .toolbarbutton-icon {
> -  margin: 0;
> +.devtools-toolbarbutton > .toolbarbutton-text {
> +  /* margin: 1px 6px; */
>  }

Why these 2 modifications?
Why the comment?

--- a/browser/themes/winstripe/devtools/common.css

> -.devtools-toolbarbutton:not([checked]):hover:active {
> +.devtools-toolbarbutton:not([checked=true]):hover:active {
>    background-color: hsla(210,18%,9%,.1);
>    background-image: -moz-linear-gradient(hsla(209,13%,54%,.35), hsla(209,13%,54%,.1) 85%, hsla(209,13%,54%,.2));
> +  box-shadow: 0 0 3px hsla(211,68%,6%,.5) inset, 0 0 0 1px hsla(209,29%,72%,.1), 0 1px 0 hsla(210,16%,76%,.1);
> +}

I don't think this shadow value is right (this is for pinstripe).
You want to use this: box-shadow: 0 1px 3px hsla(211,68%,6%,.5) inset, 0 0 0 1px hsla(209,29%,72%,.1), 0 1px 0 hsla(210,16%,76%,.1);
(In reply to Paul Rouget [:paul] from comment #40)
> Still working on the review, but I have one question: what is the point of
> using HTML in debugger.xul? Can we use XUL everywhere and get read of the
> XHTML namespace and get the XUL one by default? Is it easy to do? (not
> asking to get that in this patch)

I don't think it's hard to do. I'm not sure of the original reasoning behind having it in HTML.

I'like to do that in a follow-up if possible.

(In reply to Paul Rouget [:paul] from comment #41)
> First pass, more to come:
> 
> (no need to split the patch in 3 patches)
> 
> ------
> 
> Use class="devtools-horizontal-splitter" for the HTML Tree here:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/
> TreePanel.jsm#166
> and remove the related splitter code from theme/*/browser.css.

I was explicitly avoiding that here to avoid browser style changes.

Follow-up?

> browser/themes/winstripe/devtools/common.css/common.css
> >  /* Search input */
> >  
> >  .devtools-searchinput {
> >    -moz-appearance: none;
> >    margin: 0 3px;
> >    border: 1px solid hsla(211,68%,6%,.6);
> 
> In (gnome|pin)stipe, we fix a searchinput typo:
> > -  background-position: 4px 3px, top left, top left;
> > +  background-position: 4px center, top left, top left;
> 
> Can you do that here to please?

meaning in winstripe? Not sure where you mean. :)

> > +/* Close button */
> > +
> > +/* copied viciously from winstripe/browser.css::highlighter-closebutton; */
> 
> It should be removed from browser.css.

Again, can I lump that into the same followup as the html panel splitter?

Comments in #42 look good. I'll address those.
(I am ok with follow-ups)

> > Use class="devtools-horizontal-splitter" for the HTML Tree here:
> > http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/
> > TreePanel.jsm#166
> > and remove the related splitter code from theme/*/browser.css.
>
> I was explicitly avoiding that here to avoid browser style changes.

If you want to avoid browser changes, do it completely (the common.css patch touches browser.css).

---

> > In (gnome|pin)stipe, we fix a searchinput typo:
> > -  background-position: 4px 3px, top left, top left;
> > +  background-position: 4px center, top left, top left;
> > 
> > Can you do that here to please?
> 
> meaning in winstripe? Not sure where you mean. :)

In winstripe/…/common.css, there this line:
background-position: 4px 3px, top left, top left;

Can you change it to:

background-position: 4px center, top left, top left;
Last pass:

RTL problem: the dropdown border is on the wrong side (I guess you need to add !important to the rtl rule set).

On Windows, the splitter is not black, and when the menulist is focused, it has this ugly bright blue background.
(In reply to Rob Campbell [:rc] (:robcee) from comment #43)
> (In reply to Paul Rouget [:paul] from comment #40)
> > Still working on the review, but I have one question: what is the point of
> > using HTML in debugger.xul? Can we use XUL everywhere and get read of the
> > XHTML namespace and get the XUL one by default? Is it easy to do? (not
> > asking to get that in this patch)
> 
> I don't think it's hard to do. I'm not sure of the original reasoning behind
> having it in HTML.

Originally this was all HTML, with the intention of possibly serving the UI from the debuggee (like WebKit) and having other browsers debug Firefox, and also because HTML Rocks. In order to fix RTL bugs, I converted a lot of it of it to XUL, but just the minimum amount possible, because HTML will be RTL-friendly Real Soon Now. At this point all I will say is: frankly my dear, I don't give a damn.
Attachment #612219 - Flags: review?(paul)
Attachment #612223 - Flags: review?(paul)
Attachment #612224 - Flags: review?(paul)
(In reply to Paul Rouget [:paul] from comment #41)
> First pass, more to come:
> 
> (no need to split the patch in 3 patches)
> 
> ------
> 
> Use class="devtools-horizontal-splitter" for the HTML Tree here:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/
> TreePanel.jsm#166
> and remove the related splitter code from theme/*/browser.css.
> 
> > +++ b/browser/devtools/debugger/debugger.xul
> > +      <xul:menulist id="scripts"
> > +                    class="devtools-menulist"/>
> 
> Useless carriage return.
> 
> > +    <div id="dbg-content" class="hbox flex">
> > +      <div id="stack" class="vbox">
> > +        <div id="stackframes" class="vbox flex"></div>
> > +      </div>
> > +      <div id="script" class="vbox flex">
> > +        <div id="editor" class="vbox flex"></div>
> > +      </div>
> > +      <div id="properties" class="vbox">
> > +        <div id="variables" class="vbox flex"></div>
> > +      </div>
> 
> <div class"vbox flex">? Why not <vbox flex="1">? (not blocking, see comment
> 40)
> 
> browser/themes/winstripe/devtools/common.css/common.css
> >  /* Search input */
> >  
> >  .devtools-searchinput {
> >    -moz-appearance: none;
> >    margin: 0 3px;
> >    border: 1px solid hsla(211,68%,6%,.6);
> 
> In (gnome|pin)stipe, we fix a searchinput typo:
> > -  background-position: 4px 3px, top left, top left;
> > +  background-position: 4px center, top left, top left;
> 
> Can you do that here to please?

done all of these.

> > +/* Close button */
> > +
> > +/* copied viciously from winstripe/browser.css::highlighter-closebutton; */
> 
> It should be removed from browser.css.

Do we want to do that now and update the highlighter toolbar with the correct class from common.css? That feels a little scary.
(In reply to Paul Rouget [:paul] from comment #42)
> Second pass, more to come:
> 
> Focus Handling:
> 
> We jump for the web content to the menulist directly (then to the iframe).
> Buttons should be tabbable (just add tabbindex=0 to the buttons).

done.

> +++ b/browser/themes/gnomestripe/devtools/common.css
> >  /* Toolbar and Toolbar items */
> >  
> >  .devtools-toolbar {
> >    -moz-appearance: none;
> >    padding: 4px 3px;
> >    box-shadow: 0 1px 0 0 hsla(210, 16%, 76%, .2) inset;
> >    background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
> > +  color: hsl(210,30%,85%);
> > +  text-shadow: 0 -1px 0 hsla(210,8%,5%,.45);
> >  }
> 
> (pin|win|gnome)stripe: This is not needed since there's no label in the
> toolbar anymore.

ah yes, done.

> +++ b/browser/themes/pinstripe/devtools/common.css
> 
> > +.devtools-menulist,
> >  .devtools-toolbarbutton {
> >    -moz-appearance: none;
> >    min-width: 78px;
> >    min-height: 22px;
> >    color: hsl(210,30%,85%);
> >    text-shadow: 0 -1px 0 hsla(210,8%,5%,.45);
> >    border: 1px solid hsla(210,8%,5%,.45);
> >    border-radius: @toolbarbuttonCornerRadius@;
> >    background: -moz-linear-gradient(hsla(212,7%,57%,.35), hsla(212,7%,57%,.1)) padding-box;
> >    box-shadow: 0 1px 0 hsla(210,16%,76%,.15) inset, 0 0 0 1px hsla(210,16%,76%,.15) inset, 0 1px 0 hsla(210,16%,76%,.15);
> > +  padding: 0 1px;
> > +  margin: 0 3px;
> >  }
> 
> Is that really needed? (pinstripe and winstripe)

I think these crept in here from when I was doing the icon patch. Removing.

> > +/* Same look for:
> > + *  <toolbarbutton type=menu>, <toolbarbutton type=menu-button>, <menulist>
> > + */
> 
> Add this comment to (gnome|win)stripe too.

done.

> +++ b/browser/themes/winstripe/devtools/common.css
> 
> > +.devtools-menulist,
> >  .devtools-toolbarbutton {
> >    -moz-appearance: none;
> >    min-width: 78px;
> >    min-height: 22px;
> >    color: hsl(210,30%,85%);
> >    text-shadow: 0 -1px 0 hsla(210,8%,5%,.45);
> >    border: 1px solid hsla(211,68%,6%,.5);
> >    border-radius: 3px;
> >    background: -moz-linear-gradient(hsla(209,13%,54%,.35), hsla(209,13%,54%,.1) 85%, hsla(209,13%,54%,.2)) padding-box;
> >    box-shadow: 0 1px 0 hsla(209,29%,72%,.15) inset, 0 0 0 1px hsla(209,29%,72%,.1) inset, 0 0 0 1px hsla(209,29%,72%,.1), 0 1px 0 hsla(210,16%,76%,.1);
> > -  -moz-margin-end: 3px;
> > +  padding: 0 1px;
> > +  margin: 0 3px;
> >  }
> 
> Is that really needed? It has an impact on the Inspector toolbar too.

probably the icon patch again. excising.

should I re-add -moz-margin-end: 3px;

> 
> > -.devtools-toolbarbutton > .toolbarbutton-icon {
> > -  margin: 0;
> > +.devtools-toolbarbutton > .toolbarbutton-text {
> > +  /* margin: 1px 6px; */
> >  }
> 
> Why these 2 modifications?
> Why the comment?

icon patch. restored.

> --- a/browser/themes/winstripe/devtools/common.css
> 
> > -.devtools-toolbarbutton:not([checked]):hover:active {
> > +.devtools-toolbarbutton:not([checked=true]):hover:active {
> >    background-color: hsla(210,18%,9%,.1);
> >    background-image: -moz-linear-gradient(hsla(209,13%,54%,.35), hsla(209,13%,54%,.1) 85%, hsla(209,13%,54%,.2));
> > +  box-shadow: 0 0 3px hsla(211,68%,6%,.5) inset, 0 0 0 1px hsla(209,29%,72%,.1), 0 1px 0 hsla(210,16%,76%,.1);
> > +}
> 
> I don't think this shadow value is right (this is for pinstripe).
> You want to use this: box-shadow: 0 1px 3px hsla(211,68%,6%,.5) inset, 0 0 0
> 1px hsla(209,29%,72%,.1), 0 1px 0 hsla(210,16%,76%,.1);

ok, corrected. Thanks, testing and will update the attachment.
Attached patch dark debug v1.5 (obsolete) — Splinter Review
addressed paul's review comments. Still haven't finished windows. You can defer this review until that's done or take a pass through for any obvious problems. Reviewer's choice!
Attachment #612219 - Attachment is obsolete: true
Attachment #612223 - Attachment is obsolete: true
Attachment #612224 - Attachment is obsolete: true
Attachment #615481 - Flags: review?(jwalker)
Attached patch dark debug v1.5.1 (obsolete) — Splinter Review
found a minor nit. setAttribute->removeAttribute in debugger-view.js.
Attachment #615481 - Attachment is obsolete: true
Attachment #615482 - Flags: review?(jwalker)
Attachment #615481 - Flags: review?(jwalker)
Comment on attachment 615482 [details] [diff] [review]
dark debug v1.5.1

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

::: browser/base/content/browser.xul
@@ +1000,4 @@
>               hidden="true">
>  #ifdef XP_MACOSX
>        <toolbarbutton id="highlighter-closebutton"
> +                     class="devtools-closebutton"

Thief!

::: browser/devtools/debugger/debugger.css
@@ -51,5 @@
> - * Debugger statusbar
> - */
> -
> -#dbg-statusbar {
> -  -moz-appearance: statusbar;

According to https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS this should be in theme CSS

::: browser/themes/gnomestripe/browser.css
@@ -1987,5 @@
> -  margin-bottom: 0;
> -}
> -
> -#highlighter-closebutton > .toolbarbutton-icon {
> -  /* XXX Buttons have padding in widget/ that we don't want here but can't override with good CSS, so we must

Did you mean that you've not finished linux, or did I get the wrong impression from XXX?

::: browser/themes/pinstripe/devtools/debugger.css
@@ -60,5 @@
> -#dbg-toolbar > button {
> -  text-align: center;
> -}
> -
> -#dbg-toolbar > button, menulist {

Did you mean:

#dbg-toolbar > button,
menulist {
  ...

(Which would be a nit)
Or perhaps more likely

#dbg-toolbar > button,
#dbg-toolbar > menulist {
  ...

(In which case I actually found something of value)
(In reply to Joe Walker from comment #51)
> Comment on attachment 615482 [details] [diff] [review]
> dark debug v1.5.1
> 
> Review of attachment 615482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.xul
> @@ +1000,4 @@
> >               hidden="true">
> >  #ifdef XP_MACOSX
> >        <toolbarbutton id="highlighter-closebutton"
> > +                     class="devtools-closebutton"
> 
> Thief!

one of us will get this. :)

> ::: browser/devtools/debugger/debugger.css
> @@ -51,5 @@
> > - * Debugger statusbar
> > - */
> > -
> > -#dbg-statusbar {
> > -  -moz-appearance: statusbar;
> 
> According to https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS
> this should be in theme CSS

oh, good find. That can go away anyway.

> ::: browser/themes/gnomestripe/browser.css
> @@ -1987,5 @@
> > -  margin-bottom: 0;
> > -}
> > -
> > -#highlighter-closebutton > .toolbarbutton-icon {
> > -  /* XXX Buttons have padding in widget/ that we don't want here but can't override with good CSS, so we must
> 
> Did you mean that you've not finished linux, or did I get the wrong
> impression from XXX?

No Linux should be good to go. I think we can remove that comment.

> ::: browser/themes/pinstripe/devtools/debugger.css
> @@ -60,5 @@
> > -#dbg-toolbar > button {
> > -  text-align: center;
> > -}
> > -
> > -#dbg-toolbar > button, menulist {
> 
> Did you mean:
> 
> #dbg-toolbar > button,
> menulist {
>   ...
> 
> (Which would be a nit)
> Or perhaps more likely
> 
> #dbg-toolbar > button,
> #dbg-toolbar > menulist {
>   ...
> 
> (In which case I actually found something of value)

those are in debugger.css again? I think those can leave as well.
(In reply to Rob Campbell [:rc] (:robcee) from comment #52)
> (In reply to Joe Walker from comment #51)

> > ::: browser/devtools/debugger/debugger.css
> > @@ -51,5 @@
> > > - * Debugger statusbar
> > > - */
> > > -
> > > -#dbg-statusbar {
> > > -  -moz-appearance: statusbar;
> > 
> > According to https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS
> > this should be in theme CSS
> 
> oh, good find. That can go away anyway.

replying to myself like a crazy person. Those were removals and they are in themes now.

> > ::: browser/themes/gnomestripe/browser.css
> > @@ -1987,5 @@
> > > -  margin-bottom: 0;
> > > -}
> > > -
> > > -#highlighter-closebutton > .toolbarbutton-icon {
> > > -  /* XXX Buttons have padding in widget/ that we don't want here but can't override with good CSS, so we must
> > 
> > Did you mean that you've not finished linux, or did I get the wrong
> > impression from XXX?
> 
> No Linux should be good to go. I think we can remove that comment.

Looking again, I think this comment should stay for now. Specific to linux for the closebutton, though there are likely alternate ways to do this.

(adding an image url to the toolbarbutton for example and removing margins for the toolbarbutton text for example, might work)

> > ::: browser/themes/pinstripe/devtools/debugger.css
> > @@ -60,5 @@
> > > -#dbg-toolbar > button {
> > > -  text-align: center;
> > > -}
> > > -
> > > -#dbg-toolbar > button, menulist {
> > 
> > Did you mean:
> > 
> > #dbg-toolbar > button,
> > menulist {
> >   ...
> > 
> > (Which would be a nit)
> > Or perhaps more likely
> > 
> > #dbg-toolbar > button,
> > #dbg-toolbar > menulist {
> >   ...
> > 
> > (In which case I actually found something of value)
> 
> those are in debugger.css again? I think those can leave as well.

also, removals, don't apply here.
This is obvious, but just to make sure we've got it logged; bug 720641 adds an instance of class="highlighter-closebutton" (see id="developer-toolbar-closebutton").
We're renaming highlighter-closebutton to devtools-closebutton (either here or in bug 744906, whoever gets there first). When we do that rename, we shouldn't forget the instance that I'm adding in bug 720641.
Attached patch dark debug v1.5.2 (rebased) (obsolete) — Splinter Review
Attachment #615482 - Attachment is obsolete: true
Attachment #615482 - Flags: review?(jwalker)
Attached patch dark debug v1.6 (obsolete) — Splinter Review
Attachment #616546 - Attachment is obsolete: true
Attached image Windows Screenshot
Attached patch dark debug v1.6.1 (obsolete) — Splinter Review
restored the mac splitter code.
Attachment #616706 - Attachment is obsolete: true
Attachment #616937 - Flags: review?(paul)
Comment on attachment 616937 [details] [diff] [review]
dark debug v1.6.1

ok, this is finally getting close. I need the style master to help me get it over the line. !summon dão!
Attachment #616937 - Flags: review?(dao)
Attached patch dark debug v1.6.2 (obsolete) — Splinter Review
updated, rebased on latest changes in fx-team.
Attachment #616937 - Attachment is obsolete: true
Attachment #617533 - Flags: review?(dao)
Attachment #616937 - Flags: review?(paul)
Attachment #616937 - Flags: review?(dao)
Attachment #617533 - Attachment is patch: true
Comment on attachment 617533 [details] [diff] [review]
dark debug v1.6.2

>-.devtools-toolbarbutton:not([checked]):hover:active {
>+.devtools-toolbarbutton:not([checked=true]):hover:active {

Why these changes?
Comment on attachment 617533 [details] [diff] [review]
dark debug v1.6.2

suspending review request for now. Apparently this has some problems on linux (and likely other platforms) we need to address. Related to some of the changes made to common.css recently by the inspector.
Attachment #617533 - Flags: review?(dao)
Depends on: 749626
Depends on: 751164
Attached patch slim patch v0.1 (obsolete) — Splinter Review
attempt to reduce the current work to the strictly necessary. Not tested on Windows (but should be ok) and resizer not styled yet.
Attached patch slim patch v0.2 (obsolete) — Splinter Review
resizer Mac and Linux
Attachment #621556 - Attachment is obsolete: true
Attached patch slim patch v0.999 (obsolete) — Splinter Review
Windows style
Attachment #621558 - Attachment is obsolete: true
Attached patch slim patch v1 (obsolete) — Splinter Review
Attachment #621562 - Attachment is obsolete: true
Comment on attachment 621599 [details] [diff] [review]
slim patch v1

If this patch is ok for you, please mark attachment 617533 [details] [diff] [review] as obsolete.
Attachment #621599 - Flags: review?(rcampbell)
Alias: darkdebug
Attachment #617533 - Attachment is obsolete: true
Comment on attachment 621599 [details] [diff] [review]
slim patch v1

good good.
Attachment #621599 - Flags: review?(rcampbell) → review+
Attached patch patch v1.1Splinter Review
rebased
Attachment #621599 - Attachment is obsolete: true
We have some incorrect pieces in theme css now:

11:21 < victorporof> robcee: there's dbg-content > * > .vbox and dbg-content > * > .title in 
                     debugger theme css
11:21 < victorporof> i guess that should've been removed?

Should we do that in here or in a separate patch?
(In reply to Rob Campbell [:rc] (:robcee) from comment #70)
> We have some incorrect pieces in theme css now:
> 
> 11:21 < victorporof> robcee: there's dbg-content > * > .vbox and dbg-content
> > * > .title in 
>                      debugger theme css
> 11:21 < victorporof> i guess that should've been removed?
> 
> Should we do that in here or in a separate patch?

scratch the above. Victor's going to do this in bug 753313.
https://hg.mozilla.org/mozilla-central/rev/32a259ecff5e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.