Closed Bug 1420112 Opened 7 years ago Closed 6 years ago

Unify "jump to debugger" icons

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox59 wontfix, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: nchevobbe, Assigned: pradeepgangwar)

Details

(Keywords: good-first-bug)

Attachments

(3 files, 1 obsolete file)

Screen Shot 2017-11-23 at 10.41.54.png
516.42 KB, image/png
Details
Changed design
92.69 KB, image/png
Details
Bug 1420112 - Refresh event bubble design; .
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
In https://github.com/devtools-html/devtools-core/pull/812, we added an icon to jump to the definition of a logged function.

In the event bubble, we also have an icon to jump to the debugger (a breakpoint-like icon).

We should use the same icon in the console and in the event bubble.

Furthermore, I'd say that the one we have in the event bubble at the moment can confuse user and make them think they'll add a breakpoint at the said location. A "navigate-to" icon would be better (could be another one that the one added in Reps of course).
I think we can even change how the files are shown in the bubble to : 
> ▶︎ filename + location ↱

Clicking on the `▶︎` (and the filename) icon would expand/toggle the preview
Clicking `↱` would open the debugger.
What do you think Victoria ?
Severity: normal → enhancement
Flags: needinfo?(victoria)
Priority: -- → P3
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #0)
>
> Furthermore, I'd say that the one we have in the event bubble at the moment
> can confuse user and make them think they'll add a breakpoint at the said
> location. A "navigate-to" icon would be better (could be another one that
> the one added in Reps of course).

+1

On top of thinking that it would add a breakpoint, I had several chats with users who thought this was just for decoration and would never think about clicking on it.

Also think your proposal to change the position of the icon is good. Having it after the location seems more natural for an action that should "jump" to a location.
Wow, yeah, Nicolas' idea seems like a huge improvement! Both the breakpoint icon AND the disclosure by clicking "bubbling/DOM" are so confusing. Sounds great to have the ↱ at the end of the rows.
Flags: needinfo?(victoria)
Also, I see that we are labelling those events (jquery, react), it would be really nice if we could have access to all the library icons from the debugger and put it here instead of a dull text.
Attached image Changed design — 
Quick hacking on this
if someone is willing to take this bug, i'm putting my WIP patch in mozreview so it can be applied easily.
Keywords: good-first-bug
Yes, framework icons would be great!

With this new design, does it still make sense for the bubbling/capturing/dom badges to be on the right of this popup? I'm wondering if they should be moved between the function name and source path, so that the jump-to-debugger icons could be right-most (and aligned to right).
(In reply to Victoria Wang [:victoria] from comment #10)
> Yes, framework icons would be great!
> 
> With this new design, does it still make sense for the
> bubbling/capturing/dom badges to be on the right of this popup? I'm
> wondering if they should be moved between the function name and source path,
> so that the jump-to-debugger icons could be right-most (and aligned to
> right).

I think it would make sense to do what you suggest, since it will be : 
event name | event informations | source | jump to source

the only drawback is that the infomations (bubble, jquery, bubble) can be quite "bold". Having framework icon would tame that, but maybe we can go further an think of a cleaner wait to show those information ?
Yeah, it feels like those info badges might be more bold than they need to be. 

I'm also wondering about moving them to the left of event name so that there's more of a... sort of code-like hierarchy, where class names go before functions? Although bubbling/capturing don't represent a library/class name, so maybe those need to go elsewhere? (this may be a totally misguided idea, but just in case):

▶︎ (React icon) React: event_name ...path ↱

...with the event name colored blue or something to make it stand out more.

I feel like the source path could use some cleanup as it's a bit overwhelming-looking. Some simple things like remove the ?(hash) from after filename, truncate sooner to add some whitespace, adding syntax highlighting to the line number, etc, would help.
I might be getting too much into the weeds with the latest comments though - I definitely think that your current work that adds the disclosure/jump icons should be submitted as a patch, and these other ideas can be continue to be discussed for a later patch :)
I can try this bug. But i think for this i need to run inspector in browser mode like debugger.html. Please help me with that and i will try if i can help in this bug. :)
Flags: needinfo?(nchevobbe)
(In reply to Pradeep Gangwar [:pradeepgangwar] from comment #14)
> I can try this bug. But i think for this i need to run inspector in browser
> mode like debugger.html. Please help me with that and i will try if i can
> help in this bug. :)

We don't have this kind of thing for the inspector at the moment. You need to build and run firefox like `./mach build faster && ./mach run`
Flags: needinfo?(nchevobbe)
I applied the patch given, and it worked very well. I would like to ask what more changes are needed other than those applied in your patch.
Flags: needinfo?(nchevobbe)
(In reply to Pradeep Gangwar [:pradeepgangwar] from comment #16)
> I applied the patch given, and it worked very well. I would like to ask what
> more changes are needed other than those applied in your patch.

This should be modified to work in dark mode too, which it does not right now.
Also, you should run the tests to make sure we did not break one.
And finally, since we added something on the header to have a collapsed/expanded test, we should add a test for that as well.
Flags: needinfo?(nchevobbe)
Hello Pradeep,
Are you still working on this ? No pressure, I'm asking just to know if I can direct people here if they are willing to work on this bug.
Flags: needinfo?(pradeepgangwar39)
Actually i tried it a few weeks ago and i think to make "jump to debugger" icon work in dark mode i need to modify svg code written for it. But i had no idea of working with svg, hence i got stuck so after some attempts. I need guidance on that part.
Flags: needinfo?(pradeepgangwar39) → needinfo?(nchevobbe)
Hello Pradeep,

I updated my WIP patch to work in dark mode too: https://reviewboard.mozilla.org/r/203860/diff/1-2/

Here are the important bits : 

- In the SVG (which I simplified), there is this attribute `stroke="context-stroke"`. This says that the color is not defined directly in the svg
- Then, I put the following CSS for the image : 
```css
.event-tooltip-debugger-icon {
  -moz-context-properties: stroke;
   stroke: currentColor;
}
```

-moz-context-properties allow us to control the stroke property from the CSS (See https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-context-properties). Setting stroke to `currentColor` will then make the icon the same color as the text (defined in the "younger" ancestor.
So here the icon color will adapt to dark mode.

So what's left for this bug to land is to make sure the tests are fine, and if not, fix them. Also we should add a test for the expanded/collapsed state.

Do you have enough information to resume your work ? Don't hesitate to tell me if it's not the case so we can help you move forward on this :)
Assignee: nobody → pradeepgangwar39
Flags: needinfo?(nchevobbe)
Thanks! I will learn writing test and update here if i need some help. :)
So you can run the following command : 

./mach test devtools/client/inspector/markup/test/browser_markup_events_01.js devtools/client/inspector/markup/test/browser_markup_events_02.js devtools/client/inspector/markup/test/browser_markup_events_03.js devtools/client/inspector/markup/test/browser_markup_events_04.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.0.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.1.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.11.1.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.2.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.3.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.4.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.6.js devtools/client/inspector/markup/test/browser_markup_events_jquery_1.7.js devtools/client/inspector/markup/test/browser_markup_events_jquery_2.1.1.js devtools/client/inspector/markup/test/browser_markup_events_react_development_15.4.1.js devtools/client/inspector/markup/test/browser_markup_events_react_development_15.4.1_jsx.js devtools/client/inspector/markup/test/browser_markup_events_react_production_15.3.1.js devtools/client/inspector/markup/test/browser_markup_events_react_production_15.3.1_jsx.js devtools/client/inspector/markup/test/browser_markup_events_react_production_16.2.0.js devtools/client/inspector/markup/test/browser_markup_events_react_production_16.2.0_jsx.js --headless

This run the tests (./mach test) which are testing the event bubble, in headless (--headless), mode, which mean it won't open a new firefox window (it does if you don't pass --headless).

Those tests all rely on https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/devtools/client/inspector/markup/test/helper_events_test_runner.js#30-128 which is the place you might need to modifications in.
Hello Pradeep, I wanted to check if you're blocked by something on this ?
If you did not have the time to look into the bug, no worries :)
Flags: needinfo?(pradeepgangwar39)
I read that file carefully and understood it's working. At the end i came up with something, that might not be the correct approach. If it's not a correct approach, please guide me if i need to write a new function.
Flags: needinfo?(pradeepgangwar39)
Comment on attachment 8944200 [details]
Bug 1420112 - Refresh event bubble design; .

https://reviewboard.mozilla.org/r/214486/#review220148

Thanks a lot pradeep.

I pushed the patch to TRY (our CI, which run all the tests to make sure everything is okay with this patch).
I have a couple of comments, but nothing really big.
With those changes and a green TRY, I'll r+ you.

::: commit-message-6bb6f:1
(Diff revision 1)
> +Bug 1420112- Unify 'jump to debugger' icons; r=nchevobbe

There should be a space after the bug number: 

Bug 1420112 - ...


Also, I think the commit message should be more precise, something like: 

Bug 1420112 - Refresh event bubble design; r=nchevobbe.

Change the order of the icons so it makes more sense.
Use te same "jump to debugger" icon as in the console.
Add expanded/collapsed icon on event listener definition.

::: devtools/client/inspector/markup/test/helper_events_test_runner.js:122
(Diff revision 1)
> +    is(header.classList.contains("content-expanded"), true,
> +        "We are in expanded state and icon changed");

we should also assert that the header does not contain the class before we click on it.
Attachment #8932809 - Attachment is obsolete: true
Comment on attachment 8944200 [details]
Bug 1420112 - Refresh event bubble design; .

https://reviewboard.mozilla.org/r/214486/#review220238
Attachment #8944200 - Flags: review?(nchevobbe) → review-
Comment on attachment 8944200 [details]
Bug 1420112 - Refresh event bubble design; .

https://reviewboard.mozilla.org/r/214486/#review221174

Only one comment left about the commit message, but once this is resolved, we can land this patch.

::: commit-message-6bb6f:1
(Diff revision 2)
> +Bug 1420112- Unify 'jump to debugger' icons; r=nchevobbe
> +
> +MozReview-Commit-ID: 10q9QYTmeO7
> +***

those 3 lines should be deleted

::: devtools/client/inspector/markup/test/helper_events_test_runner.js:116
(Diff revision 2)
> +    is(header.classList.contains("content-expanded"), false,
> +        "We are not in expanded state");

nice !
Attachment #8944200 - Flags: review?(nchevobbe) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da500b8a4368
Refresh event bubble design; r=nchevobbe.
https://hg.mozilla.org/mozilla-central/rev/da500b8a4368
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: