The tree map should zoom at centering around the mouse pointer

RESOLVED FIXED in Firefox 48

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: magicp.jp, Assigned: gregtatum)

Tracking

Trunk
Firefox 48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Flags: needinfo?(gtatum)
Priority: -- → P1
Assignee

Comment 2

3 years ago
Argh, this should fix it. Was measuring off of the new width/height, not the
old width/height.
Attachment #8741121 - Flags: review?(nfitzgerald)
Assignee

Updated

3 years ago
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)
Assignee

Comment 4

3 years ago
The errors have been incorrect implementations on my part. I can create a browser test that measures exact DOM changes.
Assignee

Comment 5

3 years ago
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)
Assignee

Updated

3 years ago
Attachment #8741121 - Attachment is obsolete: true
Flags: needinfo?(gtatum)
Assignee

Comment 6

3 years ago
Posted 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+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dece9231ef6c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.