947143 - The debugger resumption order panel looks really bad

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

Attachment: Screen Shot 2013-12-06 at 13.09.30.png

We should use Tooltip.js instead.

Comment 1

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

It also used to wrap the text, but that got broken somehow.

Comment 2

[Tim Nguyen :ntim]

I'd like to take this, can you guys point me in the right direction ?

Comment 3

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

Hi Tim! Let me know if you need help with building Firefox from source [0] and running browser chrome mochitests [1].

The source code for the debugger lives here: [2]. Basically, the current panel is implemented from scratch in XUL [3], and it's referenced and used in the "toolbar view", here: [4]. It's styled in our css theme files, for linux, windows and os x, for example: [5].

We have a robust implementation called Tooltip.js [6] that has the ability of showing simple text content (amongst other things). Using Tooltip.js instead of manually creating our own panel would fix this bug and keep the code easier to maintain in the future.

For this bug, you'd have to remove all references of the current panel from js, xul and css, and use Tooltip.js instead. Read the documentation on how to use that library [7] and use it in the debugger. It'd be nice if you also improved the `setTextContent` function [8] with the ability to show the warning icon.

Let me know if you need any help! Thank you.

[0] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[1] https://developer.mozilla.org/en/docs/Browser_chrome_tests
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger
[3] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul#465
[4] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#16
[5] http://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/devtools/debugger.css#437
[6] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js
[7] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js#119
[8] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js#405

Comment 4

[Tim Nguyen :ntim]

How do I make the panel appear actually (in the UI, not the code) ?

Comment 5

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #4)
> How do I make the panel appear actually (in the UI, not the code) ?

As a web developer I actually have no idea how to use the debugger, that's why I'm asking you that actually. I use all developer tools except that one :p

Comment 6

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

1. Open http://htmlpad.org/resumption1 in a tab and start the debugger
2. Click the "First button"
3. Open http://htmlpad.org/resumption2 in a second tab and start another debugger there
4. Click the "Second button"
5. Go back to the first tab and resume execution (press the |> button in the debugger's toolbar)

Comment 8

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

Hey Tim, any progress here? Anything we can do to help?

Comment 9

[Tim Nguyen :ntim]

(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> Hey Tim, any progress here? Anything we can do to help?

Sorry, I've been so busy with homework lately (even during the holidays). I tried some stuff, but I can't seem to compile the Firefox build properly. It just doesn't update the code I changed.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Tim Nguyen [:ntim] from comment #9)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> > Hey Tim, any progress here? Anything we can do to help?
> 
> Sorry, I've been so busy with homework lately (even during the holidays). I
> tried some stuff, but I can't seem to compile the Firefox build properly. It
> just doesn't update the code I changed.

What are you running to do an incremental build? The following should update your build with any devtools changes:

  ./mach build toolkit/devtools browser

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See also https://wiki.mozilla.org/DevTools/Hacking

Comment 12

[Alexey Novak]

Hi there,

Just wondering how far is Tim working on this bug. Do not want to be rude, but if Tim is still trying to figure out the build process I would love to take care of this issue since it does look like a good first bug. After poking in the code I already have a patch for it.

Comment 13

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

You guys could race each other :)

Tim, let us know how you're doing with creating a patch for this bug. Did you manage to build Firefox?

Comment 14

[Tim Nguyen :ntim]

I haven't done a patch yet so, if Alexey wants to take this, I'm ok with it. I don't seem to have any problem building firefox now.

Comment 15

[Alexey Novak]

Attachment: 947143.1.patch

Thanks Tim. If you are OK with it I would love to give it a shot.

Few notes about it:
- I used fx-team instead of mozilla-central to create my patch, assuming that this work is related to DevTools. If I need to do same changes in mozilla-central then I can do that.
- By default ToolTip attaches itself to the top left corner of the play button which would always make it go out of the FF window boundaries. I decided to shift it right 20px so it appears within the window.
- I have a question about ToolTip styling. I did not do any modifications to it... yet. So it is showed half-transparent. If I need to make any style modifications to it what would they be, any suggestions? Do we want to make it look exactly as resumption-order-panel or maybe we want to keep it as it is?
- I did not do any modifications for `setTextContent` yet. Before start coding on it I wanted to check if you have any specific suggestions on how I supposed to modify the function. I was thinking to add an extra argument isAlert which is defaulted to false. In case when it is passed as `true` tooltip will insert an alert icon into its content body.

Thanks.

Comment 16

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

Comment on attachment 8357135 [details] [diff] [review]
947143.1.patch

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

This looks really good! Thank you! Just a nit below, and please rebase on fx-team tip (bug 943883 landed, which unifies all the debugger.css files into debugger.inc.css). Once you attach the updated patch I'll land it.

::: browser/devtools/debugger/debugger-view.js
@@ +29,5 @@
>  const EDITOR_VARIABLE_HOVER_DELAY = 350; // ms
>  const EDITOR_VARIABLE_POPUP_OFFSET_X = 5; // px
>  const EDITOR_VARIABLE_POPUP_OFFSET_Y = 0; // px
>  const EDITOR_VARIABLE_POPUP_POSITION = "before_start";
> +const TOOLBAR_ORDER_POPUP_OFFSET_X = 20; // px

Instead of an offset, try using "topcenter bottomleft" popup positioning.

You'll have to declare a const here, TOOLBAR_ORDER_POPUP_POSITION = "..." and set it as the value for the defaultPosition property on the tooltip instance.

::: browser/themes/linux/devtools/debugger.css
@@ +509,5 @@
>    min-height: 25px !important;
>    padding: 0 !important;
>  }
>  
>  #resumption-panel-desc {

Do we still need this selector?

Comment 17

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

(In reply to Alexey Novak from comment #15)

> - I have a question about ToolTip styling. I did not do any modifications to
> it... yet. So it is showed half-transparent. If I need to make any style
> modifications to it what would they be, any suggestions? Do we want to make
> it look exactly as resumption-order-panel or maybe we want to keep it as it
> is?

That's the default styling. I don't think it's necessary to modify this for now, but please feel free to file a bug about the transparency if it bothers you. We'll take a look at it and decide whether or not removing transparency would be better.

> - I did not do any modifications for `setTextContent` yet. Before start
> coding on it I wanted to check if you have any specific suggestions on how I
> supposed to modify the function. I was thinking to add an extra argument
> isAlert which is defaulted to false. In case when it is passed as `true`
> tooltip will insert an alert icon into its content body.
> 

Yeah, that can be safely done in a followup. Filed bug 957634. If you want to also work on that and have the time, it'd be swell, but don't feel obliged :)

Comment 18

[Alexey Novak]

Hi Victor,

Thanks a lot for a quick reply and for your detailed response, it helps A LOT.
I definitely want to take care of 957634 once I'm finished with the current bug.

Also will address your comments ^

Comment 19

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

Excellent, thank you!

Comment 20

[Alexey Novak]

Attachment: Replacing resumption order panel with a tooltip. part 2

I made the changes as you suggested. It looks good when a tooltip centered on the button.

Also I applied CSS changes to debugger.inc.css instead of 3 different CSS files according to introduced changes in Dev Tools. Looks like all CSS is getting consolidated into 1 file which easier to manage, I assume.

Please let me know if there is anything I need to change. Thanks.

Comment 21

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

Landed! https://hg.mozilla.org/integration/fx-team/rev/3feaef0837d6

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3feaef0837d6