The default bug view has changed. See this FAQ.

Replace text with icons in the debugger toolbar

RESOLVED FIXED in Firefox 15

Status

()

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

People

(Reporter: past, Assigned: paul)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(7 attachments, 10 obsolete attachments)

14.74 KB, image/png
Details
28.47 KB, image/png
Details
74.67 KB, image/png
Details
23.52 KB, image/png
Details
277 bytes, image/png
Details
20.47 KB, image/png
Details
32.37 KB, patch
Details | Diff | Splinter Review
The debugger toolbar currently uses text in the various buttons. Using icons would make for a more compact and good-looking toolbar.
The dark theme should arguably land first, so that we can use dark icons as all the other tools do.
Depends on: 692409
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.
Created attachment 597976 [details]
Debugger Icons
What kind of timeline do we have for this and the visual updates for the debugger?
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.
yeah, what Panos says. We want to start with some of the UI refresh ASAP though. As always, sooner is better.
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.
UI tweaks: switch icon when paused/running, make it visually togglable and get the filter box looking like
(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.
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.
Created attachment 618381 [details] [diff] [review]
icons, dehtmlification, splitters
Assignee: nobody → rcampbell
Attachment #617306 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Duplicate of this bug: 748749
Blocks: 753301
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.
Attachment #618381 - Attachment is obsolete: true
Attachment #623156 - Flags: feedback?(paul)
Created attachment 623285 [details] [diff] [review]
icons v1.2

still missing linux. don't hate me.
Attachment #623156 - Attachment is obsolete: true
Attachment #623156 - Flags: feedback?(paul)
Attachment #623285 - Flags: feedback?(paul)
Created attachment 623287 [details]
OS X Screenshot
Created attachment 623288 [details]
Windows 7 Screenshot
(Assignee)

Comment 17

5 years ago
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-" ?
Attachment #623285 - Flags: feedback?(paul)
Priority: P3 → P2
(Assignee)

Comment 18

5 years ago
For the record, debugger mockup: http://cl.ly/GoMQ
Assignee: rcampbell → paul
(Assignee)

Comment 19

5 years ago
Created attachment 626459 [details] [diff] [review]
patch v2

Rob, can you try that on Windows and Mac?
(Assignee)

Updated

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

Comment 20

5 years ago
Created attachment 626461 [details]
screenshot - linux
Attachment #623287 - Attachment is obsolete: true
Attachment #623288 - Attachment is obsolete: true
(Assignee)

Comment 21

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

Comment 22

5 years ago
Created attachment 626463 [details] [diff] [review]
patch v2.1

RTL proof
(Assignee)

Updated

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

Comment 23

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

Comment 24

5 years ago
erf, wrong radius for pinstripe.
(Assignee)

Comment 25

5 years ago
Created attachment 626471 [details] [diff] [review]
patch v2.2

correct radius for pinstripe
(Assignee)

Updated

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

Updated

5 years ago
Attachment #626471 - Flags: review?(rcampbell)
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.
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.
(Assignee)

Comment 28

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

Comment 29

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

Comment 30

5 years ago
Stephen, can we get a blue version of the pause icon? please :)
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 :)
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 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+.
Attachment #626471 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 34

5 years ago
Created attachment 626784 [details] [diff] [review]
patch v2.3

Windows fixed. Better outline. Blue pause button.
(Assignee)

Updated

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

Comment 35

5 years ago
Created attachment 626786 [details] [diff] [review]
patch v2.3

rebased
(Assignee)

Updated

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

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/13aa125cc8d2
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
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
Whiteboard: [fixed-in-fx-team]
Updated the failing test and relanded:
https://hg.mozilla.org/integration/fx-team/rev/75eaccac4579
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/75eaccac4579
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15

Comment 40

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

5 years ago
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.
(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?
Blocks: 771464
Filed bug 771464 for the duplicate step-over rules.
You need to log in before you can comment on or make changes to this bug.