Graph mouse movement doesn't work when entering another graph

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

unspecified
Firefox 40
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Going to handle this separately from Bug 1066504, since it's proven to be a different issue.

There's some code to sync the selections.  https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Graphs.jsm?from=graphs.jsm#2079, but it doesn't work, because there is no selection event firing on graph2 during the move (since it's not currently selecting).

I think the right solution is to bind mousemove / mouseup to the top level window instead of the frame window instead.
(Assignee)

Updated

3 years ago
No longer blocks: 1066504
Depends on: 1066504
Blocks: 1147941
No longer blocks: 1145697
Priority: -- → P2
(Assignee)

Comment 1

3 years ago
I'll take a look at this
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
mvp brian
(Assignee)

Comment 3

3 years ago
Created attachment 8600124 [details] [diff] [review]
timeline-range-across-graphs.patch

v1.  Playing with this, I think it fixes all the nits I have with the mouse selection on the main graph currently.  It will allow you to continue mouse movement across the entire browser (including other graphs), and reliably handle mouseup outside of the toolbox / browser window.

I'm not sure how you want to handle tests.  The tests pass locally with the patch applied because right now I am accepting an undefined `screenX` as an indication that we should just use clientX (which is what the tests pass in).  We could alternatively actually simulate the mouse events on relevant DOM nodes so the other properties are available.
Attachment #8600124 - Flags: feedback?(vporof)
Comment on attachment 8600124 [details] [diff] [review]
timeline-range-across-graphs.patch

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

Looks good and feels good!

The only issue is that the cursors can become misaligned between graphs. If mousing over each graph, the | doesn't stay in the same place or get hidden as before.
Attachment #8600124 - Flags: feedback?(vporof) → feedback+
Created attachment 8600418 [details]
Screen Shot 2015-05-01 at 3.42.55 PM.png
(Assignee)

Comment 6

3 years ago
Created attachment 8601189 [details] [diff] [review]
timeline-range-across-graphs-1.patch

Restores the disappearing cursor behavior.  Test patch incoming
Attachment #8600124 - Attachment is obsolete: true
Attachment #8601189 - Flags: review?(vporof)
(Assignee)

Comment 7

3 years ago
Created attachment 8601192 [details] [diff] [review]
timeline-range-across-graphs-2.patch

Renames clientX -> testX and clientY -> testY throughout the tests.  Also adds a new test that doesn't use the shortcut of calling event callbacks directly and instead uses dispatchEvent with a MouseEvent, setting screenX and screenY to make sure this part doesn't regress.
Attachment #8601192 - Flags: review?(vporof)
(Assignee)

Comment 8

3 years ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc3a5aae6892
Comment on attachment 8601189 [details] [diff] [review]
timeline-range-across-graphs-1.patch

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

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +945,5 @@
>     */
> +  _getRelativeEventCoordinates: function(e) {
> +    // For ease of testing, testX and testY can be passed in as the event
> +    // object.  If so, just return this.
> +    if (e.testX !== undefined) {

Nit: check for both testX and testY and via `"foo" in bar` to avoid calling getters (unlikely to ever be an issue, but not a bad idea in general).
Attachment #8601189 - Flags: review?(vporof) → review+
Attachment #8601192 - Flags: review?(vporof) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8601541 [details] [diff] [review]
timeline-range-across-graphs-1.patch

Folded the new test for this into part 1.  Going to upload the change from clientX/clientY to testX/testY in part 2.
Attachment #8601189 - Attachment is obsolete: true
Attachment #8601541 - Flags: review+
(Assignee)

Comment 11

3 years ago
Created attachment 8601610 [details] [diff] [review]
timeline-range-across-graphs-2.patch

clientX -> testX, clientY -> testY.  Also updates FlameGraph.js to accept these properties just to remove any weirdness of the two graph types accepting different parameters in tests.
Attachment #8601610 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8601192 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f1ad7599e4b
(Assignee)

Comment 13

3 years ago
Try push looks good
Keywords: checkin-needed
(Assignee)

Comment 14

3 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/abc82c178273
remote:   https://hg.mozilla.org/integration/fx-team/rev/164c7af2cc5d
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/abc82c178273
https://hg.mozilla.org/mozilla-central/rev/164c7af2cc5d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
\o/
(Assignee)

Updated

3 years ago
Blocks: 1144779
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1144793
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1144799
(Assignee)

Updated

2 years ago
Depends on: 1165228
You need to log in before you can comment on or make changes to this bug.