Prettify the Stackframes UI

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Debugger
P3
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 12 obsolete attachments)

94.30 KB, patch
Details | Diff | Splinter Review
22.91 KB, patch
Details | Diff | Splinter Review
103.06 KB, patch
vporof
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Right now it looks like a plain list, a bit too out of place wrt the rest of the frontend. Something along the lines of the "Call Stack" in bug 753301 should work wonderful.
(Assignee)

Comment 1

5 years ago
Note: this bug should also handle the stackframes-lines-are-too-long issue when the script label takes too much space.
we didn't get to this in our triage today. P2 or 3? I take it you're working on this.
(Assignee)

Comment 3

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
> we didn't get to this in our triage today. P2 or 3? I take it you're working
> on this.

Well, not yet. I'd say P3 since it's just an enhancement.
Severity: normal → enhancement
Priority: -- → P3
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
I'm very tempted to display the stack frames as breadcrumbs, just like the inspector does with nodes. Thoughts?
+1

It will also leave space for the scripts list if we want to put it in a pane later on.

Comment 6

5 years ago
I don't know how important this information is, but just in case: breadcrumbs are great, but scrolling is not trivial, and variable size is not really possible (see how all the nodes have the same width in the inspector breadcrumbs?). But this can be fixed (bug in the scrollbox code).
(Assignee)

Comment 7

5 years ago
(In reply to Paul Rouget [:paul] from comment #6)
> I don't know how important this information is, but just in case:
> breadcrumbs are great, but scrolling is not trivial, and variable size is
> not really possible (see how all the nodes have the same width in the
> inspector breadcrumbs?). But this can be fixed (bug in the scrollbox code).

Thanks Paul, I saw that when browsing the Breadcrumbs.jsm. It's also unfortunately impossible to reuse that code, since it's very tied up to HTML logic. But:

* I'm planning to fix that. BreadcrumbsScrollbox.jsm in /common, initially for the debugger, hopefully inspector will use it as well.
* Surely variable crumb size is possible with a clever(er) use of the current background images, right? Just make sure a center region repeats itself horizontally.
* Scrolling is not *that* hard. I may not use a scrollbox.
I have my doubts that breadcrumbs would work for stack frames, but I'm curious to see what you can come up with.
(Assignee)

Comment 9

5 years ago
Created attachment 690935 [details]
before
(Assignee)

Comment 10

5 years ago
Created attachment 690936 [details]
after
(Assignee)

Updated

5 years ago
Attachment #690935 - Attachment is patch: false
Attachment #690935 - Attachment mime type: text/plain → image/png
(Assignee)

Comment 11

5 years ago
(In reply to Panos Astithas [:past] from comment #8)
> I have my doubts that breadcrumbs would work for stack frames, but I'm
> curious to see what you can come up with.

attachment 690936 [details] is a very crude representation of what I'm trying to achieve.
(Assignee)

Comment 12

5 years ago
Note that "Filter scripts" will be changed in bug 809348 to something more generic.
It's pretty. I still find the density of information in the current UI better though, especially since the call stack is sufficiently separated from the location information, which although very important, is not the first thing I'm looking for as I try to get a handle on the program's state.

Perhaps you could emphasize the function names more, or put the location below the name, à la breakpoints view. Or add a drop down arrow to get the traditional stack list. I'm also concerned that I almost never find myself using the breadcrumbs in the markup panel, even though I still find them sexy.

Funny detail: if we add the breadcrumbs, then the Step Out icon would look more intuitive facing backwards.
(Assignee)

Comment 14

5 years ago
Created attachment 692603 [details] [diff] [review]
v1

WIP. Works. Tests probably fail.
Attachment #690935 - Attachment is obsolete: true
Attachment #690936 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 692604 [details]
looks like this
In the mockup, the top of the stack was on the rightmost breadcrumb, which makes sense because it mimics the traditional pattern: foo(bar(baz())) would look like foo > bar > baz. Is that still the case? I can't tell from the screenshot.
(Assignee)

Comment 17

5 years ago
(In reply to Panos Astithas [:past] from comment #16)
> In the mockup, the top of the stack was on the rightmost breadcrumb, which
> makes sense because it mimics the traditional pattern: foo(bar(baz())) would
> look like foo > bar > baz. Is that still the case? I can't tell from the
> screenshot.

You're right. I'll change it.

Some stuff is still missing from the patch: 'list the stack' button, context menus, keyboard navigation etc.
(Assignee)

Comment 18

5 years ago
Also: that sources menulist takes up a lot of space. When this patch is finished, It would make sense to land it together with bug 812083.
(Assignee)

Comment 19

5 years ago
Created attachment 692620 [details] [diff] [review]
v2

Fixed reversed stacks.
Attachment #692603 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 692713 [details]
'list the stack' context menu

A context menu seems to work great in this situation.
(Assignee)

Comment 21

5 years ago
Created attachment 692762 [details] [diff] [review]
v3

Going through try.
Attachment #692620 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Comment on attachment 692762 [details] [diff] [review]
v3

Had to make a few small MenuContainer interface changes, to loosen the burden on inheriting objects. This made the implementation of a BreadcrumbsWidget much more concise, and considerable deletions across the debugger-view.js
Attachment #692762 - Flags: review?(rcampbell)
(Assignee)

Comment 24

5 years ago
Comment on attachment 692762 [details] [diff] [review]
v3

Paul, please let me know if there's anything in BreadcrumbsWidget.jsm that may not not be compatible with (or lacking) what the inspector needs.
Attachment #692762 - Flags: feedback?(paul)

Comment 25

5 years ago
Comment on attachment 692762 [details] [diff] [review]
v3

That looks right to me.
Attachment #692762 - Flags: feedback?(paul) → feedback+
(Assignee)

Comment 26

5 years ago
(In reply to Paul Rouget [:paul] from comment #25)
> Comment on attachment 692762 [details] [diff] [review]
> v3
> 
> That looks right to me.

Thanks!
Filed bug 822388.
(Assignee)

Updated

5 years ago
Blocks: 762160
(Assignee)

Comment 27

5 years ago
Created attachment 695318 [details] [diff] [review]
v4, part1 - breadcrumbs implementation

Rebased.
Attachment #692604 - Attachment is obsolete: true
Attachment #692713 - Attachment is obsolete: true
Attachment #692762 - Attachment is obsolete: true
Attachment #695318 - Flags: review?(rcampbell)
Attachment #692762 - Flags: review?(rcampbell)
(Assignee)

Comment 28

5 years ago
Created attachment 695319 [details] [diff] [review]
v4, part2 - move the StackFrames constructor in debugger-toolbar
Attachment #695319 - Flags: review?(rcampbell)
getting some noticeable flashing in the stack breadcrumbs on first use. Seems to disappear after I've used them a bit.
(Assignee)

Comment 30

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #29)
> getting some noticeable flashing in the stack breadcrumbs on first use.
> Seems to disappear after I've used them a bit.

Unfortunately, the same thing happens with the breadcrumbs in the inspector (this bug does not change that implementation, there's bug 822388 for that).

The flashing is due to the fact that there are separate images used for all (!) the different states of a breadcrumb (normal, pressed, selected, and also leftmost, middle, rightmost etc.). Once an image is loaded, the flashing disappears. My understanding is that we can't switch to using a single sprite containing all the images, and play with background coords, because this is not usable with using -moz-border-image (needed for variable width backgrounds).

A simple fix would be to make sure all the images are loaded somehow, but I can't think of any not-extremely-hacky way of achieving this. Paul, do you have any suggestions?
Flags: needinfo?(paul)
yuck. Yeah.

One alternative would be to draw the breadcrumbs chevrons using SVG. That would be a completely different implementation though.

If I ignore the flashing, the stackframes UI looks pretty good, if a little strange to my vertically-laid-out stack reading brain.

Can you setup a try build so we can ask others for feedback?
(Assignee)

Comment 32

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #31
> 
> If I ignore the flashing, the stackframes UI looks pretty good, if a little
> strange to my vertically-laid-out stack reading brain.
> 

Right click on any breadcrumb gives you what you'd expect.

> Can you setup a try build so we can ask others for feedback?

Ok.
(In reply to Victor Porof [:vp] from comment #32)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #31
> > 
> > If I ignore the flashing, the stackframes UI looks pretty good, if a little
> > strange to my vertically-laid-out stack reading brain.
> > 
> 
> Right click on any breadcrumb gives you what you'd expect.

Would it be too much to add a separate arrow button next to the breadcrumbs, like we do for the tab list?
(Assignee)

Comment 34

5 years ago
(In reply to Panos Astithas [:past] from comment #33)
> (In reply to Victor Porof [:vp] from comment #32)
> > (In reply to Rob Campbell [:rc] (:robcee) from comment #31
> > > 
> > > If I ignore the flashing, the stackframes UI looks pretty good, if a little
> > > strange to my vertically-laid-out stack reading brain.
> > > 
> > 
> > Right click on any breadcrumb gives you what you'd expect.
> 
> Would it be too much to add a separate arrow button next to the breadcrumbs,
> like we do for the tab list?

Nope, I think I can even reuse the same image.

Comment 35

5 years ago
(In reply to Victor Porof [:vp] from comment #30)
> A simple fix would be to make sure all the images are loaded somehow, but I
> can't think of any not-extremely-hacky way of achieving this. Paul, do you
> have any suggestions?

http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser.css#2341
Flags: needinfo?(paul)
(Assignee)

Comment 36

5 years ago
(In reply to Paul Rouget [:paul] from comment #35)
> (In reply to Victor Porof [:vp] from comment #30)
> > A simple fix would be to make sure all the images are loaded somehow, but I
> > can't think of any not-extremely-hacky way of achieving this. Paul, do you
> > have any suggestions?
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/
> browser.css#2341

ooooh
*applause* ^_^
(Assignee)

Comment 38

5 years ago
(In reply to Panos Astithas [:past] from comment #33)
> > > 
> > 
> > Right click on any breadcrumb gives you what you'd expect.
> 
> Would it be too much to add a separate arrow button next to the breadcrumbs,
> like we do for the tab list?

I couldn't find anything in browser/themes that's consistent across all platforms and suggestive for showing all the frames. The tab list icon on OS X works perfectly, as you anticipated, but is completely different on Windows and Linux (4 squares instead of a downward arrow).

Can we leave this additional button for a followup, and maybe we can get something neat from shorlander in the meantime?
(Assignee)

Comment 39

5 years ago
Created attachment 704996 [details] [diff] [review]
v5, part1 - breadcrumbs implementation

Rebased, added preload hack and fixed ux nits.
Attachment #695318 - Attachment is obsolete: true
Attachment #695319 - Attachment is obsolete: true
Attachment #695318 - Flags: review?(rcampbell)
Attachment #695319 - Flags: review?(rcampbell)
Attachment #704996 - Flags: review?(rcampbell)
(Assignee)

Comment 40

5 years ago
Created attachment 704998 [details] [diff] [review]
v5, part2 - move the StackFrames constructor in debugger-toolbar
Attachment #704998 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Attachment #704998 - Attachment description: v4, part2 - move the StackFrames constructor in debugger-toolbar → v5, part2 - move the StackFrames constructor in debugger-toolbar
Comment on attachment 704996 [details] [diff] [review]
v5, part1 - breadcrumbs implementation

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

this'll need a rebasin'.
Attachment #704996 - Flags: review?(rcampbell) → review+
(Assignee)

Updated

5 years ago
Blocks: 822388
(Assignee)

Comment 42

5 years ago
Created attachment 712108 [details] [diff] [review]
v5, part1 - breadcrumbs implementation (rebased)

that's a paddlin'
Attachment #712108 - Flags: review+
Attachment #704998 - Flags: review?(rcampbell) → review+
looks like part 2 needs a rebasin' too. Think I got this one.
I find the ordering of the stack frames a little weird. I kind of expect the "current frame" or "last frame" in the stack to be on the right. With it on the left, it's like I'm at the bottom of the stack.

I think it's evident what's going on when you look at these, but feels a bit backwards.

Comment 45

5 years ago
I would expect the newest stack frame to be on the right too...
(Assignee)

Comment 46

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #43)
> looks like part 2 needs a rebasin' too. Think I got this one.

I already rebased it. I'll upload it soon.
(Assignee)

Comment 47

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #44)
(In reply to Dave Camp (:dcamp) from comment #45)

The current/newest stackframe *is* on the right and the stackframes are scrolled to always keep the focused frame into view.

Maybe my rebasing didn't work properly or I did something wrong. I'll check it.
(Assignee)

Comment 48

5 years ago
Created attachment 713379 [details] [diff] [review]
v5, combined, rebased

Rebased (again) and combined the patches. They were getting pretty hard to maintain separately.
Attachment #712108 - Attachment is obsolete: true
Attachment #713379 - Flags: review+
(Assignee)

Comment 49

5 years ago
Created attachment 717809 [details] [diff] [review]
v5.1, combined, rebased
Attachment #713379 - Attachment is obsolete: true
Attachment #717809 - Flags: review+
Blocks: 812083
(Assignee)

Comment 50

5 years ago
Created attachment 721677 [details] [diff] [review]
v5.2, combined, rebased

Rebased...
Attachment #717809 - Attachment is obsolete: true
Attachment #721677 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dc25ef7de1f2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Depends on: 850558
You need to log in before you can comment on or make changes to this bug.