1066504 - Timeline range selection is interrupted if mouse goes out of timeline area

Status: RESOLVED FIXED

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

Description

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: Attempt to mark 500ms-1000ms range.

When one wants to select are in a recorded timeline to “zoom in”, one drags mouse over the range from which to which one wants to “zoom in”.

If mouse during the drag goes out of timeline area, the selection will be cancelled and one will have to start zooming in anew. This should not happen and
zoom-in process should not be interrupted by mouse going out the area of timeline.

See video for demonstration.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Running into this frequently myself on all the timeline graphs (framerate, category).

Adding this to new perf tool tracking.

Comment 2

[Victor Porof [:vporof][:vp]]

Yeah, this is Graphs.jsm being a bit dumb about what to do when a mouseout happens. We should fix this.

Meanwhile, you can scroll or click and drag to select offscreen regions.

Comment 7

[Victor Porof [:vporof][:vp]]

Note that dragging also doesn't work. The same underlying bug prevents this, namely mouseout events killing a selection in progress.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: timeline-range.patch

This fixes the problem as I reported in Bug 1136848.  Just allow the selection to continue after mouseout and catch the case where the mouseup happens outside of the toolbox (in which case it never gets fired).  I'm not sure exactly what  the remaining code in _onMouseOut is doing and when it should be executed.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8569380 [details] [diff] [review]
timeline-range.patch

Clearing feedback request for Jordan

Comment 11

[Victor Porof [:vporof][:vp]]

Comment on attachment 8569380 [details] [diff] [review]
timeline-range.patch

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

Sure why not. If it feels right it's right. Test maybe?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> Comment on attachment 8569380 [details] [diff] [review]
> timeline-range.patch
> 
> Review of attachment 8569380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sure why not. If it feels right it's right. Test maybe?

I'll add one.  I also noticed that the same problem happened when dragging an existing selection around, so I've added in support for that.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: timeline-range.patch

There are a couple other things I want to fix wrt to graph selection, but this handles the case of a mouseup happening outside of the toolbox in the case of selecting, dragging, and resizing an existing selection.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4ba6b76f9ae

Comment 14

[Victor Porof [:vporof][:vp]]

Comment on attachment 8578333 [details] [diff] [review]
timeline-range.patch

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

ship it

Comment 15

[Victor Porof [:vporof][:vp]]

Comment on attachment 8578333 [details] [diff] [review]
timeline-range.patch

Actually now that I play with the patch some more, I notice that cursors aren't synced between graphs anymore.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #15)
> Comment on attachment 8578333 [details] [diff] [review]
> timeline-range.patch
> 
> Actually now that I play with the patch some more, I notice that cursors
> aren't synced between graphs anymore.

I'm not seeing it - what are the STR?

Comment 17

[Victor Porof [:vporof][:vp]]

Attachment: screen.gif

Just move the mouse between graphs

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: timeline-range.patch

Try this one.  Had to still clear out the cursor and input attribute on mouseout, but only if the mouse wasn't currently active (had a mousedown with no mouseup).

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8579589 [details] [diff] [review]
timeline-range.patch

Meant to r?

Comment 20

[Victor Porof [:vporof][:vp]]

Moving into the Profiler component. Filter on GUTHRIE'S WAVY CAKES.

Comment 22

[Victor Porof [:vporof][:vp]]

Comment on attachment 8579589 [details] [diff] [review]
timeline-range.patch

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

This still doesn't handle the case when the mouse moves from one graph to another. Do we want to fix that here, or in a different bug? I've been duping such bugs to this one, hoping the fix would be the same for everything. Apparently, it's not :(

Feel free to r+ if you think that should be handled in a different bug, but please file a followup and make it block perf-tool-papercuts.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #22)
> Comment on attachment 8579589 [details] [diff] [review]
> timeline-range.patch
> 
> Review of attachment 8579589 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This still doesn't handle the case when the mouse moves from one graph to
> another. Do we want to fix that here, or in a different bug? I've been
> duping such bugs to this one, hoping the fix would be the same for
> everything. Apparently, it's not :(
> 
> Feel free to r+ if you think that should be handled in a different bug, but
> please file a followup and make it block perf-tool-papercuts.

I filed Bug 1145784 for this after discussing it and spending some time looking into it, since it seems that it's a separate issue altogether.

So as long as this try turns up green, I'd say let's proceed with the incremental improvement here and handle the graph movement separately: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5277587d803f

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8579589 [details] [diff] [review]
timeline-range.patch

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

Let's land this and follow up with 1145784

Comment 25

[Victor Porof [:vporof][:vp]]

ship it!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e6349d80d3b5

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6349d80d3b5