723059 - Replace text with icons in the debugger toolbar

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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 (he/him) [:past] (please ni?)]

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)]

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)]

Attachment: Debugger Icons

Comment 4

[Stephen Horlander [:shorlander]]

What kind of timeline do we have for this and the visual updates for the debugger?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

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)]

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 (he/him) [:past] (please ni?)]

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)]

UI tweaks: switch icon when paused/running, make it visually togglable and get the filter box looking like

Comment 9

[Rob Campbell [:rc] (:robcee)]

(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 (he/him) [:past] (please ni?)]

Attachment: 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)]

Attachment: icons, dehtmlification, splitters

Comment 13

[Rob Campbell [:rc] (:robcee)]

Attachment: 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)]

Attachment: icons v1.2

still missing linux. don't hate me.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Attachment: OS X Screenshot

Comment 16

[Rob Campbell [:rc] (:robcee)]

Attachment: Windows 7 Screenshot

Comment 17

[Paul Rouget [:paul]]

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]]

For the record, debugger mockup: http://cl.ly/GoMQ

Comment 19

[Paul Rouget [:paul]]

Attachment: patch v2

Rob, can you try that on Windows and Mac?

Comment 20

[Paul Rouget [:paul]]

Attachment: screenshot - linux

Comment 21

[Paul Rouget [:paul]]

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]]

Attachment: patch v2.1

RTL proof

Comment 23

[Paul Rouget [:paul]]

(I didn't include the splitter work, this will happen in another bug)

Comment 24

[Paul Rouget [:paul]]

erf, wrong radius for pinstripe.

Comment 25

[Paul Rouget [:paul]]

Attachment: patch v2.2

correct radius for pinstripe

Comment 26

[Rob Campbell [:rc] (:robcee)]

Attachment: 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)]

Attachment: 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]]

(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]]

(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]]

Stephen, can we get a blue version of the pause icon? please :)

Comment 31

[Stephen Horlander [:shorlander]]

Attachment: 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)]

Attachment: 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)]

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]]

Attachment: patch v2.3

Windows fixed. Better outline. Blue pause button.

Comment 35

[Paul Rouget [:paul]]

Attachment: patch v2.3

rebased

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/13aa125cc8d2

Comment 37

[Ed Morley [:emorley]]

Sorry, had to back out for timeouts during browser_dbg_pause-resume.js:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=13aa125cc8d2

eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=12033453&tree=Fx-Team

https://hg.mozilla.org/integration/fx-team/rev/48b399b01efc

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

Updated the failing test and relanded:
https://hg.mozilla.org/integration/fx-team/rev/75eaccac4579

Comment 39

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/75eaccac4579

Comment 40

[David.Vincent]

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 (he/him) [:past] (please ni?)]

(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]

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 (he/him) [:past] (please ni?)]

(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 (he/him) [:past] (please ni?)]

Filed bug 771464 for the duplicate step-over rules.