Closed Bug 1066504 Opened 5 years ago Closed 5 years ago

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

Categories

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

35 Branch
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: nagisa, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Running into this frequently myself on all the timeline graphs (framerate, category).

Adding this to new perf tool tracking.
Blocks: perf-tool-v2
Flags: needinfo?(vporof)
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.
Flags: needinfo?(vporof)
Duplicate of this bug: 1117216
Duplicate of this bug: 1130440
Duplicate of this bug: 1132761
Duplicate of this bug: 1132761
Note that dragging also doesn't work. The same underlying bug prevents this, namely mouseout events killing a selection in progress.
No longer blocks: perf-tool-v2
Duplicate of this bug: 1136848
Attached patch timeline-range.patch (obsolete) — Splinter Review
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.
Attachment #8569380 - Flags: feedback?(vporof)
Attachment #8569380 - Flags: feedback?(jsantell)
Comment on attachment 8569380 [details] [diff] [review]
timeline-range.patch

Clearing feedback request for Jordan
Attachment #8569380 - Flags: feedback?(jsantell)
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?
Attachment #8569380 - Flags: feedback?(vporof) → feedback+
(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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attached patch timeline-range.patch (obsolete) — Splinter Review
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
Attachment #8569380 - Attachment is obsolete: true
Attachment #8578333 - Flags: review?(vporof)
Blocks: perf-tool-v2
No longer blocks: enable-perf-tool
Comment on attachment 8578333 [details] [diff] [review]
timeline-range.patch

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

ship it
Attachment #8578333 - Flags: review?(vporof) → review+
Blocks: 1144779
Blocks: 1144799
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.
Attachment #8578333 - Flags: review+ → review-
(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?
Flags: needinfo?(vporof)
Attached image screen.gif
Just move the mouse between graphs
Flags: needinfo?(vporof)
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).
Attachment #8578333 - Attachment is obsolete: true
Attachment #8579589 - Flags: review+
Comment on attachment 8579589 [details] [diff] [review]
timeline-range.patch

Meant to r?
Attachment #8579589 - Flags: review+ → review?(vporof)
Moving into the Profiler component. Filter on GUTHRIE'S WAVY CAKES.
Component: Developer Tools: Timeline → Developer Tools: Profiler
OS: Linux → All
Hardware: x86_64 → All
Duplicate of this bug: 1095454
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.
Attachment #8579589 - Flags: review?(vporof)
Depends on: 1145784
Blocks: 1145784
No longer depends on: 1145784
(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 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
Attachment #8579589 - Flags: review+
Keywords: checkin-needed
ship it!
https://hg.mozilla.org/integration/fx-team/rev/e6349d80d3b5
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e6349d80d3b5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.