Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 692409 (darkdebug)

Add a pretty resizer and the dark theme to the debugger

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: past, Assigned: rc)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 31 obsolete attachments)

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.
Blocks: 676586
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

Updated

6 years ago
Blocks: 708257
No longer depends on: 708257
Bug 687702 adds a devtools/common.css stylesheet with the dark theme.
Priority: -- → P2

Comment 5

6 years ago
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.

Comment 7

6 years ago
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!
Blocks: 723059

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
Created attachment 601783 [details] [diff] [review]
common.css

common.css from paul's webconsole refactoring.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
(Assignee)

Comment 11

6 years ago
Created attachment 601785 [details] [diff] [review]
darkdebug WIP

todo: update winstripe and gnomestripe. Based on common.css.
(Assignee)

Comment 12

6 years ago
Created attachment 601790 [details] [diff] [review]
darkdebug WIP alternate

Inverted UI, toolbar on the bottom. See screen.
(Assignee)

Comment 13

6 years ago
Created attachment 601791 [details]
screenshot, inverted UI
(In reply to Rob Campbell [:rc] (robcee) from comment #13)
> Created attachment 601791 [details]
> screenshot, inverted UI

http://bit.ly/ycIu92
(Assignee)

Comment 15

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
Created attachment 601967 [details] [diff] [review]
darkdebug WIP 2

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.
(Assignee)

Comment 19

6 years ago
Created attachment 602103 [details] [diff] [review]
darkdebug WIP 3

version 3. copied theme over to winstripe and gnome.
Attachment #601791 - Attachment is obsolete: true
Attachment #601967 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Created attachment 602355 [details] [diff] [review]
common.css

a slight rebase
Attachment #601783 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 602358 [details] [diff] [review]
darkdebug WIP 3.1

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
(Assignee)

Updated

6 years ago
Blocks: 732451
(Assignee)

Updated

6 years ago
Blocks: 732452

Comment 22

6 years ago
Created attachment 602460 [details] [diff] [review]
paul pass

update of common.css and fixed some XUL stuff. Comes on top of darkdebug WIP 3.1
(Assignee)

Comment 23

5 years ago
Created attachment 603472 [details] [diff] [review]
windows attempt

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.
(Assignee)

Comment 24

5 years ago
Created attachment 608287 [details] [diff] [review]
windows attempt (rebased)
(Assignee)

Updated

5 years ago
Attachment #603472 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #608287 - Attachment description: rebased → windows attempt (rebased)
(Assignee)

Comment 25

5 years ago
Created attachment 608288 [details] [diff] [review]
paul-pass (rebased)
(Assignee)

Updated

5 years ago
Attachment #602460 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
Created attachment 608289 [details] [diff] [review]
darkdebug (updated)

rebased
(Assignee)

Updated

5 years ago
Attachment #602358 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 608290 [details] [diff] [review]
common (rebased)

rebased
(Assignee)

Updated

5 years ago
Attachment #602355 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 608330 [details] [diff] [review]
updated pinstripe
(Assignee)

Comment 29

5 years ago
Created attachment 611017 [details] [diff] [review]
dark-debug (folded and rebased)
Attachment #608288 - Attachment is obsolete: true
Attachment #608289 - Attachment is obsolete: true
Attachment #608330 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
mac screenshot: http://dl.dropbox.com/u/9927208/Screen%20Shot%202012-03-30%20at%2017.18.49.png

Comment 31

5 years ago
(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.
(Assignee)

Comment 32

5 years ago
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!
(Assignee)

Comment 33

5 years ago
Created attachment 611570 [details] [diff] [review]
dark-debug (updated pinstripe)

need to update gnomestripe and windows, but this should address paul's feedback.
Attachment #611017 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
Created attachment 611575 [details]
screenshot os x

showing checked pause button, removed status label, proper toolbar button margins.
(Assignee)

Comment 35

5 years ago
Created attachment 611606 [details] [diff] [review]
windows (updated winstripe)

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
(Assignee)

Comment 36

5 years ago
Created attachment 612182 [details]
linux screenshot (ubuntu 10.04)
(Assignee)

Comment 37

5 years ago
Created attachment 612219 [details] [diff] [review]
Add a pretty resizer and the dark theme to the debugger; Part 3 -windows
(Assignee)

Updated

5 years ago
Attachment #611606 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
Created attachment 612223 [details] [diff] [review]
Add a pretty resizer and the dark theme to the debugger, part 2 -darkdebug
(Assignee)

Updated

5 years ago
Attachment #611570 - Attachment is obsolete: true
(Assignee)

Comment 39

5 years ago
Created attachment 612224 [details] [diff] [review]
Add a pretty resizer and the dark theme to the debugger, part 1 -common.css
(Assignee)

Updated

5 years ago
Attachment #608290 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #612224 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Attachment #612223 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Attachment #612219 - Flags: review?(paul)

Comment 40

5 years ago
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)

Comment 41

5 years ago
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.

Comment 42

5 years ago
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);
(Assignee)

Comment 43

5 years ago
(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.

Comment 44

5 years ago
(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;

Comment 45

5 years ago
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.

Updated

5 years ago
Attachment #612219 - Flags: review?(paul)

Updated

5 years ago
Attachment #612223 - Flags: review?(paul)

Updated

5 years ago
Attachment #612224 - Flags: review?(paul)
(Assignee)

Comment 47

5 years ago
(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.
(Assignee)

Comment 48

5 years ago
(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.
(Assignee)

Comment 49

5 years ago
Created attachment 615481 [details] [diff] [review]
dark debug v1.5

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)
(Assignee)

Comment 50

5 years ago
Created attachment 615482 [details] [diff] [review]
dark debug v1.5.1

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)
(Assignee)

Comment 52

5 years ago
(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.
(Assignee)

Comment 53

5 years ago
(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.
(Assignee)

Comment 55

5 years ago
Created attachment 616546 [details] [diff] [review]
dark debug v1.5.2 (rebased)
Attachment #615482 - Attachment is obsolete: true
Attachment #615482 - Flags: review?(jwalker)
(Assignee)

Comment 56

5 years ago
Created attachment 616706 [details] [diff] [review]
dark debug v1.6
Attachment #616546 - Attachment is obsolete: true
(Assignee)

Comment 57

5 years ago
Created attachment 616709 [details]
Windows Screenshot
(Assignee)

Comment 58

5 years ago
Created attachment 616937 [details] [diff] [review]
dark debug v1.6.1

restored the mac splitter code.
Attachment #616706 - Attachment is obsolete: true
Attachment #616937 - Flags: review?(paul)
(Assignee)

Comment 59

5 years ago
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)
(Assignee)

Comment 60

5 years ago
Created attachment 617533 [details] [diff] [review]
dark debug v1.6.2

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)
(Assignee)

Updated

5 years ago
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?
Blocks: 748749
(Assignee)

Comment 62

5 years ago
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)

Updated

5 years ago
Depends on: 749626

Updated

5 years ago
Depends on: 751164

Comment 63

5 years ago
Created attachment 621556 [details] [diff] [review]
slim patch v0.1

attempt to reduce the current work to the strictly necessary. Not tested on Windows (but should be ok) and resizer not styled yet.

Comment 64

5 years ago
Created attachment 621558 [details] [diff] [review]
slim patch v0.2

resizer Mac and Linux

Updated

5 years ago
Attachment #621556 - Attachment is obsolete: true

Comment 65

5 years ago
Created attachment 621562 [details] [diff] [review]
slim patch v0.999

Windows style

Updated

5 years ago
Attachment #621558 - Attachment is obsolete: true

Comment 66

5 years ago
Created attachment 621599 [details] [diff] [review]
slim patch v1

Updated

5 years ago
Attachment #621562 - Attachment is obsolete: true

Comment 67

5 years ago
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)
(Assignee)

Updated

5 years ago
Alias: darkdebug
(Assignee)

Updated

5 years ago
Attachment #617533 - Attachment is obsolete: true
(Assignee)

Comment 68

5 years ago
Comment on attachment 621599 [details] [diff] [review]
slim patch v1

good good.
Attachment #621599 - Flags: review?(rcampbell) → review+

Comment 69

5 years ago
Created attachment 622365 [details] [diff] [review]
patch v1.1

rebased

Updated

5 years ago
Attachment #621599 - Attachment is obsolete: true
(Assignee)

Comment 70

5 years ago
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?
(Assignee)

Comment 71

5 years ago
(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.

Comment 72

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/32a259ecff5e
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 73

5 years ago
https://hg.mozilla.org/mozilla-central/rev/32a259ecff5e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
You need to log in before you can comment on or make changes to this bug.