715970 - Highlighted node should center in the visualization (Tilt)

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

Sometimes we're selecting a node using the breadcrumbs or the HTML panel which is not visible in the visualization viewport. It would be nice if the camera automatically translated/zoomed/centered/danced so we can see it.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v0

WIP.

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v2

Fixed a few small problems with moveIntoView().

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 592517 [details] [diff] [review]
v2

Green is the color: https://tbpl.mozilla.org/?tree=Try&rev=ce9d3ecd89e5

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: v2.1

Made sure bringing the node into view only happens when the selection is changed via the highlighter or breadcrumbs, and not by picking inside Tilt. I think this way is better. Rob, what do you think?

Comment 6

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

Comment on attachment 593880 [details] [diff] [review]
v2.1

    *                 the index of the node in the this.traverseData array
+   * @param {String} aFlags
+   *                 flags specifying highlighting options
    */
-  highlightNodeFor: function TVP_highlightNodeFor(aNodeIndex)
+  highlightNodeFor: function TVP_highlightNodeFor(aNodeIndex, aFlags)
   {
     this.redraw = true;

do you see adding other flags in the future? Could the be better served by a flags object or even a moveIntoView boolean parameter?

Comment 7

[Victor Porof [:vporof][:vp]]

I think other flags are pretty feasible someday ("bounce"? "makeItPop"?). Maybe using a string for this is too hacky (the nice thing about it that instead of simply a string you could also use an array like ["moveIntoView, "makeItPop"] because indexOf).

An object may be more obvious. But then we're dealing with redundancy (like highlight(aNode, { moveIntoView: false });), since I don't see a scenario when flags should be turned off, since they're all off by default. Which approach do you prefer?

Comment 8

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

Comment on attachment 593880 [details] [diff] [review]
v2.1

I don't feel strongly about it. Providing room for future flags is an ok reason to do this and the string check isn't horrible.

Comment 9

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

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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