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.

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

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.

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.

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.

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.

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

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.

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

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

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.

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

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.

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

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.

(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 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"); }

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

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.

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?

(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));

Triaging (filter on CLIMBING SHOES).

[good first bug] whiteboard -> keyword mass change

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 on attachment 8800700 [details] Bug 1224304 - Handle canceling the element picker better https://reviewboard.mozilla.org/r/85580/#review84608

Pushed by gtatum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1d9e3c771a6c Handle canceling the element picker better r=gl

[bugday-20170215]