901250 - Implement "Scroll Into View" menu item for the Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P1)

Description

[ziyunfei]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1585.0 Safari/537.36




Expected results:

Like Firebug or Chrome's devtools, when click the "Scroll Into View" menu item, the current selected element will be scrolled into view, even the selected element is in an frame's document.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

See also bug https://bugzilla.mozilla.org/show_bug.cgi?id=1024068

For that bug, the idea is that when clicking on a node that is both visible in the DOM and also not in the viewport, we scroll the viewport to ensure that the node is in view. There are various reports that this used to work. Chrome has a command in their context menu they took straight from Firebug - I can see this as an option because the interaction is less automagical.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Jeff Griffiths (:canuckistani) from comment #2)
> See also bug https://bugzilla.mozilla.org/show_bug.cgi?id=1024068
> 
> For that bug, the idea is that when clicking on a node that is both visible
> in the DOM and also not in the viewport, we scroll the viewport to ensure
> that the node is in view.

I'd much prefer this as a context menu item to avoid any annoying behavior in which navigating nodes in the inspector makes the page jump around.

Comment 4

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

Jen, assigning to you as a potential polish bug.

Brian, since Jen is unfamiliar with this part of the codebase would you be available to mentor her here, or is someone else a better fit?

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] from comment #4)
> Jen, assigning to you as a potential polish bug.
> 
> Brian, since Jen is unfamiliar with this part of the codebase would you be
> available to mentor her here, or is someone else a better fit?

I can do that.  This is going to be a lot like the "Copy unique selector" feature when right clicking on an element in the markup view.

Basically there is a context menu item added to the inspector frontend, and then a method on the NodeActor (which is a representation of a DOM node, running in the content process).

Here are some relevant bits from the copy unique selector feature to help get started.  These can generally be copied and modified to fit for this bug.

* There is a menu item in frontend (node-menu-copyuniqueselector): http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul#51
* Here is the localized string: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/inspector.dtd#22 
* The frontend's call to the NodeActor method: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#993
* The NodeActor getUniqueSelector method: https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#575.  For the new scrollIntoView method, the actual scroll can happen using node.scrollIntoView().

Once the basics are in place we'll need to also discuss backwards compatibility and where the tests should live.  Let me know if you have any questions.

Comment 7

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug901250.patch

Here's the initial patch for feedback without tests. Let me know if anything needs fixing before I continue with the rest. Thanks!

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8589871 [details] [diff] [review]
Bug901250.patch

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

Good start!  As we discussed, there should be a change to the actor in inspector.js and the following tests should be updated:

https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/test/browser_inspector_menu-01-sensitivity.js
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/test/browser_inspector_menu-04-other.js

And if possible, it would be good to have a new server side test similar to this one: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/mochitest/test_inspector-changevalue.html

Comment 9

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug901250.patch

I think this is the most of what I could figure out based on your tips - let me know if it's on the right track or if some of the edits are incorrect.

Also ended up using rect.x and rect.y to verify position instead of top, bottom, left and right which seemed to work well for all tests. Let me know if we need to be more specific with all four coordinates and I can update.

I did notice that running the actor test by itself had a console.error message at the end:
Message: Error: Connection closed, pending request to server1.conn0.domnode54, type scrollIntoView failed

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8590548 [details] [diff] [review]
Bug901250.patch

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

Looking good, a few more notes.  Patrick, please have a look at my first inline note, trying to figure out the best way to detect this new feature.

::: browser/devtools/inspector/inspector-panel.js
@@ +115,5 @@
>      return this._target.client.traits.getUniqueSelector;
>    },
>  
> +  get canScrollIntoView() {
> +    return this._target.client.traits.scrollIntoView;

We have a new way to check for the existence of a function on an actor: this._target.actorHasMethod("domnode", "scrollIntoView").  Unfortunately, that's async so it makes getting it more inconvenient.  We could maybe do something like this at the end of _deferredOpen instead:

    this.canScrollIntoView = false;
    this._target.actorHasMethod("domnode", "scrollIntoView").then(value => {
      this.canScrollIntoView = value;
    });

Which has the benefit of not adding another trait, but is more clunky and could technically have a period of time where the value is wrong.  Also, we have a pretty clear pattern for doing this here with traits.

I don't really love my suggested approach, and can't decide which is better.  Patrick, what do you think?

@@ +685,5 @@
>        copyInnerHTML.removeAttribute("disabled");
>        copyOuterHTML.removeAttribute("disabled");
> +
> +      if (this.canScrollIntoView) {
> +        scrollIntoView.removeAttribute("disabled");

I'd go ahead and removeAttribute unconditionally here, then set the .hidden property on it below if !this.canScrollIntoView.  Similar to the unique.hidden = true line below

::: browser/devtools/inspector/test/browser_inspector_menu-04-other.js
@@ +70,5 @@
> +    yield selectNode("#scroll-view", inspector);
> +    let showScrollIntoView = inspector.panelDoc.getElementById("node-menu-scrolltoselector");
> +    ok(showScrollIntoView, "the popup menu has a scroll into view item");
> +
> +    let scrollNode = content.document.querySelector("#scroll-view");

As we discussed, this test is blocked on Bug 1137285 being landed and moving the DOM access into the test actor.  If you get through the other comments and that bug is still unresolved, I'd be OK with moving the changes to this test into a follow up bug, since the scrolling behavior is already tested on the server.

::: browser/devtools/inspector/test/doc_inspector_menu.html
@@ +15,5 @@
>        </div>
>        <p data-id="copy">Paragraph for testing copy</p>
>        <p id="sensitivity">Paragraph for sensitivity</p>
>        <p id="delete">This has to be deleted</p>
> +      <p id="scroll-view">Paragraph for scroll to view</p>

You could set style="margin-top: 1000px;" here instead of in the script (one less thing we need to deal with remotely)

::: toolkit/devtools/server/tests/mochitest/test_inspector-scroll-into-view.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=

Nit: I know these aren't filled out for others in the same folder, but if you can add the bug number here and in the <title> it'd be handy for people who see the test later

@@ +43,5 @@
> +  });
> +});
> +
> +addTest(function testScrollIntoView() {
> +  let rect = gInspectee.querySelector("#a").getBoundingClientRect();

I think you'll actually want to check #z (or any element that isn't already in the viewport to start), since #a is at the top of the page

@@ +45,5 @@
> +
> +addTest(function testScrollIntoView() {
> +  let rect = gInspectee.querySelector("#a").getBoundingClientRect();
> +  let nodeFront;
> +  promiseDone(gWalker.querySelector(gWalker.rootNode, "#a").then(front => {

This is a case where the new Task.async syntax will make a lot easier:


addTest(Task.async(function* testScrollIntoView() {
  let nodeFront = yield gWalker.querySelector(gWalker.rootNode, "#z");
  
  // assert that the DOM Node is not in view

  yield nodeFront.scrollIntoView();

  // assert that the DOM Node is in view
}));

@@ +46,5 @@
> +addTest(function testScrollIntoView() {
> +  let rect = gInspectee.querySelector("#a").getBoundingClientRect();
> +  let nodeFront;
> +  promiseDone(gWalker.querySelector(gWalker.rootNode, "#a").then(front => {
> +    nodeFront = front;

Can you add an assertion before scrolling to make sure it wasn't in the viewport to start?

@@ +47,5 @@
> +  let rect = gInspectee.querySelector("#a").getBoundingClientRect();
> +  let nodeFront;
> +  promiseDone(gWalker.querySelector(gWalker.rootNode, "#a").then(front => {
> +    nodeFront = front;
> +    nodeFront.scrollIntoView();

You'll have to yield on scrollIntoView (it's async because it's a method on the actor)

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8590548 [details] [diff] [review]
Bug901250.patch

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

::: browser/devtools/inspector/inspector-panel.js
@@ +115,5 @@
>      return this._target.client.traits.getUniqueSelector;
>    },
>  
> +  get canScrollIntoView() {
> +    return this._target.client.traits.scrollIntoView;

I agree that we already have a clear way of doing this kind of things in the inspector-panel, and that doing it another way would add some inconsistency in the code, so I'd be somehow ok to r+ the patch as is, but the problem with this is that it introduces a new trait that will be hard to get rid of if/once we move to using actorHasMethod instead (since the server won't know if the client needs the trait or not).

The other solution is in _setupNodeMenu:
- disable the item,
- call actorHasMethod,
- when that resolves, either enable the item or keep it disabled depending on the result,

=> iirc, actorHasMethod caches the actor spec, so we don't have to worry about it taking time every time the menu is shown, it may only take a short while the first time it's displayed.

@@ +1008,5 @@
>  
>    /**
> +   * Scroll the selector into view.
> +   */
> +  scrollToSelector: function InspectorPanel_scrollToUniqueSelector()

nit: The function name should be InspectorPanel_scrollToSelector to be consistent with the property name scrollToSelector.
However, see my comment about the scrollToSelector name in inspector.dtd.
Also, I think it would be nice if this method took an optional nodeFront argument, and defaulted to the current selection.
So, something like this:

/**
 * Scroll a given node into view.
 * @param {NodeFront} nodeFront Optional reference to the nodeFront to scroll,
 * defaults to the current selection.
 * @return {Promise} Resolves when the node has been scrolled into view.
 */
scrollNodeIntoView: function(nodeFront=this.selection.nodeFront) {
  if (!nodeFront.isNode()) {
    return;
  }

  return nodeFront.scrollIntoView();
},

Note that returning the promise returned by the method is useful for consumers to be able to wait for this to be done.

::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
@@ +71,5 @@
> +<!-- LOCALIZATION NOTE (inspectorScrollToSelector.label): This is the label
> +     shown in the inspector contextual-menu for the item that lets users scroll
> +     the current node into view -->
> +<!ENTITY inspectorScrollToSelector.label       "Scroll Into View">
> +<!ENTITY inspectorScrollToSelector.accesskey   "S">

I'm not sure why this is called inspectorScrollToSelector (I've seen the same "toSelector" suffix was used in inspector.xul and inspector-panel.js in multiple places).
This is about scrolling DOM nodes into view, and the whole logic relies on passing node references around, not selectors.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11)
> 
> ::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
> @@ +71,5 @@
> > +<!-- LOCALIZATION NOTE (inspectorScrollToSelector.label): This is the label
> > +     shown in the inspector contextual-menu for the item that lets users scroll
> > +     the current node into view -->
> > +<!ENTITY inspectorScrollToSelector.label       "Scroll Into View">
> > +<!ENTITY inspectorScrollToSelector.accesskey   "S">
> 
> I'm not sure why this is called inspectorScrollToSelector (I've seen the
> same "toSelector" suffix was used in inspector.xul and inspector-panel.js in
> multiple places).
> This is about scrolling DOM nodes into view, and the whole logic relies on
> passing node references around, not selectors.

Right, good catch.  This should be called inspectorScrollNodeIntoView and the function scrollNodeIntoView.

Comment 13

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug901250.patch

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8591975 [details] [diff] [review]
Bug901250.patch

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

Just a few more tweaks needed to the frontend, but this is getting close.  Also, the commit message shouldn't mention selector anymore - maybe 'Add scroll into view menu item for the inspector'

::: browser/devtools/inspector/inspector-panel.js
@@ +678,5 @@
> +
> +    scrollIntoView.setAttribute("disabled", "true");
> +
> +    this._target.actorHasMethod("domnode", "scrollIntoView").then(value => {
> +      this.canScrollIntoView = value;

I think we can directly set `scrollIntoView.hidden = !value;` in this place (and not keep track of the canScrollIntoView bit).  Well, Patrick said to use disabled but I think for backwards compat issues we've usually used hidden / visible.

@@ +686,5 @@
>        unique.removeAttribute("disabled");
>        copyInnerHTML.removeAttribute("disabled");
>        copyOuterHTML.removeAttribute("disabled");
> +
> +      if (this.canScrollIntoView) {

We can get rid of the `if` and always execute this line here.  If the server doesn't support this feature, it will be hidden so it doesn't matter if it's disabled or not

@@ +693,4 @@
>      } else {
>        unique.setAttribute("disabled", "true");
>        copyInnerHTML.setAttribute("disabled", "true");
>        copyOuterHTML.setAttribute("disabled", "true");

The line from 679 can move here - scrollIntoView.setAttribute("disabled", "true")

::: browser/devtools/inspector/test/browser_inspector_menu-04-other.js
@@ +64,5 @@
>      });
>    }
>  
> +  function* testScrollIntoView() {
> +    todo(false, "Verify that node is scrolled into the viewport.");

Please add a comment here pointing to Bug 1154107 (the follow up to implement this)

Comment 15

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug901250.patch

updated - thanks!

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8592333 [details] [diff] [review]
Bug901250.patch

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

Looks good to me.  Pushed to try:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=de1e556bc1a6

Comment 17

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug901250.patch

Fixed with executeSoon.

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

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8593089 [details] [diff] [review]
Bug901250.patch

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

Interdiff looks good.  Assuming try turns up green let's check it in!

Comment 19

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/0e494c706e41

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0e494c706e41