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

RESOLVED FIXED in mozilla12

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: bz, Assigned: tnikkel)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {perf})

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 1 obsolete attachment)

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).
Those all sound reasonable.
Keywords: perf

Updated

6 years ago
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.
Blocks: 675866

Updated

6 years ago
Duplicate of this bug: 713313

Updated

6 years ago
Whiteboard: [Snappy]
Blocks: 198964
No longer blocks: 198964
Blocks: 710372
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.
(Assignee)

Comment 14

5 years ago
Yeah, I can take this.
Assignee: nobody → tnikkel
(Assignee)

Comment 15

5 years ago
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?
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Comment 18

5 years ago
Created attachment 589133 [details] [diff] [review]
wip

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
(Assignee)

Comment 21

5 years ago
(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.
It'll probably be fine.
(Assignee)

Comment 23

5 years ago
Created attachment 589761 [details] [diff] [review]
patch

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+
(Assignee)

Comment 25

5 years ago
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
Last Resolved: 5 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.
(Assignee)

Comment 29

5 years ago
The only way to disable it as is is to edit C++ and make your own build.
Could the pref be implemented then? Thanks.
(Assignee)

Comment 31

5 years ago
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.