1318060 - Add onDOMNodeMouseOver/onDOMNodeMouseOut/onInspectIconClick props for DOM panel

Status: RESOLVED FIXED

(DevTools :: DOM, defect, P2)

Description

[Tim Nguyen :ntim]

See bug 1307941 which added those for new console.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nice, thanks for the report!

Honza

Comment 2

[Iulian Radu]

Hi, I'd like to work on this one. Can you give me some pointers, please?

Comment 3

[Tim Nguyen :ntim]

(In reply to Iulian Gabriel Radu from comment #2)
> Hi, I'd like to work on this one. Can you give me some pointers, please?

You'll need to make the toolbox reference here [0] somehow available to the dom panel's global object. Once that's done, you'll need to define a function like [1] then pass it as prop in [2].
Maybe Nicolas will have something to add?


[0] https://dxr.mozilla.org/mozilla-central/source/devtools/client/dom/dom-panel.js#22
[1] https://reviewboard.mozilla.org/r/91518/diff/2#file2591246
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/dom/content/components/dom-tree.js#63

Comment 4

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #3)
> Maybe Nicolas will have something to add?
*add to the instructions.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ntim, what function did you mean to highlight in comment 3?  The [1] link doesn't seem to highlight any particular code.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Hi there,

I think Tim wanted to point to https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/grip-message-body.js#72 , which is where we pass the onDOMNodeMouseOver/onDOMNodeMouseOut props to the Rep.

You can see where we get the highlight functions from the toolbox here : https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#64

Comment 7

[Tim Nguyen :ntim]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> :ntim, what function did you mean to highlight in comment 3?  The [1] link
> doesn't seem to highlight any particular code.

Weird, directs me to highlightNode and unhighlightNode within the diff (which is expected). Not sure where it led you.

Anyways, here's the code in question:

        highlightDomElement: (grip, options = {}) => {
          return this.toolbox && this.toolbox.highlighterUtils
            ? this.toolbox.highlighterUtils.highlightDomValueGrip(grip, options)
            : null;
        },
        unHighlightDomElement: (forceHide = false) => {
          return this.toolbox && this.toolbox.highlighterUtils
            ? this.toolbox.highlighterUtils.unhighlight(forceHide)
            : null;

Comment 8

[Andrei Petcu]

Can I work on this? I see nobody is assigned to this issue anymore.

I never contributed to Firefox's main repo, I added 3 small contributions to debugger.html some time ago and I want to try my luck with this bug. :)

Comment 9

[Tim Nguyen :ntim]

(In reply to andrei from comment #8)
> Can I work on this? I see nobody is assigned to this issue anymore.
> 
> I never contributed to Firefox's main repo, I added 3 small contributions to
> debugger.html some time ago and I want to try my luck with this bug. :)

Sure, assigned it to you :)

Let us know if you need any help!

Comment 10

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1318060 - Add onDOMNodeMouseOver/onDOMNodeMouseOut props for DOM panel;

Review commit: https://reviewboard.mozilla.org/r/139768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139768/

Comment 11

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1318060 - Added tests to ensure the right nodes are highlighted on hover;

Review commit: https://reviewboard.mozilla.org/r/139770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139770/

Comment 12

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8868191 [details]
Bug 1318060 - Add onDOMNodeMouseOver/onDOMNodeMouseOut props for DOM panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139768/diff/1-2/

Comment 13

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1318060 - Added tests to ensure the right nodes are highlighted on hover;

Review commit: https://reviewboard.mozilla.org/r/139788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139788/

Comment 14

[Matt R [:stylizit] (slow, away until Sep)]

- Added onDOMNodeMouseOver/onDOMNodeMouseOut props, it seems to pass tests in devtools/client/dom/test/ locally.
- Added new tests

I canceled previous review request because an empty file was added

Comment 15

[Tim Nguyen :ntim]

You might want to add the onInspectIconClick prop as well, that would select the node in the inspector tool.

Comment 16

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #15)
> You might want to add the onInspectIconClick prop as well, that would select
> the node in the inspector tool.

Good suggestion! Is it ok to do it as part of this patch?

Comment 17

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1318060 - Added onInspectIconClick prop and associated test;

Review commit: https://reviewboard.mozilla.org/r/140618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140618/

Comment 18

[Matt R [:stylizit] (slow, away until Sep)]

> You might want to add the onInspectIconClick prop as well, that would select
> the node in the inspector tool.

Just added this, and added a test as well.

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

How should I test this feature?
It isn't clear to me what to do to highlight elements using the DOM panel.
Also, I am not sure how to inspect an element from within the DOM panel.

Honza

Comment 20

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Jan Honza Odvarko [:Honza] from comment #19)
> How should I test this feature?
> It isn't clear to me what to do to highlight elements using the DOM panel.
> Also, I am not sure how to inspect an element from within the DOM panel.
> 
> Honza

Sure!

1) Open https://www.google.co.uk/
2) Open Inspector and the DOM Panel
3) Expand the "document" prop
4) Find a HTML element in the props/expanded props (for example <body>, or div#viewport.ctr-p from "childNodes")
5) Hover on it
6) The element should be highlighted in blue
7) Click the blue squared icon nearby a node and it should redirect to the inspector tab and select the right node in it

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8868191 [details]
Bug 1318060 - Add onDOMNodeMouseOver/onDOMNodeMouseOut props for DOM panel;

https://reviewboard.mozilla.org/r/139768/#review149150

Looks good to me!

Honza

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8868208 [details]
Bug 1318060 - Added tests to ensure the right nodes are highlighted on hover;

https://reviewboard.mozilla.org/r/139788/#review149154

The test fails for me if I run the entire DOM test suite:

mach test devtools/client/dom/test/

(it passes when I run just this single test alone)

444 INFO TEST-UNEXPECTED-FAIL | devtools/client/dom/test/browser_dom_nodes_highlight.js | Test timed out - Buffered messages finished

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8868958 [details]
Bug 1318060 - Added onInspectIconClick prop and associated test;

https://reviewboard.mozilla.org/r/140618/#review149156

Looks reasonable.

R+

Thanks for working on this!

Honza

Comment 24

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Jan Honza Odvarko [:Honza] from comment #22)
> The test fails for me if I run the entire DOM test suite:
> 
> mach test devtools/client/dom/test/
> 
> (it passes when I run just this single test alone)
> 
> 444 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/dom/test/browser_dom_nodes_highlight.js | Test timed out -
> Buffered messages finished

Thank you for your time and the review! I will investigate this issue

Comment 25

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Jan Honza Odvarko [:Honza] from comment #22)

> 444 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/dom/test/browser_dom_nodes_highlight.js | Test timed out -
> Buffered messages finished

That's strange, on OSX all passed fine at the time I pushed this patch. I've also just pulled from latest m-c and rebased this patch on top and it doesn't happen to me, everything is passing when running the full suite. 

Should we push to TRY to see what happens?

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Matt R :stylizit from comment #25)
> Should we push to TRY to see what happens?
Yes, here is try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90c238a6a8ddd1bdc42de54909679fe907e6e734

Honza

Comment 27

[vihavira]

@Honza can I work on this one while the other one I was working on is evaluated by others in devtools?

Comment 28

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 29

[Erik Carrillo [:E_Carr]]

Hello, I am an Outreachy applicant and would like to know if I can have this bug assigned to me. Thank you.

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Hello Erik,

For this bug, someone already started working on it, so you can look at the diffs to see what they were doing:

• https://hg.mozilla.org/mozreview/gecko/rev/38f24f5f16fc21503d69ae8723d75b24e1acce51#index_header
• https://hg.mozilla.org/mozreview/gecko/rev/d45817a25e93b8aebb518df8ae765b2b6582bcfc#index_header
• https://hg.mozilla.org/mozreview/gecko/rev/361d61b8d17cd2ace6a99e94adc3fc2b0c0d12cd#index_header

Unfortunately, we can't apply those patches directly, as they're too old and the code probably changed a bit.

This bug is about the DOM panel, that isn't enabled by default. You need to go to the devtools settings, and check the DOM checkbox in the Default Developer Tools section.

Then, if you want to understand what to do, you can open the DOM panel on any page, and expand the document property. In there, you should see a body property. Basically what we want is to have the same features we have when we do a document.body in the console: have an icon to select the element in the inspector, and on hover, highlight the element in the page.

This one might be a bit more challenging than the previous bug you worked on, so don't be discouraged, and don't hesitate to ask questions, either here or on Slack :)

Comment 31

[Patrick Brosset <:pbro>]

Hi Erik. Do you still intend to work on this bug? I'm just checking to know if you simply need more time (which is totally fine) or if aren't going to work on it anymore (in which case, just let me know and I'll make this bug available to others).

Comment 32

[Tim Nguyen :ntim]

Attachment: Bug 1318060 - Add onDOMNodeMouseOver/onDOMNodeMouseOut props for DOM panel. r=Honza

Comment 33

[Tim Nguyen :ntim]

Attachment: Bug 1318060 - Add tests to ensure the right nodes are highlighted on hover. r=Honza

Depends on D26485

Comment 34

[Tim Nguyen :ntim]

Attachment: Bug 1318060 - Add onInspectIconClick prop and associated test. r=Honza

Depends on D26486

Comment 35

[Tim Nguyen :ntim]

Since I keep on getting notifications for this bug and the only blocker is a test issue (which isn't great for new contributors to tackle), I decided to take this over the finish line :)

Erik, I see you're already assigned to 4 other open bugs, it would be great if you could finish those first.

Comment 36

[Nicolas Chevobbe [:nchevobbe]]

(I guess you put the wrong assignee email Tim :) )

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69d6fc2992eb
Add onDOMNodeMouseOver/onDOMNodeMouseOut props for DOM panel. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c17b4deacd3
Add tests to ensure the right nodes are highlighted on hover. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c255ed5a31
Add onInspectIconClick prop and associated test. r=Honza

Comment 38

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/69d6fc2992eb
https://hg.mozilla.org/mozilla-central/rev/4c17b4deacd3
https://hg.mozilla.org/mozilla-central/rev/46c255ed5a31

Comment 39

[Erik Carrillo [:E_Carr]]

(In reply to Tim Nguyen :ntim from comment #35)

Since I keep on getting notifications for this bug and the only blocker is a test issue (which isn't great for new contributors to tackle), I decided to take this over the finish line :)

Erik, I see you're already assigned to 4 other open bugs, it would be great if you could finish those first.

No worries Tim, I am currently working on my other bugs so as you mentioned it be better if I focus on those first.