Closed Bug 1145784 Opened 9 years ago Closed 9 years ago

Graph mouse movement doesn't work when entering another graph

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
No longer blocks: 1066504
Depends on: 1066504
Priority: -- → P2
I'll take a look at this
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
mvp brian
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+
Restores the disappearing cursor behavior.  Test patch incoming
Attachment #8600124 - Attachment is obsolete: true
Attachment #8601189 - Flags: review?(vporof)
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)
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+
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+
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+
Attachment #8601192 - Attachment is obsolete: true
Try push looks good
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/abc82c178273
https://hg.mozilla.org/mozilla-central/rev/164c7af2cc5d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Blocks: 1144779
Depends on: 1165228
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.