Last Comment Bug 675015 - Suppress synthetic mouse events due to scrolling until the scroll is complete
: Suppress synthetic mouse events due to scrolling until the scroll is complete
Status: RESOLVED FIXED
[Snappy]
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla12
Assigned To: Timothy Nikkel (:tnikkel)
:
:
Mentors:
: 713313 715584 (view as bug list)
Depends on: 712478
Blocks: 675866 710372 722095 705174
  Show dependency treegraph
 
Reported: 2011-07-28 13:01 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2015-03-22 12:20 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (5.24 KB, patch)
2012-01-17 02:18 PST, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
patch (4.42 KB, patch)
2012-01-18 19:51 PST, Timothy Nikkel (:tnikkel)
roc: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-07-28 13:01:54 PDT
shaver ran into us updating hover state on twitter as we scroll; it might make sense to suppress the synth mouse events until after the scrolling is done.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-31 22:44:10 PDT
You mean until the end of a smooth scroll operation, or something else?
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-08-01 08:10:13 PDT
There are several situations where I think we should do this:

1) Smooth scroll.
2) Scrolling with momentum on Mac (even with smoothscroll disabled this happens).
3) User hasn't released the mouse button yet while dragging the thumb (not sure.
   whether we can get :hover states here).
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-01 16:09:00 PDT
Those all sound reasonable.
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 11:00:59 PST
I know it's rather late in the cycle, any chance on getting a fix for this in time for Firefox 9.0?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-12-08 13:09:56 PST
Absolutely not (you're talking less than 36 hours turnaround!).  Nor for 10, or probably 11.  This would be a nontrivial and somewhat risky change.
Comment 6 Luke Wagner [:luke] 2011-12-24 16:31:22 PST
*** Bug 713313 has been marked as a duplicate of this bug. ***
Comment 7 :Ehsan Akhgari 2012-01-09 16:22:02 PST
*** Bug 715584 has been marked as a duplicate of this bug. ***
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-01-12 14:11:20 PST
So one option is to just record the widget-relative position in addition to the presshell-relative mMouseLocation.  And then we can check whether it's changed while our event was up, and if it has we wait on dispatching the event...  That way if scroll positions change fast enough we wouldn't fire the mousemove until they stop changing.  Might help with smoothscroll and maybe momentum scrolling if it happens faster than 60Hz.  Probably not with the "mouse down on scrollbar and drag" thing, though: users don't move that fast, right?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-12 17:06:34 PST
(In reply to Boris Zbarsky (:bz) from comment #8)
> And then we can check whether it's
> changed while our event was up, and if it has we wait on dispatching the
> event...

I don't quite follow this part.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-01-12 17:21:06 PST
I'm thinking something along these lines:

1) When we first post the "do the synth mouse move" event, record the widget-relative coordinates that its presshell-relative coordinates correspond to.

2) When the refresh driver ticks, convert the presshell-relative coordinates to widget-relative ones and compare to the old widget-relative coordinates.  If they're equal, do the synth mouse move.  Otherwise store the new widget-relative coordinates and wait for the next tick.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-12 17:53:12 PST
How does that help? even when scrolling, those coordinates are the same.

When we come to dispatch the synthetic mouse move, we could check the ancestors of the target frame to see if any of them are actively scrolling, and if they are, don't dispatch the event, defer to the next tick. The viewport root is a tricky one since it's treated as always actively scrolling ... you'd have to modify nsGfxScrollFrameInner::MarkInactive/MarkActive to record scrolling activity but not use it.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-01-12 18:48:15 PST
> How does that help? even when scrolling, those coordinates are the same.

Oh, hmm.  I guess now that scrollframes have no widgets that's true.  I guess we could do a hit-test and see whether the frame under the mouse has stabilized or something, but that sounds like it'd still be a good bit of CPU usage.  Better than now, though.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-12 20:11:05 PST
I think the second paragraph of comment #11 is quite feasible.
Comment 14 Timothy Nikkel (:tnikkel) 2012-01-13 02:20:49 PST
Yeah, I can take this.
Comment 15 Timothy Nikkel (:tnikkel) 2012-01-15 17:02:51 PST
Ah, except scroll frames are active for 3-4 seconds after the last activity, that's too long to wait for hover to be updated.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-16 16:36:58 PST
Hmm, true. Maybe a flag on the refreshdriver instead to indicate whether a scrolling operation occurred since the last refresh tick?
Comment 17 Timothy Nikkel (:tnikkel) 2012-01-16 17:21:50 PST
I think that users scroll at much less than 60 scrolls a second, so we'll still end up dispatching synth mouse moves while scrolling, just one tick later.

I have a patch that kicks off a timer when we scroll, if we get another scroll before the timer fires we restart the timer when it fires, this seems to work well.
Comment 18 Timothy Nikkel (:tnikkel) 2012-01-17 02:18:08 PST
Created attachment 589133 [details] [diff] [review]
wip

How do you feel about something like this?
Comment 19 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-17 14:16:24 PST
Comment on attachment 589133 [details] [diff] [review]
wip

I was just thinking about something like this. This way we could reduce
the number of synthetic mouse events quite a bit.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-17 15:55:14 PST
Comment on attachment 589133 [details] [diff] [review]
wip

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1828,5 @@
> +
> +  if (self->mScrollSinceLastTimerFire) {
> +    //restart the timer
> +    self->mScrollActivityTimer->InitWithFuncCallback(
> +          ScrollActivityCallback, self, 200, nsITimer::TYPE_ONE_SHOT);

The ideal behavior would be, instead of setting the flag, just cancel and reschedule the timer from ::SynthesizeMouseMoveWhenInactive. Why not do that?

@@ +1845,5 @@
> +{
> +  if (!mScrollActivityTimer) {
> +    mScrollActivityTimer = do_CreateInstance("@mozilla.org/timer;1");
> +    if (!mScrollActivityTimer) {
> +      mOuter->PresContext()->PresShell()->SynthesizeMouseMove(true);

Don't bother doing it sync, just return. This'll never happen anyway.

@@ +1887,5 @@
>  
>    // We pass in the amount to move visually
>    ScrollVisual(oldScrollFramePos);
>  
> +  SynthesizeMouseMoveWhenInactive();

Call this ScheduleSyntheticMouseMove?

::: layout/generic/nsGfxScrollFrame.h
@@ +335,5 @@
>    // If true, the layer should always be active because we always build a layer.
>    // Used for asynchronous scrolling.
>    bool mShouldBuildLayer:1;
> +  
> +  bool mScrollSinceLastTimerFire:1;

Document
Comment 21 Timothy Nikkel (:tnikkel) 2012-01-18 01:33:20 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> The ideal behavior would be, instead of setting the flag, just cancel and
> reschedule the timer from ::SynthesizeMouseMoveWhenInactive. Why not do that?

I can do that. I avoided that because I thought rescheduling the timer might be expensive.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-18 18:01:08 PST
It'll probably be fine.
Comment 23 Timothy Nikkel (:tnikkel) 2012-01-18 19:51:15 PST
Created attachment 589761 [details] [diff] [review]
patch

Ok.

Addressed comments.

I also changed it to a 100ms timer (from 200ms). This feels snappier.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-18 20:08:35 PST
Comment on attachment 589761 [details] [diff] [review]
patch

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

r+ with that

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1827,5 @@
> +nsGfxScrollFrameInner::ScrollActivityCallback(nsITimer *aTimer, void* anInstance)
> +{
> +  nsGfxScrollFrameInner* self = static_cast<nsGfxScrollFrameInner*>(anInstance);
> +  if (!self)
> +    return;

How can this be null? remove this check.

@@ +1830,5 @@
> +  if (!self)
> +    return;
> +
> +  // Fire the synth mouse move.
> +  self->mScrollActivityTimer->Cancel();

You shouldn't need to cancel since it's just one-shot. Remove this.
Comment 25 Timothy Nikkel (:tnikkel) 2012-01-18 20:14:25 PST
Made those changes and pushed to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c6be1f2db4
Comment 26 Phil Ringnalda (:philor) 2012-01-18 22:48:30 PST
This intermittently added an extra assertion to 467493-1.html and 467493-2.html, but I was already thinking about throwing up my hands and adjusting the numbers for bug 718564, where someone intermittently took one away on OS X, so I just adjusted both ways in https://hg.mozilla.org/integration/mozilla-inbound/rev/945b24b3522d and https://hg.mozilla.org/integration/mozilla-inbound/rev/51dd2c0a4c73
Comment 27 Matt Brubeck (:mbrubeck) 2012-01-19 10:48:56 PST
https://hg.mozilla.org/mozilla-central/rev/d4c6be1f2db4
Comment 28 Marat Tanalin | tanalin.com 2012-01-28 14:12:20 PST
Is it disableable (e.g. via an about:config pref)?

I personally did very like previous behavior of Firefox as for hovering during scrolling and did consider it as _benefit_ over other browsers, not as something that needs to be "fixed" at all.
Comment 29 Timothy Nikkel (:tnikkel) 2012-01-28 14:14:34 PST
The only way to disable it as is is to edit C++ and make your own build.
Comment 30 Marat Tanalin | tanalin.com 2012-01-28 14:15:31 PST
Could the pref be implemented then? Thanks.
Comment 31 Timothy Nikkel (:tnikkel) 2012-01-28 14:18:31 PST
You could file a new bug. I don't know if there is enough demand for a pref.
Comment 32 Marat Tanalin | tanalin.com 2012-01-28 15:00:34 PST
(In reply to comment #31)

Filed bug 722095.
Comment 33 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-31 07:34:54 PST
Scrolling feel a lot better now. Thanks!

[17:28]	smaug	has something changed in scroll handling
[17:29]	smaug	it is so smooth
[17:30]	smaug	perhaps tn landed his synthetic mouse event handling change
[17:33]	jaws	smaug: yeah, tn fixed bug 675015 recently

Note You need to log in before you can comment on or make changes to this bug.