| Summary: | Wheel events' latching state is not reset when appropriate | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antonio Gomes <tonikitoo> | ||||||||||||||||||||||
| Component: | UI Events | Assignee: | Antonio Gomes <tonikitoo> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | achristensen, bfulgham, buildbot, commit-queue, dbates, manian, rniwa, simon.fraser, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| URL: | http://jsbin.com/sizosozexa/edit?html,output | ||||||||||||||||||||||||
| Bug Depends on: | 156476, 156479, 155940 | ||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Antonio Gomes
2016-03-22 06:26:30 PDT
Created attachment 274647 [details]
Patch for EWS.
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.
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 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
Created attachment 274833 [details]
Patch for EWS.
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.
Created attachment 274984 [details]
Patch v1
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.
Created attachment 274985 [details]
Patch v1.1
Created attachment 275008 [details]
Patch v1.2
Patch ready for review.
Please note that it sits on top of the patch in bug 155940. 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). 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. Created attachment 275065 [details]
Patch v2.1 - addressed Dan's review + rebased
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. 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. Created attachment 275070 [details]
Patch v2.2 - addressed Simon's comments.
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 {} Created attachment 275133 [details]
Patch for la
Comment on attachment 275133 [details] Patch for la Clearing flags on attachment: 275133 Committed r198805: <http://trac.webkit.org/changeset/198805> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 156476 Re-closing because this was not actually rolled out. |