Closed Bug 675015 Opened 9 years ago Closed 9 years ago

Suppress synthetic mouse events due to scrolling until the scroll is complete

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bzbarsky, Assigned: tnikkel)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: perf, Whiteboard: [Snappy])

Attachments

(1 file, 1 obsolete file)

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.
You mean until the end of a smooth scroll operation, or something else?
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).
Keywords: perf
OS: Mac OS X → All
Hardware: x86 → All
Summary: Suppress synth mouse events due to scrolling until the scroll is complete → Suppress synthetic mouse events due to scrolling until the scroll is complete
Blocks: 705174
I know it's rather late in the cycle, any chance on getting a fix for this in time for Firefox 9.0?
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.
Duplicate of this bug: 713313
Whiteboard: [Snappy]
No longer blocks: 198964
Duplicate of this bug: 715584
Depends on: 712478
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?
(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.
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.
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.
> 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.
I think the second paragraph of comment #11 is quite feasible.
Yeah, I can take this.
Assignee: nobody → tnikkel
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.
Hmm, true. Maybe a flag on the refreshdriver instead to indicate whether a scrolling operation occurred since the last refresh tick?
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.
Attached patch wip (obsolete) — Splinter Review
How do you feel about something like this?
Attachment #589133 - Flags: feedback?(roc)
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 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
(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.
Attached patch patchSplinter Review
Ok.

Addressed comments.

I also changed it to a 100ms timer (from 200ms). This feels snappier.
Attachment #589133 - Attachment is obsolete: true
Attachment #589133 - Flags: feedback?(roc)
Attachment #589761 - Flags: review?(roc)
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.
Attachment #589761 - Flags: review?(roc) → review+
Made those changes and pushed to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c6be1f2db4
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
https://hg.mozilla.org/mozilla-central/rev/d4c6be1f2db4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
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.
The only way to disable it as is is to edit C++ and make your own build.
Could the pref be implemented then? Thanks.
You could file a new bug. I don't know if there is enough demand for a pref.
Blocks: 722095
(In reply to comment #31)

Filed bug 722095.
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
You need to log in before you can comment on or make changes to this bug.