802546 - Prettify the Stackframes UI

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[Victor Porof [:vporof][:vp]]

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.

Comment 1

[Victor Porof [:vporof][:vp]]

Note: this bug should also handle the stackframes-lines-are-too-long issue when the script label takes too much space.

Comment 2

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

we didn't get to this in our triage today. P2 or 3? I take it you're working on this.

Comment 3

[Victor Porof [:vporof][:vp]]

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

Comment 4

[Victor Porof [:vporof][:vp]]

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

Comment 5

[Girish Sharma [:Optimizer]]

+1

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

Comment 6

[Paul Rouget [:paul]]

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

Comment 7

[Victor Porof [:vporof][:vp]]

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

Comment 8

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

I have my doubts that breadcrumbs would work for stack frames, but I'm curious to see what you can come up with.

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: before

Comment 10

[Victor Porof [:vporof][:vp]]

Attachment: after

Comment 11

[Victor Porof [:vporof][:vp]]

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

Comment 12

[Victor Porof [:vporof][:vp]]

Note that "Filter scripts" will be changed in bug 809348 to something more generic.

Comment 13

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

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.

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: v1

WIP. Works. Tests probably fail.

Comment 15

[Victor Porof [:vporof][:vp]]

Attachment: looks like this

Comment 16

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

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.

Comment 17

[Victor Porof [:vporof][:vp]]

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

Comment 18

[Victor Porof [:vporof][:vp]]

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.

Comment 19

[Victor Porof [:vporof][:vp]]

Attachment: v2

Fixed reversed stacks.

Comment 20

[Victor Porof [:vporof][:vp]]

Attachment: 'list the stack' context menu

A context menu seems to work great in this situation.

Comment 21

[Victor Porof [:vporof][:vp]]

Attachment: v3

Going through try.

Comment 22

[Victor Porof [:vporof][:vp]]

Green! https://tbpl.mozilla.org/?tree=Try&rev=b29a4886e819

You can play with the builds at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b29a4886e819/

Comment 23

[Victor Porof [:vporof][:vp]]

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

Comment 24

[Victor Porof [:vporof][:vp]]

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.

Comment 25

[Paul Rouget [:paul]]

Comment on attachment 692762 [details] [diff] [review]
v3

That looks right to me.

Comment 26

[Victor Porof [:vporof][:vp]]

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

Comment 27

[Victor Porof [:vporof][:vp]]

Attachment: v4, part1 - breadcrumbs implementation

Rebased.

Comment 28

[Victor Porof [:vporof][:vp]]

Attachment: v4, part2 - move the StackFrames constructor in debugger-toolbar

Comment 29

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

getting some noticeable flashing in the stack breadcrumbs on first use. Seems to disappear after I've used them a bit.

Comment 30

[Victor Porof [:vporof][:vp]]

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

Comment 31

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

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?

Comment 32

[Victor Porof [:vporof][:vp]]

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

Comment 33

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

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

Comment 34

[Victor Porof [:vporof][:vp]]

(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

[Paul Rouget [:paul]]

(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

Comment 36

[Victor Porof [:vporof][:vp]]

(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

Comment 37

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

*applause* ^_^

Comment 38

[Victor Porof [:vporof][:vp]]

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

Comment 39

[Victor Porof [:vporof][:vp]]

Attachment: v5, part1 - breadcrumbs implementation

Rebased, added preload hack and fixed ux nits.

Comment 40

[Victor Porof [:vporof][:vp]]

Attachment: v5, part2 - move the StackFrames constructor in debugger-toolbar

Comment 41

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

Comment on attachment 704996 [details] [diff] [review]
v5, part1 - breadcrumbs implementation

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

this'll need a rebasin'.

Comment 42

[Victor Porof [:vporof][:vp]]

Attachment: v5, part1 - breadcrumbs implementation (rebased)

that's a paddlin'

Comment 43

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

looks like part 2 needs a rebasin' too. Think I got this one.

Comment 44

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

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

[Dave Camp (:dcamp)]

I would expect the newest stack frame to be on the right too...

Comment 46

[Victor Porof [:vporof][:vp]]

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

Comment 47

[Victor Porof [:vporof][:vp]]

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

Comment 48

[Victor Porof [:vporof][:vp]]

Attachment: v5, combined, rebased

Rebased (again) and combined the patches. They were getting pretty hard to maintain separately.

Comment 49

[Victor Porof [:vporof][:vp]]

Attachment: v5.1, combined, rebased

Comment 50

[Victor Porof [:vporof][:vp]]

Attachment: v5.2, combined, rebased

Rebased...

Comment 51

[Victor Porof [:vporof][:vp]]

Bam! https://hg.mozilla.org/integration/fx-team/rev/dc25ef7de1f2

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/dc25ef7de1f2