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.

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.

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.

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.

How do you feel about something like this?

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.

It'll probably be fine.

Ok. Addressed comments. I also changed it to a 100ms timer (from 200ms). This feels snappier.

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.

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

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.

(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