703346 - Scroll to offscreen element when you click the infobar

Status: RESOLVED WORKSFORME

(DevTools :: Inspector, defect, P3)

Description

[Kevin Dangoor]

When the selected element is offscreen, it would be convenient if clicking the highlighter infobar would bring that element to view.

Note that this may be less important if we had bug 694954

Comment 1

[Rob Campbell [:rc] (:robcee)]

found during a bugsearch. Moving to DevTools:Inspector.

Comment 2

[Spyros Livathinos]

I'd like to attempt and fix this.

Comment 3

[Paul Rouget [:paul]]

The solution here is to make the infobar clickable (addEventListener in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/highlighter.jsm#423) and then scroll on click.

Spyros, I assign you to this bug. Ping me or robcee on IRC (#devtools) if you need any help.

Comment 4

[Spyros Livathinos]

Attachment: Suggested working solution.

Comment 5

[Paul Rouget [:paul]]

Comment on attachment 593780 [details] [diff] [review]
Suggested working solution.

First, thanks a lot for taking care of this bug :)

>diff -r e33539a90ae2 browser/devtools/highlighter/highlighter.jsm
>--- a/browser/devtools/highlighter/highlighter.jsm	Sun Jan 29 20:42:00 2012 -0800
>+++ b/browser/devtools/highlighter/highlighter.jsm	Thu Feb 02 12:53:17 2012 +0200
>@@ -90,16 +90,18 @@ const INSPECTOR_INVISIBLE_ELEMENTS = {
>  *   void hide();
>  *   boolean isHidden();
>  *
>  *   // Redraw the highlighter if the visible portion of the node has changed.
>  *   void invalidateSize(aScroll);
>  *
>  *   // Is a node highlightable.
>  *   boolean isNodeHighlightable(aNode);
>+ *   // Is a node hidden.
>+ *   boolean isNodeHidden(aNode);
>  *
>  *   // Add/Remove lsiteners
>  *   // @param aEvent - event name
>  *   // @param aListener - function callback
>  *   void addListener(aEvent, aListener);
>  *   void removeListener(aEvent, aListener);
>  *
>  * Events:
>@@ -214,17 +216,16 @@ Highlighter.prototype = {
>    *        Should we scroll to ensure that the selected node is visible.
>    */
>   highlight: function Highlighter_highlight(aNode, aScroll)
>   {
>     if (this.hidden)
>       this.show();
> 
>     let oldNode = this.node;
>-
>     if (!aNode) {
>       if (!this.node)
>         this.node = this.win.document.documentElement;
>     } else {
>       this.node = aNode;
>     }
> 
>     if (oldNode !== this.node) {
>@@ -343,16 +344,43 @@ Highlighter.prototype = {
>   isNodeHighlightable: function Highlighter_isNodeHighlightable(aNode)
>   {
>     if (aNode.nodeType != aNode.ELEMENT_NODE) {
>       return false;
>     }
>     let nodeName = aNode.nodeName.toLowerCase();
>     return !INSPECTOR_INVISIBLE_ELEMENTS[nodeName];
>   },
>+
>+  /**
>+   * Is the specified node visible in the user's current view?
>+   * @param nsIDOMNode aNode
>+   *        the DOM element in question
>+   * @returns boolean
>+   *          True if the node is relatively hidden or false otherwise.
>+   */
>+   isNodeHidden: function Highlighter_isNodeHidden(aNode)
>+   {
>+      let viewBBorder = this.win.innerHeight + this.win.pageYOffset;
>+      let viewLBorder = this.win.innerWidth + this.win.pageXOffset;
>+      let viewUBorder = this.win.pageYOffset;
>+      let viewRBorder = this.win.pageXOffset;
>+      let elementYPosition = aNode.offsetTop;
>+      let elementXPosition = aNode.offsetLeft;
>+      
>+      // Jump to the selected element's position if it is not inside 
>+      // the user's viewable area
>+      if (elementYPosition > viewBBorder || elementYPosition < viewUBorder || 
>+          elementXPosition > viewLBorder || elementYPosition < viewRBorder) {
>+        return true; 
>+       }
>+
>+      return false;
>+   },
>+
>   /**
>    * Build the veil:
>    *
>    * <vbox id="highlighter-veil-container">
>    *   <box id="highlighter-veil-topbox" class="highlighter-veil"/>
>    *   <hbox id="highlighter-veil-middlebox">
>    *     <box id="highlighter-veil-leftbox" class="highlighter-veil"/>
>    *     <box id="highlighter-veil-transparentbox"/>
>@@ -549,16 +577,26 @@ Highlighter.prototype = {
>       for (let i = 0; i < this.node.classList.length; i++) {
>         let classLabel = this.chromeDoc.createElement("label");
>         classLabel.className = "highlighter-nodeinfobar-class plain";
>         classLabel.textContent = "." + this.node.classList[i];
>         fragment.appendChild(classLabel);
>       }
>       classes.appendChild(fragment);
>     }
>+
>+    aNode = this.node;
>+    // If the node is hidden from the user's view, 
>+    // make sure to scroll so that it's visible.
>+    if (!!this.isNodeHidden(aNode)) {

You already return a boolean. No need to add "!!".

>+      this.nodeInfo.container.addEventListener("click", function() {
>+          aNode.scrollIntoView(true);
>+      });
>+    }

You add a listener every time we update the infobar, without removing them. It means that when
you will click, you will call scrollIntoView dozens of time (the infobar is updated every time
a node is highlight).

Better to do:

    this.nodeInfo.container.addEventListener("click", function() {
      if (this.isNodeHidden(this.node)) {
        aNode.scrollIntoView(true);
       }
     });

once for all in buildInfobar.

See what I mean?

Also, I don't think you need to implement and use isNodeHidden.
scrollIntoView already test that for you. So I would just call scrollIntoView every time
the infobar is clicked, without testing anything.

Next step: uploading a new patch addressing these comments (and mark this one as obsolete).
Ping me on IRC if you need more explanations!

Thanks!

Comment 6

[Spyros Livathinos]

Attachment: New patch with edits according to the comments above.

Got rid of the isNodeHidden in favor of just using scrollIntoView without a check and instantiated a new Listener when the infoBar is built.

Comment 7

[Paul Rouget [:paul]]

> scrollIntoView already test that for you. So I would just call scrollIntoView every time
the infobar is clicked, without testing anything.

So apparently I was wrong here. ScrollIntoView will scroll the page every time you click.

We need to use scrollIntoViewIfNeeded (bug 403510).

Comment 8

[Paul Rouget [:paul]]

I guess that it will take time before we get something like `scrollIntoViewIfNeeded`. I feel like a JS implementation in our devtools might be easier to do.

See comment 5, and disregard the note about `isNodeHidden`.

Comment 9

[Spyros Livathinos]

Attachment: Proposed fix.

Added a JS implementation of the scrollIntoViewIfNeeded(aNode) function which is using isNodeHidden to determine whether the element in question is out of the user's current view.

Comment 10

[Paul Rouget [:paul]]

triage, filter on centaur

Comment 11

[Paul Rouget [:paul]]

JavaScript implementation of scrollIntoViewIfNeeded: https://gist.github.com/2581101

Comment 12

[Paul Rouget [:paul]]

Comment on attachment 603659 [details] [diff] [review]
Proposed fix.

To improve this code, we need to support iframes and center the node.

Comment 13

[Geoff Lankow (:darktrojan)]

Spyros, I've just created bug 754659 which might be a good next bug for you after this one.

Also, back to this bug, if the infobar is going to be clickable, I think it should be underlined when the mouse is over it, and/or the cursor should change.

Comment 14

[Paul Rouget [:paul]]

Now that bug 724585 is fixed, it should be easier to fix this bug.
Spyros, want to give it a try?

Comment 15

[Rob Campbell [:rc] (:robcee)]

assigned and with patch? Anything to do here?

Comment 16

[Spyros Livathinos]

I'll look into implementing Paul's suggestions if it's not too late to resume work on this.

Comment 17

[Paul Rouget [:paul]]

(In reply to Spyros Livathinos from comment #16)
> I'll look into implementing Paul's suggestions if it's not too late to
> resume work on this.

It's never too late :)

Comment 18

[Spyros Livathinos]

Attachment: Proposed fix (new scrollIntoView implementation)

I implemented a new scrollIntoViewIfNeeded which centers the node automatically in the user's currently viewable area.

I'm not sure if this is the best way to go forward, especially since there is already another implementation of scrollIntoViewIfNeeded here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/LayoutHelpers.jsm#235

Would it make more sense to extend the existing function instead of having two different implementations?

Comment 19

[Paul Rouget [:paul]]

(In reply to Spyros Livathinos from comment #18)
> Would it make more sense to extend the existing function instead of having
> two different implementations?

Yes.

Comment 20

[Spyros Livathinos]

(In reply to Paul Rouget [:paul] from comment #19)
> (In reply to Spyros Livathinos from comment #18)
> > Would it make more sense to extend the existing function instead of having
> > two different implementations?
> 
> Yes.

As it currently stands, the scrollIntoViewIfNeeded function centers the node ONLY if the node is hidden from the user's view ie. if we have a partially visible node, the node is not being centered but the window is scrolled with a scrollTo functionality (top of of the element aligns with the window). Do we want this functionality?

Iframes are supported by the way.

Comment 21

[Spyros Livathinos]

Attachment: Proposed fix (reusing scrollIntoViewIfNeeded)

Reused older scrollIntoViewIfNeeded implementation from LayoutHelpers.

Comment 22

[Spyros Livathinos]

Attachment: Proposed fix (reusing scrollIntoViewIfNeeded)

Comment 23

[Paul Rouget [:paul]]

Comment on attachment 671748 [details] [diff] [review]
Proposed fix (reusing scrollIntoViewIfNeeded)

>+    this.nodeInfo.container.removeEventListener("click", this, false);

This won't remove the listener (you don't add "this", but your own bound function.

>+    // When the user clicks on the infobar of an "out-of-view" node,
>+    // make sure to scroll the view so that the node is visible.
>+    this.nodeInfo.container.addEventListener("click", (function() {
>+      LayoutHelpers.scrollIntoViewIfNeeded(this.node);
>+    }).bind(this), false);

I don't think you want to do that on the container. Clicking on the menu for example, would call this function, and we don't want that. Use the box around the text (highlighter-nodeinfobar-text)

Comment 24

[Paul Rouget [:paul]]

This has been fixed by the Toolbox bug.

Bug triage, filter on PINKISBEAUTIFUL