The tree map should zoom at centering around the mouse pointer

RESOLVED FIXED in Firefox 48

Status

DevTools
Memory
P1
normal
RESOLVED FIXED
2 years ago
7 days ago

People

(Reporter: magicp, Assigned: gregtatum)

Tracking

Trunk
Firefox 48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8739696 [details]
cannot-zoom-at-centering-around-the-mouse-pointer.mp4

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

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

Comment 2

2 years ago
Created attachment 8741121 [details] [diff] [review]
The tree map should zoom at centering around the mouse pointer

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

2 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

2 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

2 years ago
Created attachment 8741357 [details] [diff] [review]
The tree map should zoom at centering around the mouse pointer

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

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

Comment 6

2 years ago
Created attachment 8741358 [details]
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

2 years ago
Keywords: checkin-needed

Comment 11

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

Updated

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