Last Comment Bug 715970 - Highlighted node should center in the visualization (Tilt)
: Highlighted node should center in the visualization (Tilt)
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Victor Porof [:vporof][:vp]
: Patrick Brosset <:pbro>
Depends on: 722129
Blocks: 719042
  Show dependency treegraph
Reported: 2012-01-06 11:11 PST by Victor Porof [:vporof][:vp]
Modified: 2012-02-13 06:54 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v0 (2.00 KB, patch)
2012-01-29 04:33 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v1 (5.95 KB, patch)
2012-01-29 06:49 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (6.37 KB, patch)
2012-01-29 10:04 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.1 (7.98 KB, patch)
2012-02-02 09:17 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-01-06 11:11:39 PST
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] 2012-01-29 04:33:27 PST
Created attachment 592485 [details] [diff] [review]

Comment 2 Victor Porof [:vporof][:vp] 2012-01-29 06:49:46 PST
Created attachment 592500 [details] [diff] [review]
Comment 3 Victor Porof [:vporof][:vp] 2012-01-29 10:04:13 PST
Created attachment 592517 [details] [diff] [review]

Fixed a few small problems with moveIntoView().
Comment 4 Victor Porof [:vporof][:vp] 2012-01-30 00:25:20 PST
Comment on attachment 592517 [details] [diff] [review]

Green is the color:
Comment 5 Victor Porof [:vporof][:vp] 2012-02-02 09:17:06 PST
Created attachment 593880 [details] [diff] [review]

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) 2012-02-08 12:29:31 PST
Comment on attachment 593880 [details] [diff] [review]

    *                 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] 2012-02-08 12:40:27 PST
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) 2012-02-08 13:08:27 PST
Comment on attachment 593880 [details] [diff] [review]

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) 2012-02-10 07:27:48 PST
Comment 10 Tim Taubert [:ttaubert] 2012-02-13 06:54:59 PST

Note You need to log in before you can comment on or make changes to this bug.