Last Comment Bug 723059 - Replace text with icons in the debugger toolbar
: Replace text with icons in the debugger toolbar
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: Paul Rouget [:paul]
:
Mentors:
: 748749 (view as bug list)
Depends on: darkdebug
Blocks: minotaur 753301 771464
  Show dependency treegraph
 
Reported: 2012-02-01 04:02 PST by Panos Astithas [:past] (away until 7/21)
Modified: 2012-07-06 04:40 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Debugger Icons (14.74 KB, image/png)
2012-02-16 13:08 PST, Rob Campbell [:rc] (:robcee)
no flags Details
Working patch (16.44 KB, patch)
2012-04-22 04:59 PDT, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
icons, dehtmlification, splitters (16.89 KB, patch)
2012-04-25 12:11 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
icons v1 (15.20 KB, patch)
2012-05-11 08:40 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
icons v1.2 (23.20 KB, patch)
2012-05-11 13:49 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
OS X Screenshot (120.68 KB, image/png)
2012-05-11 13:50 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
Windows 7 Screenshot (19.72 KB, image/png)
2012-05-11 13:51 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
patch v2 (27.47 KB, patch)
2012-05-23 08:29 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
screenshot - linux (28.47 KB, image/png)
2012-05-23 08:32 PDT, Paul Rouget [:paul]
no flags Details
patch v2.1 (27.56 KB, patch)
2012-05-23 08:37 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.2 (29.36 KB, patch)
2012-05-23 08:53 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
screenshot - os x (74.67 KB, image/png)
2012-05-23 15:44 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
screenshot - os x - inspector toolbar (23.52 KB, image/png)
2012-05-23 15:46 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
Pause Icon (Active) (277 bytes, image/png)
2012-05-23 17:38 PDT, Stephen Horlander [:shorlander]
no flags Details
screenshot - windows + inspector (20.47 KB, image/png)
2012-05-24 05:40 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
patch v2.3 (32.39 KB, patch)
2012-05-24 06:54 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.3 (32.37 KB, patch)
2012-05-24 07:01 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Panos Astithas [:past] (away until 7/21) 2012-02-01 04:02:24 PST
The debugger toolbar currently uses text in the various buttons. Using icons would make for a more compact and good-looking toolbar.
Comment 1 Panos Astithas [:past] (away until 7/21) 2012-02-01 11:58:22 PST
The dark theme should arguably land first, so that we can use dark icons as all the other tools do.
Comment 2 Rob Campbell [:rc] (:robcee) 2012-02-16 13:08:06 PST
We should also get some ideas in here for shorlander to work with.

The standard Step In, Step Over and Step Out icons from other debuggers (see screenshot).

I think the icons similar to Firebug and Chrome are what we're looking for.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-02-16 13:08:48 PST
Created attachment 597976 [details]
Debugger Icons
Comment 4 Stephen Horlander [:shorlander] 2012-02-22 13:46:44 PST
What kind of timeline do we have for this and the visual updates for the debugger?
Comment 5 Panos Astithas [:past] (away until 7/21) 2012-02-23 03:19:07 PST
Rob and Dave would know more, but I think we are hoping to have the debugger preffed on sometime in the Firefox 14 timeframe. That said, we had prioritized the visual update work as something we would do ASAP.
Comment 6 Rob Campbell [:rc] (:robcee) 2012-02-23 10:44:12 PST
yeah, what Panos says. We want to start with some of the UI refresh ASAP though. As always, sooner is better.
Comment 7 Panos Astithas [:past] (away until 7/21) 2012-03-18 02:40:15 PDT
I should also note that we need slick icons for the editor as well (breakpoints and paused line indicator). Current one (the other is only in fx-team at the moment):

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/orion/orion.css#79

Not sure if you'd like a separate bug for these though.
Comment 8 Rob Campbell [:rc] (:robcee) 2012-03-29 11:20:36 PDT
UI tweaks: switch icon when paused/running, make it visually togglable and get the filter box looking like
Comment 9 Rob Campbell [:rc] (:robcee) 2012-03-29 11:21:03 PDT
(In reply to Panos Astithas [:past] from comment #7)
> I should also note that we need slick icons for the editor as well
> (breakpoints and paused line indicator). Current one (the other is only in
> fx-team at the moment):
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/
> orion/orion.css#79
> 
> Not sure if you'd like a separate bug for these though.

filed bug 740482.
Comment 10 Panos Astithas [:past] (away until 7/21) 2012-04-22 04:59:08 PDT
Created attachment 617306 [details] [diff] [review]
Working patch

This is a patch with all the related changes in remote-debug. It works in my Mac, but I haven't tested it in Windows and Linux.
Comment 11 Rob Campbell [:rc] (:robcee) 2012-04-25 12:11:40 PDT
Created attachment 618381 [details] [diff] [review]
icons, dehtmlification, splitters
Comment 12 Rob Campbell [:rc] (:robcee) 2012-04-25 12:13:05 PDT
*** Bug 748749 has been marked as a duplicate of this bug. ***
Comment 13 Rob Campbell [:rc] (:robcee) 2012-05-11 08:40:44 PDT
Created attachment 623156 [details] [diff] [review]
icons v1

first attempt. This does affect the inspector toolbar buttons so clearly not going to go, but would like some feedback to make this work.
Comment 14 Rob Campbell [:rc] (:robcee) 2012-05-11 13:49:40 PDT
Created attachment 623285 [details] [diff] [review]
icons v1.2

still missing linux. don't hate me.
Comment 15 Rob Campbell [:rc] (:robcee) 2012-05-11 13:50:30 PDT
Created attachment 623287 [details]
OS X Screenshot
Comment 16 Rob Campbell [:rc] (:robcee) 2012-05-11 13:51:01 PDT
Created attachment 623288 [details]
Windows 7 Screenshot
Comment 17 Paul Rouget [:paul] 2012-05-14 02:38:59 PDT
Comment on attachment 623285 [details] [diff] [review]
icons v1.2

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

Looks hot :D

::: browser/devtools/debugger/debugger-view.js
@@ +529,1 @@
>  

Can you do that in CSS (based on the `checked` attribute)?

::: browser/devtools/debugger/debugger.css
@@ +49,5 @@
>  
> +#dbg-content {
> +  padding: 0;
> +}
> +

Shouldn't it be in browser/themes/?

::: browser/devtools/debugger/debugger.xul
@@ +96,5 @@
> +        <toolbarbutton id="step-out"
> +                       class="devtools-toolbarbutton"
> +                       image="chrome://browser/skin/devtools/step-out.png"
> +                       tabindex="0"/>
> +      </hbox>

If you remove the labels, you need to add tooltips.

::: browser/themes/winstripe/devtools/common.css
@@ +80,1 @@
>    margin: 0;

You also need to remove a border (-moz-border-end-width:0) but not for the very last element.

Also, you'll probably need to move the box-shadow from the button to the buttonbox.

You can see the problem here: http://i.imgur.com/PPJuL.png

::: browser/themes/winstripe/jar.mn
@@ +376,1 @@
>  #ifdef MOZ_SERVICES_SYNC

Can you prefix the icons name with "debugger-" ?
Comment 18 Paul Rouget [:paul] 2012-05-22 10:26:15 PDT
For the record, debugger mockup: http://cl.ly/GoMQ
Comment 19 Paul Rouget [:paul] 2012-05-23 08:29:54 PDT
Created attachment 626459 [details] [diff] [review]
patch v2

Rob, can you try that on Windows and Mac?
Comment 20 Paul Rouget [:paul] 2012-05-23 08:32:04 PDT
Created attachment 626461 [details]
screenshot - linux
Comment 21 Paul Rouget [:paul] 2012-05-23 08:33:56 PDT
Comment on attachment 626459 [details] [diff] [review]
patch v2

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

::: browser/themes/gnomestripe/devtools/debugger.css
@@ +266,5 @@
> +  list-style-image: url("chrome://browser/skin/devtools/debugger-step-over.png");
> +}
> +
> +#debugger-controls > toolbarbutton {
> +  border-width: 0 1px 0 0;

Oops, this is not RTL proof.
Comment 22 Paul Rouget [:paul] 2012-05-23 08:37:28 PDT
Created attachment 626463 [details] [diff] [review]
patch v2.1

RTL proof
Comment 23 Paul Rouget [:paul] 2012-05-23 08:38:26 PDT
(I didn't include the splitter work, this will happen in another bug)
Comment 24 Paul Rouget [:paul] 2012-05-23 08:41:26 PDT
erf, wrong radius for pinstripe.
Comment 25 Paul Rouget [:paul] 2012-05-23 08:53:35 PDT
Created attachment 626471 [details] [diff] [review]
patch v2.2

correct radius for pinstripe
Comment 26 Rob Campbell [:rc] (:robcee) 2012-05-23 15:44:42 PDT
Created attachment 626607 [details]
screenshot - os x

screenshot. Looks good, though the checked state of the pause button does not have the usual checked text color. Button radii look good.

Removed the changes to add the side-splitters? Presumably to simplify this patch. I am ok with that.
Comment 27 Rob Campbell [:rc] (:robcee) 2012-05-23 15:46:06 PDT
Created attachment 626608 [details]
screenshot - os x - inspector toolbar

for reference, here's the inspector toolbar.

note the focus ring around the settings menu button. That's also present without this patch so we should probably file something for that to adjust the padding or to remove the focus ring.
Comment 28 Paul Rouget [:paul] 2012-05-23 16:40:03 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #26)
> Created attachment 626607 [details]
> screenshot - os x
> 
> screenshot. Looks good, though the checked state of the pause button does
> not have the usual checked text color.

We can fix that. Do you agree with having only a pause icon? (no play icon)

> Button radii look good.
> Removed the changes to add the side-splitters?

We will do that in a separate bug.
Comment 29 Paul Rouget [:paul] 2012-05-23 16:40:59 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #27)
> Created attachment 626608 [details]
> screenshot - os x - inspector toolbar
> 
> for reference, here's the inspector toolbar.
> 
> note the focus ring around the settings menu button. That's also present
> without this patch so we should probably file something for that to adjust
> the padding or to remove the focus ring.

I did put the focus ring in purpose. I didn't realize it looked like that on Mac though.
Comment 30 Paul Rouget [:paul] 2012-05-23 16:42:28 PDT
Stephen, can we get a blue version of the pause icon? please :)
Comment 31 Stephen Horlander [:shorlander] 2012-05-23 17:38:48 PDT
Created attachment 626655 [details]
Pause Icon (Active)

(In reply to Paul Rouget [:paul] from comment #30)
> Stephen, can we get a blue version of the pause icon? please :)
Comment 32 Rob Campbell [:rc] (:robcee) 2012-05-24 05:40:20 PDT
Created attachment 626763 [details]
screenshot - windows + inspector

here's the windows screenshot.

No icons, so I'm guessing there was some file-naming or jar mangling. I'll take a look at the patch.
Comment 33 Rob Campbell [:rc] (:robcee) 2012-05-24 05:47:36 PDT
Comment on attachment 626471 [details] [diff] [review]
patch v2.2

yeah, forgot to add skin/aero/browser/devtools/debugger-*.png in the jar file.

otherwise r+.
Comment 34 Paul Rouget [:paul] 2012-05-24 06:54:55 PDT
Created attachment 626784 [details] [diff] [review]
patch v2.3

Windows fixed. Better outline. Blue pause button.
Comment 35 Paul Rouget [:paul] 2012-05-24 07:01:07 PDT
Created attachment 626786 [details] [diff] [review]
patch v2.3

rebased
Comment 36 Panos Astithas [:past] (away until 7/21) 2012-05-24 10:01:09 PDT
https://hg.mozilla.org/integration/fx-team/rev/13aa125cc8d2
Comment 38 Panos Astithas [:past] (away until 7/21) 2012-05-24 23:50:08 PDT
Updated the failing test and relanded:
https://hg.mozilla.org/integration/fx-team/rev/75eaccac4579
Comment 39 Rob Campbell [:rc] (:robcee) 2012-05-26 10:26:23 PDT
https://hg.mozilla.org/mozilla-central/rev/75eaccac4579
Comment 40 David.Vincent 2012-05-31 10:06:26 PDT
There is an error for the pause button with Windows :

Code is :
#resume {
  -moz-image-region: rect(0px, 16px, 16px, 0px);
}

#resume[checked=true] {
  -moz-image-region: rect(0px, 32px, 16px, 16px);
  list-style-image: url("chrome://browser/skin/devtools/debugger-pause.png");
}

The code should be :
#resume {
  list-style-image: url("chrome://browser/skin/devtools/debugger-pause.png");
  -moz-image-region: rect(0px, 16px, 16px, 0px);
}

#resume[checked=true] {
  -moz-image-region: rect(0px, 32px, 16px, 16px);
}
Comment 41 Panos Astithas [:past] (away until 7/21) 2012-05-31 10:31:43 PDT
(In reply to David.Vincent from comment #40)
> There is an error for the pause button with Windows :
> 
> Code is :
> #resume {
>   -moz-image-region: rect(0px, 16px, 16px, 0px);
> }
> 
> #resume[checked=true] {
>   -moz-image-region: rect(0px, 32px, 16px, 16px);
>   list-style-image: url("chrome://browser/skin/devtools/debugger-pause.png");
> }
> 
> The code should be :
> #resume {
>   list-style-image: url("chrome://browser/skin/devtools/debugger-pause.png");
>   -moz-image-region: rect(0px, 16px, 16px, 0px);
> }
> 
> #resume[checked=true] {
>   -moz-image-region: rect(0px, 32px, 16px, 16px);
> }

Yes, there is a fix for that in bug 758683, but in that same bug we will be changing the icon anyway, so this will be resolved today, one way or another.
Comment 42 KLB 2012-07-04 09:16:21 PDT
The following CSS instruction is in the patch twice:

#step-over {
  list-style-image: url("chrome://browser/skin/devtools/debugger-step-over.png");
}

The first instance of this instruction could probably be removed.
Comment 43 Panos Astithas [:past] (away until 7/21) 2012-07-05 00:56:50 PDT
(In reply to KLB from comment #42)
> The following CSS instruction is in the patch twice:
> 
> #step-over {
>   list-style-image:
> url("chrome://browser/skin/devtools/debugger-step-over.png");
> }
> 
> The first instance of this instruction could probably be removed.

Can you please file a followup bug for this?
Comment 44 Panos Astithas [:past] (away until 7/21) 2012-07-06 04:40:09 PDT
Filed bug 771464 for the duplicate step-over rules.

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