Bug 99940

Summary: Should only fire a single set of mouse events and update hover state once when scrolling is done
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: UI EventsAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, andersca, anilsson, ap, arv, bdakin, davemoore, dbates, dglazkov, eae, efidler, eric, esprehn, gmak, jamesr, jonlee, kling, koivisto, leviw, mlattanzio, nduca, simon.fraser, tabatkins, tasak, tonikitoo, tony, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 99669    
Attachments:
Description Flags
test case
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review+

Description Ojan Vafai 2012-10-21 10:51:13 PDT
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.
Comment 1 Eric Seidel (no email) 2012-10-21 10:57:17 PDT
(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.
Comment 2 Ojan Vafai 2012-10-21 11:06:54 PDT
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.
Comment 3 Eric Seidel (no email) 2012-10-21 11:22:54 PDT
I think starting with a 50ms throttle for hover state updates would be a great first attempt.
Comment 4 Ojan Vafai 2012-10-21 11:31:15 PDT
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.
Comment 5 James Robinson 2012-10-22 20:32:06 PDT
(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.
Comment 6 James Robinson 2012-10-22 20:32:38 PDT
That said, I'm definitely supportive of firing fewer mouse events and doing fewer hover state calculations while the user is scrolling.
Comment 7 Takashi Sakamoto 2012-10-23 00:32:22 PDT
Created attachment 170072 [details]
WIP
Comment 8 Takashi Sakamoto 2012-10-23 00:40:29 PDT
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>.)
Comment 9 WebKit Review Bot 2012-10-23 05:42:59 PDT
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
Comment 10 Build Bot 2012-10-23 06:35:56 PDT
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
Comment 11 Ojan Vafai 2012-10-23 09:30:01 PDT
(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.
Comment 12 Ojan Vafai 2012-10-23 13:44:21 PDT
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.
Comment 13 Ojan Vafai 2012-10-23 16:16:49 PDT
Created attachment 170262 [details]
Patch
Comment 14 Ojan Vafai 2012-10-23 16:19:02 PDT
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.
Comment 15 Elliott Sprehn 2012-10-23 16:28:00 PDT
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?
Comment 16 Darin Adler 2012-10-23 16:42:47 PDT
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?
Comment 17 Ojan Vafai 2012-10-23 17:08:14 PDT
(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.
Comment 18 Ojan Vafai 2012-10-23 18:33:08 PDT
Created attachment 170285 [details]
Patch
Comment 19 Build Bot 2012-10-24 00:45:14 PDT
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
Comment 20 Alexey Proskuryakov 2012-10-24 08:49:00 PDT
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.
Comment 21 Ryosuke Niwa 2012-11-03 18:05:06 PDT
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.
Comment 22 Ojan Vafai 2012-11-05 11:31:53 PST
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.
Comment 23 Levi Weintraub 2012-11-05 15:04:55 PST
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.
Comment 24 Ojan Vafai 2012-11-05 15:49:20 PST
Created attachment 172422 [details]
Patch
Comment 25 Ojan Vafai 2012-11-05 15:51:18 PST
Created attachment 172424 [details]
Patch
Comment 26 Ojan Vafai 2012-11-05 15:53:03 PST
> 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.
Comment 27 Build Bot 2012-11-05 17:31:59 PST
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
Comment 28 Build Bot 2012-11-05 18:49:04 PST
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
Comment 29 Ojan Vafai 2012-11-09 15:43:27 PST
Created attachment 173386 [details]
Patch
Comment 30 Levi Weintraub 2012-11-09 18:12:12 PST
Comment on attachment 173386 [details]
Patch

Lots of seeming flake on the EWS bots, so I'd watch this going on. Otherwise, LGTM.
Comment 31 Build Bot 2012-11-09 18:38:03 PST
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
Comment 32 WebKit Review Bot 2012-11-09 21:16:31 PST
Comment on attachment 173386 [details]
Patch

Clearing flags on attachment: 173386

Committed r134150: <http://trac.webkit.org/changeset/134150>
Comment 33 WebKit Review Bot 2012-11-09 21:16:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 Beth Dakin 2012-11-13 21:43:00 PST
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.
Comment 35 Beth Dakin 2012-11-13 21:43:53 PST
(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.
Comment 36 Dimitri Glazkov (Google) 2012-11-14 13:21:08 PST
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
Comment 37 Simon Fraser (smfr) 2012-12-05 16:43:29 PST
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.
Comment 38 Ojan Vafai 2012-12-05 16:51:06 PST
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?
Comment 39 Ojan Vafai 2012-12-10 15:35:26 PST
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>
Comment 40 Ojan Vafai 2012-12-10 15:39:51 PST
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.
Comment 41 Radar WebKit Bug Importer 2013-06-24 16:58:08 PDT
<rdar://problem/14254013>
Comment 42 Dave Hyatt 2015-04-23 10:09:40 PDT
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.
Comment 43 Simon Fraser (smfr) 2015-04-23 10:16:22 PDT
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 ?
Comment 44 Dave Hyatt 2015-04-23 11:42:34 PDT
Fixed in r183198.