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]
:
: Jason Laster [:jlast]
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]
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]
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 User image Panos Astithas [:past] 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 User image Panos Astithas [:past] 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 User image 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 User image Rob Campbell [:rc] (:robcee) 2012-02-16 13:08:48 PST
Created attachment 597976 [details]
Debugger Icons
Comment 4 User image 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 User image Panos Astithas [:past] 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 User image 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 User image Panos Astithas [:past] 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 User image 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 User image 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 User image Panos Astithas [:past] 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 User image Rob Campbell [:rc] (:robcee) 2012-04-25 12:11:40 PDT
Created attachment 618381 [details] [diff] [review]
icons, dehtmlification, splitters
Comment 12 User image Rob Campbell [:rc] (:robcee) 2012-04-25 12:13:05 PDT
*** Bug 748749 has been marked as a duplicate of this bug. ***
Comment 13 User image 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 User image 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 User image Rob Campbell [:rc] (:robcee) 2012-05-11 13:50:30 PDT
Created attachment 623287 [details]
OS X Screenshot
Comment 16 User image Rob Campbell [:rc] (:robcee) 2012-05-11 13:51:01 PDT
Created attachment 623288 [details]
Windows 7 Screenshot
Comment 17 User image 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 User image Paul Rouget [:paul] 2012-05-22 10:26:15 PDT
For the record, debugger mockup: http://cl.ly/GoMQ
Comment 19 User image 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 User image Paul Rouget [:paul] 2012-05-23 08:32:04 PDT
Created attachment 626461 [details]
screenshot - linux
Comment 21 User image 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 User image Paul Rouget [:paul] 2012-05-23 08:37:28 PDT
Created attachment 626463 [details] [diff] [review]
patch v2.1

RTL proof
Comment 23 User image 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 User image Paul Rouget [:paul] 2012-05-23 08:41:26 PDT
erf, wrong radius for pinstripe.
Comment 25 User image 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 User image 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 User image 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 User image 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 User image 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 User image Paul Rouget [:paul] 2012-05-23 16:42:28 PDT
Stephen, can we get a blue version of the pause icon? please :)
Comment 31 User image 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 User image 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 User image 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 User image 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 User image Paul Rouget [:paul] 2012-05-24 07:01:07 PDT
Created attachment 626786 [details] [diff] [review]
patch v2.3

rebased
Comment 36 User image Panos Astithas [:past] 2012-05-24 10:01:09 PDT
https://hg.mozilla.org/integration/fx-team/rev/13aa125cc8d2
Comment 38 User image Panos Astithas [:past] 2012-05-24 23:50:08 PDT
Updated the failing test and relanded:
https://hg.mozilla.org/integration/fx-team/rev/75eaccac4579
Comment 39 User image Rob Campbell [:rc] (:robcee) 2012-05-26 10:26:23 PDT
https://hg.mozilla.org/mozilla-central/rev/75eaccac4579
Comment 40 User image 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 User image Panos Astithas [:past] 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 User image 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 User image Panos Astithas [:past] 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 User image Panos Astithas [:past] 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.