1224304 - Once you click the element picker icon and move the mouse over the content area, you can't recover your place in the tree view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Steps to reproduce:

1)  Open inspector.
2)  Select an element in the tree view.
3)  Think you want to select something on the page, click the "Pick an element
    from the page" button. 
4)  Move your mouse over the page, but don't click anywhere.
5)  Change your mind, click the "Pick an element from the page" button again.

EXPECTED RESULTS:  The element selected in step 2 is selected.

ACTUAL RESULTS: Whatever last thing you happened to hover before moving your mouse out of the content area is selected.

Comment 1

[Patrick Brosset <:pbro>]

(In reply to Boris Zbarsky [:bz] from comment #0)
> ACTUAL RESULTS: Whatever last thing you happened to hover before moving your
> mouse out of the content area is selected.
The last element you happened to have hovered over isn't selected (unless you're having a bug I could not reproduce), it is scrolled into view in the inspector, and has a special (pale blue) background that indicates that it has been hovered. The element selected in step 2 is still selected, but it's out of view (and if the DOM is deep, it might be hard to find it again).

This is something that I've found frustrating too, and my idea then was to add something to scroll back to the selected element in the inspector. This could be a button, keyboard shortcut, whatever. But I completely agree we need something to easily navigate (on a related note, I think we need a back button too, see bug 1221590).

Comment 2

[Patrick Brosset <:pbro>]

I don't see where a button could fit in the UI, and right-click ctx menu is out of the question because this selects the node you right-click on. So maybe just hitting a letter on the keyboard would work.
Like, hitting 'S' would scroll back to the currently selected element if it is out of view.
I realize this would make it a very un-discoverable feature, but I just can't think of anything else right now.
In any case, I'm happy to mentor anyone who wants to fix this. Apart from the UX question, this should be very easy to fix.

Comment 3

[Florens Verschelde :fvsch]

Alternate keyboard-only solution: use the Esc key.

1)  Open inspector.
2)  Select an element in the tree view.
3)  Think you want to select something on the page, click the "Pick an element
    from the page" button.
4)  Move your mouse over the page, but don't click anywhere.
5)  Change your mind, *hit the Esc key*.

ACTUAL RESULT: Whatever last thing you happened to hover before hitting the Esc key is highlighted as hovered in the DOM view, and the DOM view is scrolled to show that element.

IMPROVED RESULT: Same as the current result, but when hitting the Esc key the DOM view should scroll back to show the selected element.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fwiw, my instinct was to hit "esc" to try to "stop trying to select something and just go back to the thing I was looking for before", exactly as comment 3 suggests.  I think that would be a quite natural thing to do.

Comment 5

[Stuart]

Hi there,

I'm interested in working on this. Would it be possible to get some kind of hint? I think I need to be within devtools/client/framework/toolbox-highlighter-utils.js and modify the stopPicker function? And I think I'll need to use the toolbox.walker, but I'm not certain as to how exactly.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Stuart from comment #5)
> Hi there,
> 
> I'm interested in working on this. Would it be possible to get some kind of
> hint? I think I need to be within
> devtools/client/framework/toolbox-highlighter-utils.js and modify the
> stopPicker function? And I think I'll need to use the toolbox.walker, but
> I'm not certain as to how exactly.
Hey, thanks for your interest!
I think most of the code will be in devtools/client/markupview/markup-view.js
This is the module that's in charge of the DOM tree view in devtools.

One approach would be for this to remember the scroll position when the picker mode starts, and then restore it when it's canceled (when ESC is pressed).
This could lead to problems though if, while in picker mode, the user highlights elements in the page that cause the tree to expand, in which case the stored scroll position would be wrong.

Another approach would be to just scroll the selected node into view in the DOM tree when the picker mode is canceled.
This might be the best solution. For this, I think you should add a method called something like `scrollIntoView` to MarkupContainer. This way, the MarkupView class could call this when needed.
For info, in the MarkupView class, you can retrieve:
- the selected node: this._inspector.selection.nodeFront
- the container for this node: this.getContainer(this._inspector.selection.nodeFront)
- therefore, to scroll it into view: this.getContainer(this._inspector.selection.nodeFront).scrollIntoView()

Now, you'll need to hook this up to the pick mode. For this, you can listen to 2 events:
this._inspector.toolbox.on("picker-started", cb) -> To know when the picker starts.
this.walker.on("picker-node-canceled", cb) -> To know when the picker is canceled.

This is what came to mind after thinking like 5 minutes about it, so there might be other solutions.
Hope this helps.
I see you're new to bugzilla, so please go through https://wiki.mozilla.org/DevTools/GetInvolved and https://wiki.mozilla.org/DevTools/Hacking before starting. And don't hesitate to ping us on IRC or needinfo me on this bug.

Comment 7

[Patrick Brosset <:pbro>]

By the way, this approach would also be compatible with the "S" shortcut key I mentioned in comment 2. This would be a nice thing to add too.

Comment 8

[Patrick Brosset <:pbro>]

Ahah, right when I was thinking about this, I noticed this tweet from Chromedevtools:
https://twitter.com/ChromeDevTools/status/661234102025121792
So, until now, they didn't have auto-expand, which I think is a powerful feature. So they added it. BUT, they actually auto-collapse too, which means that if you cancel the picker, the DOM tree is back in the state it was before starting it. This is an interesting approach I think. Still, you'd have to make sure the currently selected node is still scrolled in view (they don't do this now, and they don't support ESC either).

Comment 9

[Stuart]

Attachment: Not exactly working.

Hi Patrick,

Sorry for the delay. This is what I've got so far; but it's not quite working as intended. Not quite sure what I should be doing from here.

Regards,
Stuart.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8690017 [details] [diff] [review]
Not exactly working.

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

This looks mostly good. I think you can simplify the code a bit, and make use of scrollIntoViewIfNeeded.
Finally, since this is a quick change, maybe you could also take the opportunity to add the "S" shortcut I mentioned in comment 2 and comment 7.
To do this, find the the _onKeyDown function in MarkupView. Add a new case in the switch statement in this function. Something like:

case Ci.nsIDOMKeyEvent.DOM_VK_S:
  this._selectedContainer.scrollIntoView();
  break;

The last thing that'll be needed for this change is a new automated test to ensure no future changes break this feature without us noticing.
Writing new devtools tests isn't the easiest thing in the world, so you should first read some docs here: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests
And then take a look at existing tests here: \devtools\client\markupview\test\
After this, please let me know if you have questions and if you want help getting started with the test.

::: devtools/client/markupview/markup-view.js
@@ +97,5 @@
>  
>    // Binding functions that need to be called in scope.
>    this._mutationObserver = this._mutationObserver.bind(this);
>    this._onDisplayChange = this._onDisplayChange.bind(this);
> +  this._onToolboxPickerStarted = this._onToolboxPickerStarted.bind(this);

I just realized we don't need to track the picker-node-started event in fact. When the picker is canceled, we can just use the reference of the currently selected container. So you can remove this.

@@ +124,3 @@
>    this._inspector.selection.on("new-node-front", this._onNewSelection);
>    this._inspector.toolbox.on("picker-node-hovered", this._onToolboxPickerHover);
> +  this._inspector.toolbox.on("picker-node-started", this._onToolboxPickerStarted);

and you can remove this.

@@ +160,5 @@
>        this._showContainerAsHovered(nodeFront);
>      }, e => console.error(e));
>    },
>  
> +  _onToolboxPickerStarted: function() {

And this function too.

@@ +165,5 @@
> +    this.pickerStartNode = this._inspector.selection.nodeFront;
> +  },
> +
> +  _onToolboxPickerCanceled: function() {
> +    this.getContainer(this.pickerStartNode).scrollIntoView();

And here, you can do this instead:
this._selectedContainer.scrollIntoView();

@@ +1554,3 @@
>      this._inspector.selection.off("new-node-front", this._onNewSelection);
>      this._inspector.toolbox.off("picker-node-hovered", this._onToolboxPickerHover);
> +    this._inspector.toolbox.off("picker-node-started", this._onToolboxPickerStarted);

You can remove this too.

@@ +2010,5 @@
>        }, this.markup.CONTAINER_FLASHING_DURATION);
>      }
>    },
>  
> +  scrollIntoView: function() {

Please add a jsdoc comment for this new function. It's quite simple so I would normally not ask this, but it can actually be confusing to people, as in, is this going to scroll the page, or the markup-view panel?
So I think it's worth add a quick comment explaining that this scrolls the panel.

@@ +2011,5 @@
>      }
>    },
>  
> +  scrollIntoView: function() {
> +      this._showBoxModel(this.node);

Why did you choose to use this method?
It turns out we already have imported in this module a handy function for scrolling nodes into view, which you can use here:

scrollIntoViewIfNeeded(this.elt);

Comment 11

[Stuart]

Hi Patrick,

Thanks very much for the detailed review. Should be able to submit that patch later today. :)I may well have questions about the tests, but I'll give it a shot first.

> Why did you choose to use this method?
I ended up just trying some methods to see their effect to be honest. Wasn't sure what to leave there for the review.

Don't suppose you have any other mentored bugs that may be suitable for my level to fix on after this?

Regards,
Stuart.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Stuart from comment #11)
> Hi Patrick,
> 
> Thanks very much for the detailed review. Should be able to submit that
> patch later today. :)I may well have questions about the tests, but I'll
> give it a shot first.
Awesome. Let me know how that goes.
> > Why did you choose to use this method?
> I ended up just trying some methods to see their effect to be honest. Wasn't
> sure what to leave there for the review.
Got it.
> Don't suppose you have any other mentored bugs that may be suitable for my
> level to fix on after this?
You can always find bugs at http://firefox-dev.tools/ (select the "mentored bugs" radio button and then check any tools you'd like, you can also try the "easy bugs" category and then just ask to be mentored by dropping a comment on the bug if you need).
Maybe bug 1219611?

Comment 13

[Stuart]

Thanks.

Quick question actually, for the line below, how did you know that "this.elt" was the right thing to use? It wasn't immediately obvious to me what that was or did.

>scrollIntoViewIfNeeded(this.elt);

Regards,
Stuart.

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Stuart from comment #13)
> Thanks.
> 
> Quick question actually, for the line below, how did you know that
> "this.elt" was the right thing to use? It wasn't immediately obvious to me
> what that was or did.
> 
> >scrollIntoViewIfNeeded(this.elt);
Well, it's mostly because I know the classes and objects in markup-view.js really well :)
There's also some logic to it, each object we instantiate in this module manage a given part of the DOM, and they each have a reference to that DOM object via 'this.elt'.
Unfortunately, every panel's UI in devtools is coded a little bit differently, and we don't yet use a framework, so there are some inconsistencies, and that makes it hard for people learning the code.
I can only suggest that using the Browser Toolbox, you set a breakpoint somewhere in the markup-view's code and explorer the scope variables and 'this' properties from there.

Comment 15

[Stuart]

Attachment: Working patch, failing test.

Hi Patrick,

Think it's pretty much there now. Only thing is that when the test runs and presses the escape virtual key, it just hangs there. Not sure what to do on that; if I pass "VK_RETURN", the test fails as expected.

Regards,
Stuart.

Comment 16

[Stuart]

(In reply to Stuart from comment #15)
> Created attachment 8690947 [details] [diff] [review]
> Working patch, failing test.
Just realised I left a line in there that I should have removed. That's been removed, will submit that with the test fix.

> +    this._inspector.toolbox.off("picker-node-started", this._onToolboxPickerStarted);

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8690947 [details] [diff] [review]
Working patch, failing test.

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

::: devtools/client/markupview/markup-view.js
@@ +2011,5 @@
> +  /**
> +  * Scrolls the markup-view panel to it's selected element.
> +  */
> +  scrollIntoView: function() {
> +      scrollIntoViewIfNeeded(this.elt);

nit: we indent with 2 spaces in the devtools codebase.
Please take a look at our style guide, and more importantly, I suggest that you use eslint, this makes it a lot easier to avoid these formatting inconsistencies:
https://wiki.mozilla.org/DevTools/CodingStandards

::: devtools/client/markupview/test/browser_markupview_keybindings_04.js
@@ +32,1 @@
>    assertNodeSelected(inspector, "div");

We also want to check that the "div" is visible in the viewport (scrolled into view), because that's the main thing your patch changes.
For this, I suggest that you create a new test. Keep the changes you made here in this test, they're good. But please also create a new test file in the same directory (making sure you also register it in the browser.ini file in the same directory).
This new test should use a test page with a deep/long DOM tree so that there's a scroll bar in the inspector, it should then start the picker, hover over elements in the page, then simulate ESCAPE, and check that the inspector is scrolled back up.

@@ +85,3 @@
>      options: {}
>    }, {}, false);
>    yield inspector.once("inspector-updated");

When keyBind is VK_ESCAPE, the picker mode is canceled, which means that no new node is going to be selected which, in turn, means that the inspector-updated event is *not* going to be emitted. That's why the test hangs in that case.

You should do something like this instead:

if (keyBind === "VK_ESCAPE") {
  yield inspector.walker.once("picker-node-canceled");
} else {
  yield inspector.once("inspector-updated");
}

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #17)
> We also want to check that the "div" is visible in the viewport (scrolled
> into view), because that's the main thing your patch changes.
> For this, I suggest that you create a new test. Keep the changes you made
> here in this test, they're good. But please also create a new test file in
> the same directory (making sure you also register it in the browser.ini file
> in the same directory).
> This new test should use a test page with a deep/long DOM tree so that
> there's a scroll bar in the inspector, it should then start the picker,
> hover over elements in the page, then simulate ESCAPE, and check that the
> inspector is scrolled back up.
Creating new devtools tests isn't the easiest. You should definitely look at other tests, see if you can reuse some code to do this. And don't hesitate to ask if you want me to provide some code too.

Comment 19

[Patrick Brosset <:pbro>]

I just realized something, the "S" shortcut is already used for scrolling the node *in the page* into view, so we shouldn't use this here. We need to find another shortcut.
Fwiw, in the SublimeText code editor, they have something like this with ctrl+L to jump to the current line selection.

Comment 20

[Stuart]

Attachment: revision 3

Hi Patrick,

Here are the some of the changes mentioned above. Unfortunately I've been doing other things, so the new test is yet to come. But in reference to the old test; this now (since getting the latest code changes) throws a TypeError: listener is not a non-null object. Whereas I'm sure it was passing okay on the day of your comment17. At the moment to me, the stack trace was not entirely useful but that's probably due to my minimal understanding of how the tests work.

Are there recent changes which would effect this test, or is it just me missing something simple?

Comment 21

[Patrick Brosset <:pbro>]

(In reply to Stuart from comment #20)
> this now (since getting the latest code changes) throws a
> TypeError: listener is not a non-null object. Whereas I'm sure it was
> passing okay on the day of your comment17. At the moment to me, the stack
> trace was not entirely useful but that's probably due to my minimal
> understanding of how the tests work.
Ah it might be related to 
yield inspector.walker.once("picker-node-canceled");

the 'once' method of 'EventEmitter' class can be called in 2 ways:
once(eventName, callback)
or
promise = once(eventName)

So, if you omit the callback, then it returns a promise you can use instead, and that's what we use in a lot of tests (in fact we use this heavily along with 'yield' in tasks so that we can write asynchronous code that looks like synchronous code).

*But*, in 'inspector.walker.once("picker-node-canceled")', I'm afraid 'inspector.walker' does not implement 'EventEmitter', so the 'once' method we call here does not behave the same, and it might be that it doesn't support being called with just an eventName and no callback.

Try this instead:

yield new Promise(resolve => inspector.walker.once("picker-node-canceled", resolve));

Comment 22

[Patrick Brosset <:pbro>]

Triaging (filter on CLIMBING SHOES).

Comment 23

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 24

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e01d6d6b4b4

Comment 25

[Greg Tatum [:gregtatum]]

Attachment: Bug 1224304 - Handle canceling the element picker better

This changes the behavior of the element picker so that when it is
cancelled the previously selected DOM node is re-scrolled into view.
Additionally the existing behavior of the keyboard shortcuts for the
element picker was broken when the devtools toolbox was docked. The main
content area was not being focused, so the keyboard shortcuts for the
element picker were not being used. When the toolbox is detached, the
focus event is still not fired, as it's not desirable to have the
content pop into view over the devtools.

Finally there is now an additional implementation of the Escape shortcut
when the devtools are focused. The console Escape shortcut is ignored
until the element picker has been disabled making disabling the element
picker consistent irrelevant of the context.

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

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8800700 [details]
Bug 1224304 - Handle canceling the element picker better

https://reviewboard.mozilla.org/r/85580/#review84300

::: devtools/client/framework/toolbox-highlighter-utils.js:96
(Diff revision 1)
>      });
>    };
>  
>    /**
>     * Start/stop the element picker on the debuggee target.
> +   * @param {boolean} doFocus - Optionally focus the content area once the picker is

s/{boolean}/{Boolean}

::: devtools/client/framework/toolbox-highlighter-utils.js:114
(Diff revision 1)
>     * Start the element picker on the debuggee target.
>     * This will request the inspector actor to start listening for mouse events
>     * on the target page to highlight the hovered/picked element.
>     * Depending on the server-side capabilities, this may fire events when nodes
>     * are hovered.
>     * @return A promise that resolves when the picker has started or immediately

Add a @param for the doFocus as you have done in togglePicker.

::: devtools/client/inspector/markup/markup.js:131
(Diff revision 1)
>    this._onCopy = this._onCopy.bind(this);
>    this._onFocus = this._onFocus.bind(this);
>    this._onMouseMove = this._onMouseMove.bind(this);
>    this._onMouseOut = this._onMouseOut.bind(this);
>    this._onToolboxPickerHover = this._onToolboxPickerHover.bind(this);
> +  this._onToolboxPickerCanceled = this._onToolboxPickerCanceled.bind(this);

Moved this above this._onToolboxPickerHover. That way it lists _onToolboxPickerX alphabetically.

::: devtools/client/inspector/markup/markup.js:152
(Diff revision 1)
>    this.walker.on("mutations", this._mutationObserver);
>    this.walker.on("display-change", this._onDisplayChange);
> +
>    this._inspector.selection.on("new-node-front", this._onNewSelection);
>    this._inspector.toolbox.on("picker-node-hovered", this._onToolboxPickerHover);
> +  this._inspector.toolbox.on("picker-canceled", this._onToolboxPickerCanceled);

Move this above "picker-node-hovered". Again just keeping some sort of alphabetical order.

::: devtools/client/inspector/markup/markup.js:215
(Diff revision 1)
> +   * If the element picker gets canceled, make sure and re-center the view on the
> +   * current selected element.
> +   */
> +  _onToolboxPickerCanceled: function () {
> +    if (this._selectedContainer) {
> +      scrollIntoViewIfNeeded(this._selectedContainer.editor.elt, true);

true is unnecessary since the default value for centered is true in scrollIntoViewIfNeeded(elem, centered)

::: devtools/client/inspector/test/browser_inspector_highlighter-cancel.js:14
(Diff revision 1)
> +const TEST_URL = URL_ROOT + "doc_inspector_long-divs.html";
> +
> +add_task(function* () {
> +  let {inspector, toolbox, testActor} = yield openInspectorForURL(TEST_URL);
> +
> +  info("Selecting div#focus-here");

Remove this info() since it is repeats what we can infer from selectAndHighlightNode(). Also, selectAndHighlightNode does an info().

::: devtools/client/inspector/test/doc_inspector_long-divs.html:1
(Diff revision 1)
> +<!doctype html>

This file should have probably have a license header.

I seen html files use either one:

"<!-- Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/ -->"
     
and 

"/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */"

::: devtools/client/inspector/test/doc_inspector_long-divs.html:8
(Diff revision 1)
> +<head>
> +  <meta charset="utf-8">
> +  <title>Inspector Long Div Listing</title>
> +  <style>
> +    div {
> +      background-color:#0002;

Add a space between property name and value.

::: devtools/client/inspector/test/head.js:106
(Diff revision 1)
>    return nodeOrSelector;
>  }
>  
>  /**
>   * Start the element picker and focus the content window.
>   * @param {Toolbox} toolbox

Add a new @param for skipFocus and description.

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8800700 [details]
Bug 1224304 - Handle canceling the element picker better

https://reviewboard.mozilla.org/r/85580/#review84608

Comment 28

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5c5bcfffa3a

Comment 29

[Greg Tatum [:gregtatum]]

Comment on attachment 8800700 [details]
Bug 1224304 - Handle canceling the element picker better

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

Comment 30

[Greg Tatum [:gregtatum]]

Comment on attachment 8800700 [details]
Bug 1224304 - Handle canceling the element picker better

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

Comment 31

[Greg Tatum [:gregtatum]]

Comment on attachment 8800700 [details]
Bug 1224304 - Handle canceling the element picker better

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85580/diff/3-4/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d9e3c771a6c
Handle canceling the element picker better r=gl

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d9e3c771a6c

Comment 34

[Surentharan]

[bugday-20170215]