Closed Bug 1263397 Opened 4 years ago Closed 4 years ago

The tree map should zoom at centering around the mouse pointer

Categories

(DevTools :: Memory, defect, P1)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: magicp.jp, Assigned: gregtatum)

Details

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160409030219

Steps to reproduce:

1. Start latest Nightly
2. Go to any sites (e.g. about:home)
3. Open DevTools > Memory
4. Select Tree Map view (default)
5. Take snapshot
6. Zoom-in on the tree map at centering around the mouse pointer using mouse wheel 


Actual results:

Cannot zoom at centering around the mouse pointer.


Expected results:

The tree map should zoom at centering around the mouse pointer.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Flags: needinfo?(gtatum)
Priority: -- → P1
Argh, this should fix it. Was measuring off of the new width/height, not the
old width/height.
Attachment #8741121 - Flags: review?(nfitzgerald)
Assignee: nobody → gtatum
Comment on attachment 8741121 [details] [diff] [review]
The tree map should zoom at centering around the mouse pointer

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

How can we test the zooming better? Zooming has been a little finicky, and I don't feel comfortable landing new changes without a testing story for the zooming.
Attachment #8741121 - Flags: review?(nfitzgerald)
The errors have been incorrect implementations on my part. I can create a browser test that measures exact DOM changes.
This patch adds specific units to the tests to measure specifically what changed.
Before it was just checking general relationships, like if the zoom was in between
certain values. This should lock in the current behavior. The only other way to test
this behavior would be to redo the calculations for centering on the test. I'm not
sure if you feel that it warrants that, or if locking in specific values for the
working behavior will be enough.
Attachment #8741357 - Flags: review?(nfitzgerald)
Attachment #8741121 - Attachment is obsolete: true
Flags: needinfo?(gtatum)
Attached video zoom-behavior.mov
This video demonstrates the fixed zooming behavior.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #6)
> Created attachment 8741358 [details]
> zoom-behavior.mov
> 
> This video demonstrates the fixed zooming behavior.

Great!
Comment on attachment 8741357 [details] [diff] [review]
The tree map should zoom at centering around the mouse pointer

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

\o/

::: devtools/client/memory/test/browser/browser_memory_tree_map-02.js
@@ +120,5 @@
> +       "Translate X was affected by the mouse position");
> +    ok(floatEquality(dragZoom.translateY, 18.18181818181817),
> +       "Translate Y was affected by the mouse position");
> +    is(dragZoom.zoom, 0.2, "Zooming starts at 0");
> +  }

Thank you very much! This let's me sleep at night :)
Attachment #8741357 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dece9231ef6c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.