Bug 99940 – Should only fire a single set of mouse events and update hover state once when scrolling is done

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Ojan Vafai]

Created attachment 169808 [details]
test case

RIght now we fire them continuously. Gecko does not. On some pages this gives them much better performance (see blocked bug). Also, IMO, the user-experience in Gecko is nicer, although that's certainly arguable.

Perhaps the ideal solution would be to pay attention to scrolling speed and only update hover state and mouse events when the scrolling is below a certain speed.

[Eric Seidel (no email)]

(In reply to comment #0)
> Perhaps the ideal solution would be to pay attention to scrolling speed and only update hover state and mouse events when the scrolling is below a certain speed.

It sounds like we could do that just by setting a throttle on hover events.  I don't know how good/bad of a UX that would be w/o trying.

[Ojan Vafai]

Looking at EventHandler.cpp it looks like we already do some throttling, but it's mostly limited to mousemove events, not all mouse events and not hover state. My intuition is that the delay we'd need would be too long for a good user-experience, but if ~50ms is enough, maybe it's fine.

[Eric Seidel (no email)]

I think starting with a 50ms throttle for hover state updates would be a great first attempt.

[Ojan Vafai]

James/Nat, listening to mouse events currently disables threaded scrolling, right?

Thinking about this more, I kind of like the idea of not firing them at all until the scroll is done. That appears to be what Gecko does, although I haven't looked at their source to confirm. What I like about this approach is that it would mean that mouse event handlers wouldn't need to disable threaded scrolling.

[James Robinson]

(In reply to comment #4)
> James/Nat, listening to mouse events currently disables threaded scrolling, right?

Listening to mousewheel events prevents handling mousewheels as scrolls off of the main thread.  I don't think that's what this bug is about, though.

> Thinking about this more, I kind of like the idea of not firing them at all until the scroll is done. That appears to be what Gecko does, although I haven't looked at their source to confirm. What I like about this approach is that it would mean that mouse event handlers wouldn't need to disable threaded scrolling.

I don't think this will have any impact one way or the other on threaded scrolling.  The only thing that's relevant for threaded scrolling are input events with a default action of scrolling that can be cancelled.

[James Robinson]

That said, I'm definitely supportive of firing fewer mouse events and doing fewer hover state calculations while the user is scrolling.

[Takashi Sakamoto]

Created attachment 170072 [details]
WIP

[Takashi Sakamoto]

I looked at testcase by using Firefox. I think, Firefox seems to fire mouseover/mouseout events when mouse cursor is actually moved.
So scrolling testcase.html by using only mouse wheel didn't cause Firefox to fire mouse events.
(Except the case that the page has any <iframe>.)

[WebKit Review Bot]

Comment on attachment 170072 [details]
WIP

Attachment 170072 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14489926

New failing tests:
fast/events/frame-scroll-fake-mouse-move.html
fast/events/overflow-scroll-fake-mouse-move.html

[Build Bot]

Comment on attachment 170072 [details]
WIP

Attachment 170072 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14488860

New failing tests:
fast/events/frame-scroll-fake-mouse-move.html
fast/events/overflow-scroll-fake-mouse-move.html

[Ojan Vafai]

(In reply to comment #8)
> I looked at testcase by using Firefox. I think, Firefox seems to fire mouseover/mouseout events when mouse cursor is actually moved.
> So scrolling testcase.html by using only mouse wheel didn't cause Firefox to fire mouse events.
> (Except the case that the page has any <iframe>.)

That's not the behavior I'm seeing in the test case on this bug on Firefox 16 on Ubuntu. Scrolling with just the scroll wheel or just the keyboard arrow keys does fire mouse events and update hover state, but only once at the end of the scroll.

[Ojan Vafai]

We already try to throttle. We just don't do a great job of it. Takashi-san, I hope you don't mind if I take this over. I basically have the patch to fix this locally. I'm just optimizing it some now.

[Ojan Vafai]

Created attachment 170262 [details]
Patch

[Ojan Vafai]

This works *much* better. This makes the page in 99669 scroll smoothly as long as you don't move the mouse. Once you actually move the mouse, the hover/layout/hittest/something else takes so long to run that it's janky again. The point is that once you're scrolling, the scroll doesn't fire events after this patch until you're done scrolling.

[Elliott Sprehn]

Comment on attachment 170262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170262&action=review

> Source/WebCore/page/EventHandler.cpp:162
> +    explicit RunningAverageDurationTracker(double *avgDuration, unsigned numberOfRunsToTrack)

You don't need the explicit. There's 2 args.

> Source/WebCore/page/EventHandler.cpp:180
> +    double* m_avgDuration;

I think m_average would be nicer

> Source/WebCore/page/EventHandler.cpp:2821
> +        m_fakeMouseMoveEventTimer.setDelay(m_mouseMovedDurationRunningAverage + .05);

How'd you pick .05?

> Source/WebCore/platform/Timer.h:144
> +    double delay() { return m_delay; }

const

> LayoutTests/fast/scrolling/fake-mouse-event-throttling.html:15
> +<body onload="runNextTest()" style="overflow: scroll">

body { overflow: scroll; } in the style?

[Darin Adler]

Comment on attachment 170262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170262&action=review

Are we seeing this same phenomenon with WebKit2 scrolling? What effect does this patch have with WebKit2?

>> Source/WebCore/page/EventHandler.cpp:162
>> +    explicit RunningAverageDurationTracker(double *avgDuration, unsigned numberOfRunsToTrack)
> 
> You don't need the explicit. There's 2 args.

The * here is in the wrong place.

>> Source/WebCore/page/EventHandler.cpp:180
>> +    double* m_avgDuration;
> 
> I think m_average would be nicer

I agree, no reason to abbreviate to avg.

>> Source/WebCore/page/EventHandler.cpp:2821
>> +        m_fakeMouseMoveEventTimer.setDelay(m_mouseMovedDurationRunningAverage + .05);
> 
> How'd you pick .05?

Normally it’s best to have a named constant just there is a place to put the comment explaining the source of the number.

> Source/WebCore/platform/Timer.h:143
> +    void setDelay(double delay) { m_delay = delay; }

Is it correct that setting a delay on an already-existing timer has no effect until the next restart?

[Ojan Vafai]

(In reply to comment #16)
> (From update of attachment 170262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170262&action=review
> 
> Are we seeing this same phenomenon with WebKit2 scrolling?

Yes. Today's WebKit nightly gets the same behavior as Chrome.

> What effect does this patch have with WebKit2?

I didn't test a WebKit2 build, but this is all cross-platform code. Note that this has no effect on real platform mouse events. This only has an effect on code that calls FrameView::scrollPositionChanged, so I expect the patch to not have significant platform-specific differences.

> >> Source/WebCore/page/EventHandler.cpp:2821
> >> +        m_fakeMouseMoveEventTimer.setDelay(m_mouseMovedDurationRunningAverage + .05);
> > 
> > How'd you pick .05?
> 
> Normally it’s best to have a named constant just there is a place to put the comment explaining the source of the number.

Whoops. Missed this one. Meant to do that. I picked .05 fairly arbitrarily (raising by 50ms seems reasonable to me).

> > Source/WebCore/platform/Timer.h:143
> > +    void setDelay(double delay) { m_delay = delay; }
> 
> Is it correct that setting a delay on an already-existing timer has no effect until the next restart?

I went back and forth on this. Now that I think about it more...I think it's probably not right. I'll stop the timer and restart it with the new delay if it's active.

[Ojan Vafai]

Created attachment 170285 [details]
Patch

[Build Bot]

Comment on attachment 170285 [details]
Patch

Attachment 170285 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14554112

New failing tests:
fast/scrolling/fake-mouse-event-throttling.html

[Alexey Proskuryakov]

Perhaps it would make sense to e-mail webkit-dev about this. There are many people CC'ed on this bug, but not everyone who could have an informed opinion.

[Ryosuke Niwa]

Comment on attachment 170285 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170285&action=review

> Source/WebCore/ChangeLog:12
> +        mouse events take and adjust thr throttling appropriately.

Typo: thr.

> Source/WebCore/page/EventHandler.cpp:144
> +const double fakeMouseMoveRunningAverageCount = 10;

Why don't we add a static variable in RunningAverageDurationTracker instead of picking an arbitrary valu.

> Source/WebCore/page/EventHandler.cpp:186
> +    double* m_average;

Why don't we use a reference instead?

> Source/WebCore/page/EventHandler.cpp:2830
> +    m_fakeMouseMoveEventTimer.restart();

Can the logic to compute new delay be extracted as a function? Maybe we can rename RunningAverageDurationTracker and make this a static function there.

[Ojan Vafai]

Comment on attachment 170285 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170285&action=review

>> Source/WebCore/page/EventHandler.cpp:144
>> +const double fakeMouseMoveRunningAverageCount = 10;
> 
> Why don't we add a static variable in RunningAverageDurationTracker instead of picking an arbitrary valu.

We'll still be picking an arbitrary value. We just then won't be passing it in. Not sure what the advantage there is. Keeping RunningAverageDurationTracker as simple and generic as possible seems cleaner to me.

>> Source/WebCore/page/EventHandler.cpp:186
>> +    double* m_average;
> 
> Why don't we use a reference instead?

I was just doing what the old code did. How does using a reference make this better? The point is that we're modifying a member variable on EventHandler.

>> Source/WebCore/page/EventHandler.cpp:2830
>> +    m_fakeMouseMoveEventTimer.restart();
> 
> Can the logic to compute new delay be extracted as a function? Maybe we can rename RunningAverageDurationTracker and make this a static function there.

It could, but that's really all this function does. I'm not really sure how that would improve this code.

[Levi Weintraub]

Comment on attachment 170285 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170285&action=review

This seems reasonable to me. Any idea why the test failed on Mac EWS?

> Source/WebCore/page/EventHandler.cpp:1836
> +

Seems like an odd, random space :)

>>> Source/WebCore/page/EventHandler.cpp:2830
>>> +    m_fakeMouseMoveEventTimer.restart();
>> 
>> Can the logic to compute new delay be extracted as a function? Maybe we can rename RunningAverageDurationTracker and make this a static function there.
> 
> It could, but that's really all this function does. I'm not really sure how that would improve this code.

Perhaps Ryosuke is imagining there being other heuristics to choose from? If another is implemented, that could make sense. In the meantime this seems reasonable to me.

[Ojan Vafai]

Created attachment 172422 [details]
Patch

[Ojan Vafai]

Created attachment 172424 [details]
Patch

[Ojan Vafai]

> This seems reasonable to me. Any idea why the test failed on Mac EWS?

Whoops. Didn't notice that. Hopefully we'll link to the right output results this time and I can see if it's timing out or not. Not really sure what would be going wrong here.

[Build Bot]

Comment on attachment 172424 [details]
Patch

Attachment 172424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14747187

New failing tests:
fast/scrolling/fake-mouse-event-throttling.html

[Build Bot]

Comment on attachment 172424 [details]
Patch

Attachment 172424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14747204

New failing tests:
fast/scrolling/fake-mouse-event-throttling.html

[Ojan Vafai]

Created attachment 173386 [details]
Patch

[Levi Weintraub]

Comment on attachment 173386 [details]
Patch

Lots of seeming flake on the EWS bots, so I'd watch this going on. Otherwise, LGTM.

[Build Bot]

Comment on attachment 173386 [details]
Patch

Attachment 173386 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14770851

New failing tests:
http/tests/inspector/stacktraces/csp-injected-content-warning-contains-stacktrace.html

[WebKit Review Bot]

Comment on attachment 173386 [details]
Patch

Clearing flags on attachment: 173386

Committed r134150: <http://trac.webkit.org/changeset/134150>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Beth Dakin]

I noticed that after this patch, if you hover the mouse over a comment field long enough for a tooltip to appear and then you scroll, the tooltip will say up until you stop scrolling. That's unfortunate.

[Beth Dakin]

(In reply to comment #34)
> I noticed that after this patch, if you hover the mouse over a comment field long enough for a tooltip to appear and then you scroll, the tooltip will say up until you stop scrolling. That's unfortunate.

Ahem, sorry, "hover the mouse over a comment field IN FACEBOOK," is what I meant to say. Probably applies to all tooltips though.

[Dimitri Glazkov (Google)]

It looks like the patch broke fast/events/overflow-scroll-fake-mouse-move.html and the newly added fast/events/frame-scroll-fake-mouse-move.html is failing...

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fevents%2Fframe-scroll-fake-mouse-move.html%2Cfast%2Fevents%2Foverflow-scroll-fake-mouse-move.html

[Simon Fraser (smfr)]

Ojan, could you address the issues raised in the last two comments? In particular, the issue about tooltips sticking around when scrolling on pages like Facebook is particularly bad, and needs to be addressed.

[Ojan Vafai]

Sorry I haven't responded yet. It's been somewhere near the top of my todo list. :(

Anyways, I'm not sure I'll get to fixing this right away, so a rollout might be the best option in the short-term. The facebook UI actually looks fine IMO since the tooltip scrolls along with the page. I don't really see a problem with it.

On the other hand, the tooltips in Gmail look less fine since they're position fixed and actually point to the wrong item when you scroll.

FWIW, this is what Firefox already does. There's is a little less aggressive though.

Hopefully I'll get around to this in the next few days. What I want to try is to get rid of the bit where we increase the throttling delay and always use 100ms. This matches the Firefox behavior. The open question is whether the Firefox behavior is better or worse than our old behavior. I suppose I should solicit opinions on webkit-dev?

[Ojan Vafai]

Reverted r134150 for reason:

Caused JS-based tooltips to remain during scroll on Facebook and Gmail. Rollout until I have time to experiment with less aggresive approaches.

Committed r137214: <http://trac.webkit.org/changeset/137214>

[Ojan Vafai]

I likely won't get to this in the next few weeks/months, in case anyone else wants to experiment. Specifically, I'd like to try the idea in comment 38. The key is to test this on something with trackpad-based scrolling (e.g. a mac laptop or a chromebook). Using a scroll-wheel as enough pauses that you don't notice the issues on Facebook/gmail.

[Radar WebKit Bug Importer]

<rdar://problem/14254013>

[Dave Hyatt]

Created attachment 251450 [details]
Patch

Here is my cut at this. It seems to work fine with Facebook tooltips, and Gmail tooltips already stick around during scrolling even today, and that doesn't change.

[Simon Fraser (smfr)]

Comment on attachment 251450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251450&action=review

> Source/WebCore/ChangeLog:11
> +        complete. This has the effect of preventing fake mouse moveds from firing until the scroll stops.

mouse moves?

> Source/WebCore/page/EventHandler.cpp:2918
> +    m_fakeMouseMoveEventTimer.startOneShot(m_maxMouseMovedDuration > fakeMouseMoveDurationThreshold ?fakeMouseMoveLongInterval : fakeMouseMoveShortInterval);

Space after ?

[Dave Hyatt]

Fixed in r183198.