136029 2014-08-17 14:02:03 -0700 [Mac] Gesture scrolls don't work in the WebKit1 clients after scrolling a non-scrollable iFrame 2014-08-18 13:42:53 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Mac All RESOLVED FIXED InRadar P1 Blocker --- 134569 136040 1 bfulgham bfulgham bdakin bfulgham jonlee sam simon.fraser webkit-bug-importer oldest_to_newest 1029679 0 bfulgham 2014-08-17 14:02:03 -0700 Testing with various WebKit1 clients after my change for Iframe scroll gestures has revealed an edge case: Under the following conditions: 1. A Cocoa application embeds a WebView using WK1. 2. The webview consists of an Iframe. 3. The Iframe size is the same as the WebView, so that there is no scrollable region. If the user performs a scroll wheel gesture on the web view, other scrollable portions of the user interface become "locked up". 1029680 1 bfulgham 2014-08-17 14:03:05 -0700 <rdar://problem/17767169> 1029681 2 bfulgham 2014-08-17 14:05:01 -0700 Analysis indicates that WebKit is issuing two scrollWheel events during the handling of this gesture. The two events are intercepted at the OS level and result in some undesirable side effects that end up locking the view. It looks like the nuance I missed in my earlier work was that although we bail out early when interacting with IFrames in the general case (under WK1), when we pass through the EventHandler::passWheelEventToWidget for an IFrame we actually DO dispatch the 'scrollWheel' event to the NSScrollView backing the IFrame. This seems to set up the multiple-scrollWheel event condition found in Dictionary.app. I think the correct fix is to recognize a WK1 case where the widget to which we just passed the wheel event is an frame view (IFrame), to not dispatch a second scrollWheel event by returning 'true' in this case. 1029682 3 236736 bfulgham 2014-08-17 14:12:46 -0700 Created attachment 236736 Patch 1029683 4 sam 2014-08-17 14:27:18 -0700 Since this depends on host application logic, it might be a good candidate for a TestWebKitAPI test. 1029709 5 236736 mjs 2014-08-17 23:35:09 -0700 Comment on attachment 236736 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236736&action=review > Source/WebCore/page/mac/EventHandlerMac.mm:923 > + // If the platform widget is a frame view (IFrame), and we are in WebKit1, 'passWheelEventToWidget' has already > + // called 'scrollWheel' on the underlying platform widget. We need to return true in this case to avoid getting > + // multiple copies of the scrollWheel event sent to the platform widget. IS such a long comment really necessary? I'd suggest no comment, or a one-liner stating the reason. 1029768 6 bfulgham 2014-08-18 08:54:34 -0700 Committed r172703: <http://trac.webkit.org/changeset/172703> 1029769 7 bfulgham 2014-08-18 08:55:08 -0700 (In reply to comment #5) > (From update of attachment 236736 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236736&action=review > > > Source/WebCore/page/mac/EventHandlerMac.mm:923 > > + // If the platform widget is a frame view (IFrame), and we are in WebKit1, 'passWheelEventToWidget' has already > > + // called 'scrollWheel' on the underlying platform widget. We need to return true in this case to avoid getting > > + // multiple copies of the scrollWheel event sent to the platform widget. > > IS such a long comment really necessary? I'd suggest no comment, or a one-liner stating the reason. OK! Fixed while landing. 1029771 8 bfulgham 2014-08-18 08:55:36 -0700 See Bug 136040 for work on creating a TestWebKitAPI test for this condition. 1029780 9 236736 darin 2014-08-18 09:32:50 -0700 Comment on attachment 236736 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236736&action=review > Source/WebCore/page/EventHandler.cpp:2658 > +bool EventHandler::platformCompletePlatformWidgetWheelEvent(const PlatformWheelEvent&, const Widget*, ContainerNode*) Since this can never be null, it should be a Widget& rather than a Widget*. 1029783 10 236736 bfulgham 2014-08-18 09:37:42 -0700 Comment on attachment 236736 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236736&action=review >> Source/WebCore/page/EventHandler.cpp:2658 >> +bool EventHandler::platformCompletePlatformWidgetWheelEvent(const PlatformWheelEvent&, const Widget*, ContainerNode*) > > Since this can never be null, it should be a Widget& rather than a Widget*. Good point! I'll land this as a follow-up patch. 1029787 11 bfulgham 2014-08-18 09:52:05 -0700 Added Darin's suggested clean-up: Committed r172705: <http://trac.webkit.org/changeset/172705> 1029852 12 bfulgham 2014-08-18 13:42:53 -0700 Correct a bad merge from my last change: Committed in r172720. <http://trac.webkit.org/changeset/172720> 236736 2014-08-17 14:12:46 -0700 2014-08-18 09:37:41 -0700 Patch bug-136029-20140817141237.patch text/plain 4935 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE3MjY5NikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI4IEBACisyMDE0LTA4LTE3ICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFtNYWNdIEdlc3R1cmUgc2Ny b2xscyBkb24ndCB3b3JrIGluIHRoZSBXZWJLaXQxIGNsaWVudHMgYWZ0ZXIgc2Nyb2xsaW5nIGEg bm9uLXNjcm9sbGFibGUgaUZyYW1lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xMzYwMjkKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzE3NzY3MTY5Pgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgaXMg ZGlmZmljdWx0IHRvIHRlc3QgaW4gb3VyIGxheW91dCB0ZXN0cywgc2luY2UgaXQgcmVsaWVzIG9u IHNwZWNpZmljIGhvc3QgYXBwbGljYXRpb24KKyAgICAgICAgYmVoYXZpb3IuCisKKyAgICAgICAg SWYgdGhlIHBsYXRmb3JtIHdpZGdldCBpcyBhIGZyYW1lIHZpZXcgKElGcmFtZSksIGFuZCB3ZSBh cmUgaW4gV2ViS2l0MSwgJ3Bhc3NXaGVlbEV2ZW50VG9XaWRnZXQnCisgICAgICAgIGhhcyBhbHJl YWR5IGNhbGxlZCAnc2Nyb2xsV2hlZWwnIG9uIHRoZSB1bmRlcmx5aW5nIHBsYXRmb3JtIHdpZGdl dC4gV2UgbmVlZCB0byByZXR1cm4gdHJ1ZSBpbgorICAgICAgICB0aGlzIGNhc2UgdG8gYXZvaWQg Z2V0dGluZyBtdWx0aXBsZSBjb3BpZXMgb2YgdGhlIHNjcm9sbFdoZWVsIGV2ZW50IHNlbnQgdG8g dGhlIHBsYXRmb3JtIHdpZGdldC4KKworICAgICAgICAqIHBhZ2UvRXZlbnRIYW5kbGVyLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkV2ZW50SGFuZGxlcjo6cGxhdGZvcm1Db21wbGV0ZVBsYXRmb3Jt V2lkZ2V0V2hlZWxFdmVudCk6IFJldmlzZSBkZWZhdWx0IHN0dWIgdG8KKyAgICAgICAgYWNjZXB0 IGEgV2lkZ2V0KiBhcmd1bWVudC4KKyAgICAgICAgKFdlYkNvcmU6OkV2ZW50SGFuZGxlcjo6aGFu ZGxlV2hlZWxFdmVudCk6CisgICAgICAgICogcGFnZS9FdmVudEhhbmRsZXIuaDoKKyAgICAgICAg KiBwYWdlL21hYy9FdmVudEhhbmRsZXJNYWMubW06CisgICAgICAgIChXZWJDb3JlOjpFdmVudEhh bmRsZXI6OnBsYXRmb3JtQ29tcGxldGVQbGF0Zm9ybVdpZGdldFdoZWVsRXZlbnQpOiBDaGVjayBm b3IgV0sxIGNhc2UgYW5kCisgICAgICAgIHJldHVybiAndHJ1ZScgd2hlbiB0aGUgdW5kZXJseWlu ZyBOU1Njcm9sbFZpZXcgaGFzIGJlZW4gaXNzdWVzIGEgc2Nyb2xsV2hlZWwgZXZlbnQuCisgICAg ICAgIAorCiAyMDE0LTA4LTE1ICBEZWFuIEphY2tzb24gIDxkaW5vQGFwcGxlLmNvbT4KIAogICAg ICAgICBSRUdSRVNTSU9OIChyMTY0MTczKTogU2FmYXJpIEF1dG9GaWxsIGJ1dHRvbiBpbiBwb3Bv dmVyLCBkZWZhdWx0IGJ1dHRvbnMgb24gaUNsb3VkIGNvbnRyb2wgcGFuZWwgYW5kIFNhZmFyaSBT YWZlIEJyb3dzaW5nIHBhZ2VzIGRvbid0IGhhdmUgZGVmYXVsdCBhcHBlYXJhbmNlCkluZGV4OiBT b3VyY2UvV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L1dlYkNvcmUvcGFnZS9FdmVudEhhbmRsZXIuY3BwCShyZXZpc2lvbiAxNzI2OTUpCisrKyBTb3Vy Y2UvV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI2NTUs NyArMjY1NSw3IEBAIGJvb2wgRXZlbnRIYW5kbGVyOjpwbGF0Zm9ybUNvbXBsZXRlV2hlZWwKICAg ICByZXR1cm4gZGlkSGFuZGxlRXZlbnQ7CiB9CiAKLWJvb2wgRXZlbnRIYW5kbGVyOjpwbGF0Zm9y bUNvbXBsZXRlUGxhdGZvcm1XaWRnZXRXaGVlbEV2ZW50KGNvbnN0IFBsYXRmb3JtV2hlZWxFdmVu dCYsIENvbnRhaW5lck5vZGUqKQorYm9vbCBFdmVudEhhbmRsZXI6OnBsYXRmb3JtQ29tcGxldGVQ bGF0Zm9ybVdpZGdldFdoZWVsRXZlbnQoY29uc3QgUGxhdGZvcm1XaGVlbEV2ZW50JiwgY29uc3Qg V2lkZ2V0KiwgQ29udGFpbmVyTm9kZSopCiB7CiAgICAgcmV0dXJuIHRydWU7CiB9CkBAIC0yNzE0 LDcgKzI3MTQsNyBAQCBib29sIEV2ZW50SGFuZGxlcjo6aGFuZGxlV2hlZWxFdmVudChjb25zCiAg ICAgICAgICAgICAgICAgICAgICAgICBzY3JvbGxhYmxlQXJlYS0+c2V0U2Nyb2xsZWRQcm9ncmFt bWF0aWNhbGx5KGZhbHNlKTsKICAgICAgICAgICAgICAgICAgICAgaWYgKCF3aWRnZXQtPnBsYXRm b3JtV2lkZ2V0KCkpCiAgICAgICAgICAgICAgICAgICAgICAgICByZXR1cm4gdHJ1ZTsKLSAgICAg ICAgICAgICAgICAgICAgcmV0dXJuIHBsYXRmb3JtQ29tcGxldGVQbGF0Zm9ybVdpZGdldFdoZWVs RXZlbnQoZXZlbnQsIHNjcm9sbGFibGVDb250YWluZXIuZ2V0KCkpOworICAgICAgICAgICAgICAg ICAgICByZXR1cm4gcGxhdGZvcm1Db21wbGV0ZVBsYXRmb3JtV2lkZ2V0V2hlZWxFdmVudChldmVu dCwgd2lkZ2V0LCBzY3JvbGxhYmxlQ29udGFpbmVyLmdldCgpKTsKICAgICAgICAgICAgICAgICB9 CiAgICAgICAgICAgICB9CiAgICAgICAgIH0KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BhZ2UvRXZl bnRIYW5kbGVyLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGFnZS9FdmVudEhhbmRs ZXIuaAkocmV2aXNpb24gMTcyNjk1KQorKysgU291cmNlL1dlYkNvcmUvcGFnZS9FdmVudEhhbmRs ZXIuaAkod29ya2luZyBjb3B5KQpAQCAtMjAwLDcgKzIwMCw3IEBAIHB1YmxpYzoKICAgICB2b2lk IHBsYXRmb3JtUHJlcGFyZUZvcldoZWVsRXZlbnRzKGNvbnN0IFBsYXRmb3JtV2hlZWxFdmVudCYs IGNvbnN0IEhpdFRlc3RSZXN1bHQmLCBSZWZQdHI8RWxlbWVudD4mIGV2ZW50VGFyZ2V0LCBSZWZQ dHI8Q29udGFpbmVyTm9kZT4mIHNjcm9sbGFibGVDb250YWluZXIsIFNjcm9sbGFibGVBcmVhKiYs IGJvb2wmIGlzT3ZlcldpZGdldCk7CiAgICAgdm9pZCBwbGF0Zm9ybVJlY29yZFdoZWVsRXZlbnQo Y29uc3QgUGxhdGZvcm1XaGVlbEV2ZW50Jik7CiAgICAgYm9vbCBwbGF0Zm9ybUNvbXBsZXRlV2hl ZWxFdmVudChjb25zdCBQbGF0Zm9ybVdoZWVsRXZlbnQmLCBFbGVtZW50KiBldmVudFRhcmdldCwg Q29udGFpbmVyTm9kZSogc2Nyb2xsYWJsZUNvbnRhaW5lciwgU2Nyb2xsYWJsZUFyZWEqKTsKLSAg ICBib29sIHBsYXRmb3JtQ29tcGxldGVQbGF0Zm9ybVdpZGdldFdoZWVsRXZlbnQoY29uc3QgUGxh dGZvcm1XaGVlbEV2ZW50JiwgQ29udGFpbmVyTm9kZSogc2Nyb2xsYWJsZUNvbnRhaW5lcik7Cisg ICAgYm9vbCBwbGF0Zm9ybUNvbXBsZXRlUGxhdGZvcm1XaWRnZXRXaGVlbEV2ZW50KGNvbnN0IFBs YXRmb3JtV2hlZWxFdmVudCYsIGNvbnN0IFdpZGdldCosIENvbnRhaW5lck5vZGUqIHNjcm9sbGFi bGVDb250YWluZXIpOwogCiAjaWYgRU5BQkxFKElPU19UT1VDSF9FVkVOVFMpIHx8IEVOQUJMRShJ T1NfR0VTVFVSRV9FVkVOVFMpCiAgICAgdHlwZWRlZiBWZWN0b3I8UmVmUHRyPFRvdWNoPj4gVG91 Y2hBcnJheTsKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BhZ2UvbWFjL0V2ZW50SGFuZGxlck1hYy5t bQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wYWdlL21hYy9FdmVudEhhbmRsZXJNYWMu bW0JKHJldmlzaW9uIDE3MjY5NSkKKysrIFNvdXJjZS9XZWJDb3JlL3BhZ2UvbWFjL0V2ZW50SGFu ZGxlck1hYy5tbQkod29ya2luZyBjb3B5KQpAQCAtOTE2LDggKzkxNiwxNCBAQCBib29sIEV2ZW50 SGFuZGxlcjo6cGxhdGZvcm1Db21wbGV0ZVdoZWVsCiAgICAgcmV0dXJuIGRpZEhhbmRsZUV2ZW50 OwogfQogCi1ib29sIEV2ZW50SGFuZGxlcjo6cGxhdGZvcm1Db21wbGV0ZVBsYXRmb3JtV2lkZ2V0 V2hlZWxFdmVudChjb25zdCBQbGF0Zm9ybVdoZWVsRXZlbnQmIHdoZWVsRXZlbnQsIENvbnRhaW5l ck5vZGUqIHNjcm9sbGFibGVDb250YWluZXIpCitib29sIEV2ZW50SGFuZGxlcjo6cGxhdGZvcm1D b21wbGV0ZVBsYXRmb3JtV2lkZ2V0V2hlZWxFdmVudChjb25zdCBQbGF0Zm9ybVdoZWVsRXZlbnQm IHdoZWVsRXZlbnQsIGNvbnN0IFdpZGdldCogd2lkZ2V0LCBDb250YWluZXJOb2RlKiBzY3JvbGxh YmxlQ29udGFpbmVyKQogeworICAgIC8vIElmIHRoZSBwbGF0Zm9ybSB3aWRnZXQgaXMgYSBmcmFt ZSB2aWV3IChJRnJhbWUpLCBhbmQgd2UgYXJlIGluIFdlYktpdDEsICdwYXNzV2hlZWxFdmVudFRv V2lkZ2V0JyBoYXMgYWxyZWFkeQorICAgIC8vIGNhbGxlZCAnc2Nyb2xsV2hlZWwnIG9uIHRoZSB1 bmRlcmx5aW5nIHBsYXRmb3JtIHdpZGdldC4gV2UgbmVlZCB0byByZXR1cm4gdHJ1ZSBpbiB0aGlz IGNhc2UgdG8gYXZvaWQgZ2V0dGluZworICAgIC8vIG11bHRpcGxlIGNvcGllcyBvZiB0aGUgc2Ny b2xsV2hlZWwgZXZlbnQgc2VudCB0byB0aGUgcGxhdGZvcm0gd2lkZ2V0LgorICAgIGlmIChmcmFt ZUhhc1BsYXRmb3JtV2lkZ2V0KG1fZnJhbWUpICYmIHdpZGdldC0+aXNGcmFtZVZpZXcoKSkKKyAg ICAgICAgcmV0dXJuIHRydWU7CisKICAgICBpZiAod2hlZWxFdmVudC51c2VMYXRjaGVkRXZlbnRF bGVtZW50KCkgJiYgbV9sYXRjaGVkU2Nyb2xsYWJsZUNvbnRhaW5lciAmJiBzY3JvbGxhYmxlQ29u dGFpbmVyID09IG1fbGF0Y2hlZFNjcm9sbGFibGVDb250YWluZXIpCiAgICAgICAgIHJldHVybiAh bV9zdGFydGVkR2VzdHVyZUF0U2Nyb2xsTGltaXQ7CiAK