155746 – Wheel events' latching state is not reset when appropriate

RESOLVED FIXED 155746

Wheel events' latching state is not reset when appropriate

https://bugs.webkit.org/show_bug.cgi?id=155746

Summary Wheel events' latching state is not reset when appropriate

Antonio Gomes

Reported 2016-03-22 06:26:30 PDT

(With latest Safari nightly:) 1) Load http://jsbin.com/sizosozexa/edit?html,output (note two listbox'es (<select>s) in the rightmost frame) 2) Try to alternate scrolling between both listbox'es with wheel events (with two fingers scroll / trackpad) Expected: - The listbox scrolled is the one underneath the mouse cursor. Actual: - It happens sometimes that the wheel events latching logic, sticks with a given listbox as ScrollableContainer, regardless of where the cursor is when user starts scrolling (with two fingers scroll / trackpad).

Attachments
Patch for EWS. (1.91 KB, patch) 2016-03-22 06:30 PDT, Antonio Gomes	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (786.35 KB, application/zip) 2016-03-22 07:18 PDT, Build Bot	no flags	Details
Patch for EWS. (8.35 KB, patch) 2016-03-24 07:42 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Patch v1 (18.18 KB, patch) 2016-03-26 06:49 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Patch v1.1 (19.59 KB, patch) 2016-03-26 07:48 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Patch v1.2 (20.74 KB, patch) 2016-03-27 21:16 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Patch v2 - addressed Dan's review. (18.75 KB, patch) 2016-03-28 11:00 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Patch v2.1 - addressed Dan's review + rebased (18.68 KB, patch) 2016-03-28 16:34 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Patch v2.2 - addressed Simon's comments. (18.58 KB, patch) 2016-03-28 17:54 PDT, Antonio Gomes	simon.fraser: review+	Details Formatted Diff Diff
Patch for la (18.59 KB, patch) 2016-03-29 14:25 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Antonio Gomes

Comment 1 2016-03-22 06:30:26 PDT

Created attachment 274647 [details] Patch for EWS.

WebKit Commit Bot

Comment 2 2016-03-22 06:32:53 PDT

Attachment 274647 [details] did not pass style-queue: ERROR: Source/WebCore/platform/PlatformWheelEvent.h:211: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 3 2016-03-22 07:18:02 PDT

Comment on attachment 274647 [details] Patch for EWS. Attachment 274647 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1020998 New failing tests: tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html

Build Bot

Comment 4 2016-03-22 07:18:06 PDT

Created attachment 274651 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Antonio Gomes

Comment 5 2016-03-24 07:42:24 PDT

Created attachment 274833 [details] Patch for EWS.

WebKit Commit Bot

Comment 6 2016-03-24 07:43:42 PDT

Attachment 274833 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:34: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Antonio Gomes

Comment 7 2016-03-26 06:49:07 PDT

Created attachment 274984 [details] Patch v1

WebKit Commit Bot

Comment 8 2016-03-26 06:51:21 PDT

Attachment 274984 [details] did not pass style-queue: ERROR: Source/WebCore/page/mac/EventHandlerMac.mm:961: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/page/mac/EventHandlerMac.mm:970: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Antonio Gomes

Comment 9 2016-03-26 07:48:50 PDT

Created attachment 274985 [details] Patch v1.1

Antonio Gomes

Comment 10 2016-03-27 21:16:01 PDT

Created attachment 275008 [details] Patch v1.2 Patch ready for review.

Antonio Gomes

Comment 11 2016-03-27 21:21:38 PDT

Please note that it sits on top of the patch in bug 155940.

Daniel Bates

Comment 12 2016-03-28 00:15:34 PDT

Comment on attachment 275008 [details] Patch v1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=275008&action=review I have not reviewed this for correctness I noticed some minor issues and had some questions. I am saddened that EventHandler::doOrScheduleClearLatchedStateIfNeeded needs to have access to internal state of MainFrame. I wish there was another way. > LayoutTests/tiled-drawing/scrolling/resources/selects_iframe.html:1 > +<!DOCTYPE html> This is minor. I know that you followed the naming convention of the file LayoutTests/tiled-drawing/scrolling/resources/select_iframe.html. Regardless, the name of this file is inconsistent with the name of the other files included in this patch as well as the naming convention we typically use for LayoutTests. In particular, we use dashes (-) instead of underscores (_) between words. > LayoutTests/tiled-drawing/scrolling/resources/selects_iframe.html:35 > +<html> > + <body> > + <h1>This is a Heading</h1> > + <p>This is a paragraph.</p> > + <select id="selectLeft" size="5" multiple="multiple" name="driver1[]"> > + <option value="" >(No Driver Filter)</option> > + <option value="drivername=''"></option> > + <option value="drivername='alex'">alex</option> > + <option value="drivername='marc'">marc</option> > + <option value="drivername='frank'">frank</option> > + <option value="drivername='james'">james</option> > + <option value="drivername='michael'">michael</option> > + <option value="drivername='john'">john</option> > + <option value="drivername='tony'">tony</option> > + <option value="drivername='mary'">mary</option> > + <option value="drivername='francis'">francis</option> > + <option value="drivername='joannie'">joannie</option> > + <option value="drivername='mike'">mike</option> > + </select> > + <select id="selectRight" size="5" multiple="multiple" name="driver1[]"> > + <option value="" >(No Driver Filter)</option> > + <option value="drivername=''"></option> > + <option value="drivername='alex'">alex</option> > + <option value="drivername='marc'">marc</option> > + <option value="drivername='frank'">frank</option> > + <option value="drivername='james'">james</option> > + <option value="drivername='michael'">michael</option> > + <option value="drivername='john'">john</option> > + <option value="drivername='tony'">tony</option> > + <option value="drivername='mary'">mary</option> > + <option value="drivername='francis'">francis</option> > + <option value="drivername='joannie'">joannie</option> > + <option value="drivername='mike'">mike</option> > + </select> Can we simplify this code and make the indentation consistent? > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:27 > + testRunner.notifyDone(); This is unnecessary given that you call finishJSTest() above. Notice that finishJSTest() calls testRunner.notifyDone(). > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:43 > + var startPosX = Math.round(selectRight.offsetLeft) + 10; > + var startPosY = Math.round(selectRight.offsetTop) + 10; > + eventSender.mouseMoveTo(startPosX, startPosY); > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'began', 'none'); > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none'); > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none'); > + eventSender.callAfterScrollingCompletes(checkForScroll); This is minor. I know that you are using a similar style for this code as in other tests. This file alternative between using single quoted string literals and double quoted string literals. I suggest that we pick one style and stick with it throughout this file for consistency. Even better, can we extract the common code into shared function that can be used by this test and other latched scrolling tests instead of duplicating similar code in each of these tests? > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:60 > + testRunner.dumpAsText(); This is unnecessary. Notice that js-test-pre.js does this for you. > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:61 > + testRunner.waitUntilDone(); It is more idiomatic to use "window.jsTestIsAsync = true" when using js-test-pre.js instead of "testRunner.waitUntilDone()". > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:78 > + } else { > + var messageLocation = document.getElementById('parent'); > + var message = document.createElement('div'); > + message.innerHTML = "<p>This test is better run under DumpRenderTree. To manually test it, place the mouse pointer<br/>" > + + "within one left <select> boundary, and then use the mouse wheel or a two-finger to scroll down the list, without momentum scrolling.<br/>" > + + "Do the same for the right <select>.<br/><br/>" > + + "The left <select> should not scroll when trying to scroll the right <select>.</p>"; > + messageLocation.appendChild(message); > + } I would have put this message in the call to description() (below): description("Tests that latched state logic does not get stuck scrolling a specific <select>. To manually run this test, place the mouse pointer<br>" + "within one left <select> boundary, and then use the mouse wheel or a two-finger to scroll down the list, without momentum scrolling.<br>" + "Do the same for the right <select>. The left <select> should not scroll when trying to scroll the right <select>."); Notice I changed some of the words and phrasing to make it read well and used HTML character references (e.g. < for '<'). Feel free to update the wording. > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:83 > + </iframe> Please remove this closing tag. The HTML attribute name is unnecessary. > Source/WebCore/page/MainFrame.h:87 > + friend void EventHandler::doOrScheduleClearLatchedStateIfNeeded(const PlatformWheelEvent&); > + Timer& resetLatchingStateIfNoMomentumWheelEventsStartTimer() { return m_resetLatchingStateIfNoMomentumWheelEventsStartTimer; } > + Timer m_resetLatchingStateIfNoMomentumWheelEventsStartTimer; :( Can we come up with a better way to do this so that we do not have to expose the internal state of MainFrame to EventHandler::doOrScheduleClearLatchedStateIfNeeded()? > Source/WebCore/page/mac/EventHandlerMac.mm:936 > +void EventHandler::doOrScheduleClearLatchedStateIfNeeded(const PlatformWheelEvent& event) Can we come up with a better name for this? Maybe clearOrScheduleClearingLatchedStateIfNeeded? (I am not happy with that name either).

Antonio Gomes

Comment 13 2016-03-28 11:00:58 PDT

Created attachment 275029 [details] Patch v2 - addressed Dan's review. > > LayoutTests/tiled-drawing/scrolling/resources/selects_iframe.html:1 > > +<!DOCTYPE html> > > This is minor. I know that you followed the naming convention of the file > LayoutTests/tiled-drawing/scrolling/resources/select_iframe.html. > Regardless, the name of this file is inconsistent with the name of the other > files included in this patch as well as the naming convention we typically > use for LayoutTests. In particular, we use dashes (-) instead of underscores > (_) between words. Fixed. Used '-' rather than "_" on the file name. > > LayoutTests/tiled-drawing/scrolling/resources/selects_iframe.html:35 > > +<html> > > + <body> > > + <h1>This is a Heading</h1> > > + <p>This is a paragraph.</p> > > + <select id="selectLeft" size="5" multiple="multiple" name="driver1[]"> > > + <option value="" >(No Driver Filter)</option> > > + <option value="drivername=''"></option> > > + <option value="drivername='alex'">alex</option> > > + <option value="drivername='marc'">marc</option> > > + <option value="drivername='frank'">frank</option> > > + <option value="drivername='james'">james</option> > > + <option value="drivername='michael'">michael</option> > > + <option value="drivername='john'">john</option> > > + <option value="drivername='tony'">tony</option> > > + <option value="drivername='mary'">mary</option> > > + <option value="drivername='francis'">francis</option> > > + <option value="drivername='joannie'">joannie</option> > > + <option value="drivername='mike'">mike</option> > > + </select> > > + <select id="selectRight" size="5" multiple="multiple" name="driver1[]"> > > + <option value="" >(No Driver Filter)</option> > > + <option value="drivername=''"></option> > > + <option value="drivername='alex'">alex</option> > > + <option value="drivername='marc'">marc</option> > > + <option value="drivername='frank'">frank</option> > > + <option value="drivername='james'">james</option> > > + <option value="drivername='michael'">michael</option> > > + <option value="drivername='john'">john</option> > > + <option value="drivername='tony'">tony</option> > > + <option value="drivername='mary'">mary</option> > > + <option value="drivername='francis'">francis</option> > > + <option value="drivername='joannie'">joannie</option> > > + <option value="drivername='mike'">mike</option> > > + </select> > > Can we simplify this code and make the indentation consistent? Done. > > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:27 > > + testRunner.notifyDone(); > > This is unnecessary given that you call finishJSTest() above. Notice that > finishJSTest() calls testRunner.notifyDone(). Done. Removed explicit call to notifyDone. > > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:43 > > + var startPosX = Math.round(selectRight.offsetLeft) + 10; > > + var startPosY = Math.round(selectRight.offsetTop) + 10; > > + eventSender.mouseMoveTo(startPosX, startPosY); > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'began', 'none'); > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none'); > > + eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none'); > > + eventSender.callAfterScrollingCompletes(checkForScroll); > > This is minor. I know that you are using a similar style for this code as in > other tests. This file alternative between using single quoted string > literals and double quoted string literals. I suggest that we pick one style > and stick with it throughout this file for consistency. Done. Changed to use double quoted everywhere. > Even better, can we extract the common code into shared function that can be > used by this test and other latched scrolling tests instead of duplicating > similar code in each of these tests? I thought of doing this at first too, but decided not go with it. As is, I think it is a bit more straightforward to understand what each step of the test does. > > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:60 > > + testRunner.dumpAsText(); > > This is unnecessary. Notice that js-test-pre.js does this for you. > > > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:61 > > + testRunner.waitUntilDone(); > > It is more idiomatic to use "window.jsTestIsAsync = true" when using > js-test-pre.js instead of "testRunner.waitUntilDone()". > Done. > > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:78 > > + } else { > > + var messageLocation = document.getElementById('parent'); > > + var message = document.createElement('div'); > > + message.innerHTML = "<p>This test is better run under DumpRenderTree. To manually test it, place the mouse pointer<br/>" > > + + "within one left <select> boundary, and then use the mouse wheel or a two-finger to scroll down the list, without momentum scrolling.<br/>" > > + + "Do the same for the right <select>.<br/><br/>" > > + + "The left <select> should not scroll when trying to scroll the right <select>.</p>"; > > + messageLocation.appendChild(message); > > + } > > I would have put this message in the call to description() (below): > > description("Tests that latched state logic does not get stuck scrolling a > specific <select>. To manually run this test, place the mouse > pointer<br>" + > "within one left <select> boundary, and then use the mouse > wheel or a two-finger to scroll down the list, without momentum > scrolling.<br>" + > "Do the same for the right <select>. The left > <select> should not scroll when trying to scroll the right > <select>."); > > Notice I changed some of the words and phrasing to make it read well and > used HTML character references (e.g. < for '<'). Feel free to update the > wording. Thanks, done as per your suggestion. > > LayoutTests/tiled-drawing/scrolling/scroll-iframe-latched-selects.html:83 > > + </iframe> > > Please remove this closing tag. The HTML attribute name is unnecessary. > > > Source/WebCore/page/MainFrame.h:87 > > + friend void EventHandler::doOrScheduleClearLatchedStateIfNeeded(const PlatformWheelEvent&); > > + Timer& resetLatchingStateIfNoMomentumWheelEventsStartTimer() { return m_resetLatchingStateIfNoMomentumWheelEventsStartTimer; } > > + Timer m_resetLatchingStateIfNoMomentumWheelEventsStartTimer; > > :( Can we come up with a better way to do this so that we do not have to > expose the internal state of MainFrame to > EventHandler::doOrScheduleClearLatchedStateIfNeeded()? I was not happy with it in MainFrame class either. I moved it to EventHandler, although the timer only get installed, if the instance if a main frame. MainFrame.cpp/h is untouched. > > Source/WebCore/page/mac/EventHandlerMac.mm:936 > > +void EventHandler::doOrScheduleClearLatchedStateIfNeeded(const PlatformWheelEvent& event) > > Can we come up with a better name for this? Maybe > clearOrScheduleClearingLatchedStateIfNeeded? (I am not happy with that name > either). Changed to clearOrScheduleClearingLatchedStateIfNeeded.

Antonio Gomes

Comment 14 2016-03-28 16:34:03 PDT

Created attachment 275065 [details] Patch v2.1 - addressed Dan's review + rebased

Simon Fraser (smfr)

Comment 15 2016-03-28 16:43:33 PDT

Comment on attachment 275065 [details] Patch v2.1 - addressed Dan's review + rebased View in context: https://bugs.webkit.org/attachment.cgi?id=275065&action=review > Source/WebCore/ChangeLog:11 > + a static scroll moviment (without the flicker animation). moviment => movement > Source/WebCore/ChangeLog:20 > + On the second scenario (static scroll), when the scroll moviment ends, movement > Source/WebCore/ChangeLog:21 > + the latched state is not resetted. This might make scrolling elsewhere on the page to scroll ...not reset. This might actually cause scrolling elsewhere on the page to scroll... > Source/WebCore/ChangeLog:26 > + In order to fix this, patch installs a timer as soon as a non momentum non-momentum > Source/WebCore/ChangeLog:27 > + wheel scroll event end fires. It works as following: as follows > Source/WebCore/ChangeLog:29 > + - If the timer times out, the latched state is resetted, avoid scrolling to get stuck to times out -> fires. ...reset, preventing scrolling getting stuck > Source/WebCore/ChangeLog:31 > + - If platform starts sending momentum wheel scroll events before it times out, the timer is platform code > Source/WebCore/page/EventHandler.h:494 > + Timer m_clearLatchedStateIfNoMomentumWheelEventsStartTimer; I think we can use a shorter name for this. Maybe m_pendingMomentumWheelEventsTimer > Source/WebCore/page/mac/EventHandlerMac.mm:951 > + m_frame.mainFrame().resetLatchingState(); So this call to resetLatchingState() is new, and happens when we've seen the last non-momentum event and we're under the 0.1s interval. Is it OK to reset at this point? > Source/WebCore/platform/PlatformWheelEvent.h:221 > + inline bool PlatformWheelEvent::isTransitioningToMomentumScrolling() const Call this isTransitioningToMomentumScroll() to match isEndOfNonMomentumScroll.

Antonio Gomes

Comment 16 2016-03-28 17:53:47 PDT

Comment on attachment 275065 [details] Patch v2.1 - addressed Dan's review + rebased View in context: https://bugs.webkit.org/attachment.cgi?id=275065&action=review >> Source/WebCore/ChangeLog:11 >> + a static scroll moviment (without the flicker animation). > > moviment => movement Done. >> Source/WebCore/ChangeLog:20 >> + On the second scenario (static scroll), when the scroll moviment ends, > > movement Done. >> Source/WebCore/ChangeLog:21 >> + the latched state is not resetted. This might make scrolling elsewhere on the page to scroll > > ...not reset. This might actually cause scrolling elsewhere on the page to scroll... Done. >> Source/WebCore/ChangeLog:26 >> + In order to fix this, patch installs a timer as soon as a non momentum > > non-momentum Done. >> Source/WebCore/ChangeLog:27 >> + wheel scroll event end fires. It works as following: > > as follows Done. >> Source/WebCore/ChangeLog:31 >> + - If platform starts sending momentum wheel scroll events before it times out, the timer is > > platform code Done. >> Source/WebCore/page/EventHandler.h:494 >> + Timer m_clearLatchedStateIfNoMomentumWheelEventsStartTimer; > > I think we can use a shorter name for this. Maybe m_pendingMomentumWheelEventsTimer Done. >> Source/WebCore/page/mac/EventHandlerMac.mm:951 >> + m_frame.mainFrame().resetLatchingState(); > > So this call to resetLatchingState() is new, and happens when we've seen the last non-momentum event and we're under the 0.1s interval. Is it OK to reset at this point? This call to resetLatchingState happens when: 1- there is an end of non-momentum scroll event (this activates the timer as per line 947); 2- before the timer fires (so still under 0.1s interval), another wheel scroll begins (see shouldConsiderLatching() in line 946). Given the very short interval (0.1s), users won't be able to trigger this case manually, but it could happen on wheel events sent by JS. It is my understanding that it is be safe to reset given that there were an explicit finger-up gesture, not followed by a start of momentum scroll. >> Source/WebCore/platform/PlatformWheelEvent.h:221 >> + inline bool PlatformWheelEvent::isTransitioningToMomentumScrolling() const > > Call this isTransitioningToMomentumScroll() to match isEndOfNonMomentumScroll. Done.

Antonio Gomes

Comment 17 2016-03-28 17:54:05 PDT

Created attachment 275070 [details] Patch v2.2 - addressed Simon's comments.

Simon Fraser (smfr)

Comment 18 2016-03-29 14:12:34 PDT

Comment on attachment 275070 [details] Patch v2.2 - addressed Simon's comments. View in context: https://bugs.webkit.org/attachment.cgi?id=275070&action=review > Source/WebCore/page/mac/EventHandlerMac.mm:953 > + // Wheel events machinary is transitioning to momenthum scrolling, so no need to reset latched state. Stop the timer! momenthum. This comment should be moved to the block below, adding {}

Antonio Gomes

Comment 19 2016-03-29 14:25:26 PDT

Created attachment 275133 [details] Patch for la

WebKit Commit Bot

Comment 20 2016-03-29 15:17:03 PDT

Comment on attachment 275133 [details] Patch for la Clearing flags on attachment: 275133 Committed r198805: <http://trac.webkit.org/changeset/198805>

WebKit Commit Bot

Comment 21 2016-03-29 15:17:09 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 22 2016-04-11 15:44:54 PDT

<rdar://problem/25668015>

WebKit Commit Bot

Comment 23 2016-04-11 15:49:16 PDT

Re-opened since this is blocked by bug 156476

Alex Christensen

Comment 24 2016-04-11 16:16:39 PDT

Re-closing because this was not actually rolled out.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component UI Events

Assignee

Antonio Gomes

Reported

2016-03-22 06:26 PDT

Modified

2016-04-11 16:16 PDT History

CC List

10 users Show

URL

http://jsbin.com/sizosozexa/edit?html,output

Keywords InRadar

Depends on

156476 156479 155940

Blocks

Dependencies

tree graph