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 Events | Assignee: | 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: |
|
(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. 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. I think starting with a 50ms throttle for hover state updates would be a great first attempt. 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. (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. That said, I'm definitely supportive of firing fewer mouse events and doing fewer hover state calculations while the user is scrolling. Created attachment 170072 [details]
WIP
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 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 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 (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. 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. Created attachment 170262 [details]
Patch
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 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 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? (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. Created attachment 170285 [details]
Patch
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 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 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 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 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. Created attachment 172422 [details]
Patch
Created attachment 172424 [details]
Patch
> 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 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 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 Created attachment 173386 [details]
Patch
Comment on attachment 173386 [details]
Patch
Lots of seeming flake on the EWS bots, so I'd watch this going on. Otherwise, LGTM.
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 on attachment 173386 [details] Patch Clearing flags on attachment: 173386 Committed r134150: <http://trac.webkit.org/changeset/134150> All reviewed patches have been landed. Closing bug. 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. (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. 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 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. 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? 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> 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. 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 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 ? Fixed in r183198. |
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.