Last Comment Bug 692409 - (darkdebug) Add a pretty resizer and the dark theme to the debugger
(darkdebug)
: Add a pretty resizer and the dark theme to the debugger
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 15
Assigned To: Rob Campbell [:rc] (:robcee)
:
: James Long (:jlongster)
Mentors:
Depends on: 749626 751164
Blocks: 708257 minotaur 723059 732451 732452 748749
  Show dependency treegraph
 
Reported: 2011-10-06 03:08 PDT by Panos Astithas [:past]
Modified: 2012-05-11 11:30 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
common.css (11.13 KB, patch)
2012-02-29 15:08 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
darkdebug WIP (6.63 KB, patch)
2012-02-29 15:09 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
darkdebug WIP alternate (6.66 KB, patch)
2012-02-29 15:21 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
screenshot, inverted UI (369.68 KB, image/png)
2012-02-29 15:21 PST, Rob Campbell [:rc] (:robcee)
no flags Details
darkdebug WIP 2 (6.70 KB, patch)
2012-03-01 07:36 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
darkdebug WIP 3 (11.56 KB, patch)
2012-03-01 14:03 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
common.css (11.15 KB, patch)
2012-03-02 07:19 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
darkdebug WIP 3.1 (11.54 KB, patch)
2012-03-02 07:26 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
paul pass (11.47 KB, patch)
2012-03-02 13:14 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
windows attempt (7.15 KB, patch)
2012-03-06 14:32 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
windows attempt (rebased) (7.22 KB, patch)
2012-03-22 03:47 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
paul-pass (rebased) (7.60 KB, patch)
2012-03-22 03:52 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
darkdebug (updated) (12.49 KB, patch)
2012-03-22 03:54 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
common (rebased) (11.23 KB, patch)
2012-03-22 03:54 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
updated pinstripe (3.27 KB, patch)
2012-03-22 07:36 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
dark-debug (folded and rebased) (19.21 KB, patch)
2012-03-30 13:51 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
dark-debug (updated pinstripe) (20.08 KB, patch)
2012-04-02 13:30 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
screenshot os x (220.99 KB, image/png)
2012-04-02 13:33 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
windows (updated winstripe) (8.78 KB, patch)
2012-04-02 15:02 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
linux screenshot (ubuntu 10.04) (81.82 KB, image/png)
2012-04-04 07:23 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
Add a pretty resizer and the dark theme to the debugger; Part 3 -windows (9.31 KB, patch)
2012-04-04 09:11 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Add a pretty resizer and the dark theme to the debugger, part 2 -darkdebug (19.20 KB, patch)
2012-04-04 09:13 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Add a pretty resizer and the dark theme to the debugger, part 1 -common.css (11.82 KB, patch)
2012-04-04 09:14 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
dark debug v1.5 (30.25 KB, patch)
2012-04-16 14:50 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
dark debug v1.5.1 (30.25 KB, patch)
2012-04-16 14:52 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
dark debug v1.5.2 (rebased) (28.98 KB, patch)
2012-04-19 06:42 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
dark debug v1.6 (29.09 KB, patch)
2012-04-19 12:50 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Windows Screenshot (34.24 KB, image/png)
2012-04-19 12:51 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
dark debug v1.6.1 (29.11 KB, patch)
2012-04-20 05:08 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
dark debug v1.6.2 (29.25 KB, patch)
2012-04-23 10:37 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
slim patch v0.1 (5.30 KB, patch)
2012-05-07 04:39 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
slim patch v0.2 (11.82 KB, patch)
2012-05-07 04:55 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
slim patch v0.999 (11.82 KB, patch)
2012-05-07 05:13 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
slim patch v1 (11.85 KB, patch)
2012-05-07 07:56 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
patch v1.1 (8.42 KB, patch)
2012-05-09 06:51 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-10-06 03:08:22 PDT
Instead of a splitter, we should be using the same pretty resizer as the highlighter toolbar, from bug 691712.
Comment 1 Victor Porof [:vporof][:vp] 2011-11-11 07:23:05 PST
On this note, maybe some elements from the debugger UI should also be resizable, like the stackframes list or property view.
Comment 2 Panos Astithas [:past] 2011-11-11 07:33:49 PST
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.
Comment 3 Panos Astithas [:past] 2011-11-21 06:48:50 PST
(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.
Comment 4 Panos Astithas [:past] 2011-12-19 01:40:15 PST
Bug 687702 adds a devtools/common.css stylesheet with the dark theme.
Comment 5 Paul Rouget [:paul] 2012-01-14 10:10:13 PST
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 :)
Comment 6 Panos Astithas [:past] 2012-01-14 12:09:09 PST
(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 Paul Rouget [:paul] 2012-01-14 12:16:06 PST
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.
Comment 8 Panos Astithas [:past] 2012-01-14 13:02:08 PST
(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!
Comment 9 Paul Rouget [:paul] 2012-02-28 00:41:08 PST
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.
Comment 10 Rob Campbell [:rc] (:robcee) 2012-02-29 15:08:51 PST
Created attachment 601783 [details] [diff] [review]
common.css

common.css from paul's webconsole refactoring.
Comment 11 Rob Campbell [:rc] (:robcee) 2012-02-29 15:09:55 PST
Created attachment 601785 [details] [diff] [review]
darkdebug WIP

todo: update winstripe and gnomestripe. Based on common.css.
Comment 12 Rob Campbell [:rc] (:robcee) 2012-02-29 15:21:23 PST
Created attachment 601790 [details] [diff] [review]
darkdebug WIP alternate

Inverted UI, toolbar on the bottom. See screen.
Comment 13 Rob Campbell [:rc] (:robcee) 2012-02-29 15:21:58 PST
Created attachment 601791 [details]
screenshot, inverted UI
Comment 14 Victor Porof [:vporof][:vp] 2012-02-29 15:29:42 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #13)
> Created attachment 601791 [details]
> screenshot, inverted UI

http://bit.ly/ycIu92
Comment 15 Rob Campbell [:rc] (:robcee) 2012-03-01 06:38:41 PST
(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.
Comment 16 Victor Porof [:vporof][:vp] 2012-03-01 07:06:36 PST
(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.
Comment 17 Rob Campbell [:rc] (:robcee) 2012-03-01 07:36:23 PST
Created attachment 601967 [details] [diff] [review]
darkdebug WIP 2

toolbar back on top per señor horlander. Text color on label matches buttons now.
Comment 18 Panos Astithas [:past] 2012-03-01 11:08:44 PST
Love it! My eyes don't hurt any more.
Comment 19 Rob Campbell [:rc] (:robcee) 2012-03-01 14:03:53 PST
Created attachment 602103 [details] [diff] [review]
darkdebug WIP 3

version 3. copied theme over to winstripe and gnome.
Comment 20 Rob Campbell [:rc] (:robcee) 2012-03-02 07:19:33 PST
Created attachment 602355 [details] [diff] [review]
common.css

a slight rebase
Comment 21 Rob Campbell [:rc] (:robcee) 2012-03-02 07:26:10 PST
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.
Comment 22 Paul Rouget [:paul] 2012-03-02 13:14:32 PST
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
Comment 23 Rob Campbell [:rc] (:robcee) 2012-03-06 14:32:33 PST
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.
Comment 24 Rob Campbell [:rc] (:robcee) 2012-03-22 03:47:48 PDT
Created attachment 608287 [details] [diff] [review]
windows attempt (rebased)
Comment 25 Rob Campbell [:rc] (:robcee) 2012-03-22 03:52:37 PDT
Created attachment 608288 [details] [diff] [review]
paul-pass (rebased)
Comment 26 Rob Campbell [:rc] (:robcee) 2012-03-22 03:54:04 PDT
Created attachment 608289 [details] [diff] [review]
darkdebug (updated)

rebased
Comment 27 Rob Campbell [:rc] (:robcee) 2012-03-22 03:54:35 PDT
Created attachment 608290 [details] [diff] [review]
common (rebased)

rebased
Comment 28 Rob Campbell [:rc] (:robcee) 2012-03-22 07:36:23 PDT
Created attachment 608330 [details] [diff] [review]
updated pinstripe
Comment 29 Rob Campbell [:rc] (:robcee) 2012-03-30 13:51:28 PDT
Created attachment 611017 [details] [diff] [review]
dark-debug (folded and rebased)
Comment 30 Rob Campbell [:rc] (:robcee) 2012-03-30 13:53:23 PDT
mac screenshot: http://dl.dropbox.com/u/9927208/Screen%20Shot%202012-03-30%20at%2017.18.49.png
Comment 31 Paul Rouget [:paul] 2012-03-30 15:07:32 PDT
(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.
Comment 32 Rob Campbell [:rc] (:robcee) 2012-04-02 07:15:13 PDT
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!
Comment 33 Rob Campbell [:rc] (:robcee) 2012-04-02 13:30:01 PDT
Created attachment 611570 [details] [diff] [review]
dark-debug (updated pinstripe)

need to update gnomestripe and windows, but this should address paul's feedback.
Comment 34 Rob Campbell [:rc] (:robcee) 2012-04-02 13:33:17 PDT
Created attachment 611575 [details]
screenshot os x

showing checked pause button, removed status label, proper toolbar button margins.
Comment 35 Rob Campbell [:rc] (:robcee) 2012-04-02 15:02:32 PDT
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).
Comment 36 Rob Campbell [:rc] (:robcee) 2012-04-04 07:23:36 PDT
Created attachment 612182 [details]
linux screenshot (ubuntu 10.04)
Comment 37 Rob Campbell [:rc] (:robcee) 2012-04-04 09:11:48 PDT
Created attachment 612219 [details] [diff] [review]
Add a pretty resizer and the dark theme to the debugger; Part 3 -windows
Comment 38 Rob Campbell [:rc] (:robcee) 2012-04-04 09:13:32 PDT
Created attachment 612223 [details] [diff] [review]
Add a pretty resizer and the dark theme to the debugger, part 2 -darkdebug
Comment 39 Rob Campbell [:rc] (:robcee) 2012-04-04 09:14:11 PDT
Created attachment 612224 [details] [diff] [review]
Add a pretty resizer and the dark theme to the debugger, part 1 -common.css
Comment 40 Paul Rouget [:paul] 2012-04-05 07:28:48 PDT
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 Paul Rouget [:paul] 2012-04-05 08:43:51 PDT
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 Paul Rouget [:paul] 2012-04-05 09:31:13 PDT
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);
Comment 43 Rob Campbell [:rc] (:robcee) 2012-04-05 10:26:38 PDT
(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 Paul Rouget [:paul] 2012-04-05 10:50:53 PDT
(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 Paul Rouget [:paul] 2012-04-05 11:39:46 PDT
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.
Comment 46 Panos Astithas [:past] 2012-04-06 09:36:55 PDT
(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.
Comment 47 Rob Campbell [:rc] (:robcee) 2012-04-16 10:58:03 PDT
(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.
Comment 48 Rob Campbell [:rc] (:robcee) 2012-04-16 11:27:40 PDT
(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.
Comment 49 Rob Campbell [:rc] (:robcee) 2012-04-16 14:50:08 PDT
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!
Comment 50 Rob Campbell [:rc] (:robcee) 2012-04-16 14:52:54 PDT
Created attachment 615482 [details] [diff] [review]
dark debug v1.5.1

found a minor nit. setAttribute->removeAttribute in debugger-view.js.
Comment 51 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-04-16 15:24:21 PDT
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)
Comment 52 Rob Campbell [:rc] (:robcee) 2012-04-17 04:48:14 PDT
(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.
Comment 53 Rob Campbell [:rc] (:robcee) 2012-04-17 10:10:11 PDT
(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.
Comment 54 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-04-19 01:26:08 PDT
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.
Comment 55 Rob Campbell [:rc] (:robcee) 2012-04-19 06:42:12 PDT
Created attachment 616546 [details] [diff] [review]
dark debug v1.5.2 (rebased)
Comment 56 Rob Campbell [:rc] (:robcee) 2012-04-19 12:50:39 PDT
Created attachment 616706 [details] [diff] [review]
dark debug v1.6
Comment 57 Rob Campbell [:rc] (:robcee) 2012-04-19 12:51:19 PDT
Created attachment 616709 [details]
Windows Screenshot
Comment 58 Rob Campbell [:rc] (:robcee) 2012-04-20 05:08:14 PDT
Created attachment 616937 [details] [diff] [review]
dark debug v1.6.1

restored the mac splitter code.
Comment 59 Rob Campbell [:rc] (:robcee) 2012-04-20 06:03:01 PDT
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!
Comment 60 Rob Campbell [:rc] (:robcee) 2012-04-23 10:37:46 PDT
Created attachment 617533 [details] [diff] [review]
dark debug v1.6.2

updated, rebased on latest changes in fx-team.
Comment 61 Dão Gottwald [:dao] 2012-04-24 07:20:09 PDT
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 62 Rob Campbell [:rc] (:robcee) 2012-04-25 15:00:14 PDT
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.
Comment 63 Paul Rouget [:paul] 2012-05-07 04:39:50 PDT
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 Paul Rouget [:paul] 2012-05-07 04:55:39 PDT
Created attachment 621558 [details] [diff] [review]
slim patch v0.2

resizer Mac and Linux
Comment 65 Paul Rouget [:paul] 2012-05-07 05:13:04 PDT
Created attachment 621562 [details] [diff] [review]
slim patch v0.999

Windows style
Comment 66 Paul Rouget [:paul] 2012-05-07 07:56:03 PDT
Created attachment 621599 [details] [diff] [review]
slim patch v1
Comment 67 Paul Rouget [:paul] 2012-05-07 08:04:12 PDT
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.
Comment 68 Rob Campbell [:rc] (:robcee) 2012-05-08 06:42:57 PDT
Comment on attachment 621599 [details] [diff] [review]
slim patch v1

good good.
Comment 69 Paul Rouget [:paul] 2012-05-09 06:51:58 PDT
Created attachment 622365 [details] [diff] [review]
patch v1.1

rebased
Comment 70 Rob Campbell [:rc] (:robcee) 2012-05-10 11:34:37 PDT
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?
Comment 71 Rob Campbell [:rc] (:robcee) 2012-05-10 11:38:40 PDT
(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 73 Rob Campbell [:rc] (:robcee) 2012-05-11 11:30:22 PDT
https://hg.mozilla.org/mozilla-central/rev/32a259ecff5e

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