111001 2013-02-27 11:56:43 -0800 Disabling rubber-banding at page top/bottom should not be limited to initial scroll events 2014-02-04 09:37:29 -0800 1 1 1 Unclassified WebKit UI Events 528+ (Nightly build) Mac Unspecified ASSIGNED P2 Normal --- 110006 1 conrad_shultz conrad_shultz andersca bdakin bfulgham cmarcelo jamesr luiz sam thorton tonikitoo webkit.review.bot oldest_to_newest 843334 0 conrad_shultz 2013-02-27 11:56:43 -0800 API introduced as part of the fix for bug 110006 allows rubber-banding to be selectively suppressed for initial scroll events but does not allow suppression of rubber-banding within a scroll session (i.e. if one scrolls to a page extreme and then continues scrolling beyond the boundary). 843349 1 190571 conrad_shultz 2013-02-27 12:06:25 -0800 Created attachment 190571 Patch 844488 2 190571 bdakin 2013-02-28 14:39:56 -0800 Comment on attachment 190571 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190571&action=review I chatted with Sam about this, and ultimately he convinced me that it is just a little to weird to hardcode behavior in WebCore that honors PlatformWheelEventPhaseBegan for horizontal swipes but not for vertical swipes. Ultimately it is just hardcoding a little too much client-specific preference down in client-agnostic code. Instead, you should add new SPI to to WebKit2 that indicates which directions should ignore or honor the BeganPhase when deciding who will handle the wheel event. Perhaps something like WKPageShouldForwardScrollEventsAtTopWithPhase(WKScrollPhase), and then you will have to define a new enum for WKScrollPhase, and of course you'll have three other functions for bottom, left, and right. Then that info can be plumbed down to WebCore, and you can consult that state information in the you are editing in this patch. Sorry to make your life a little harder here! But I think this will be the best. > Source/WebCore/page/scrolling/ScrollingTree.cpp:327 > +bool ScrollingTree::wheelEventMayBeHandledExternally(const PlatformWheelEvent& wheelEvent) What about shouldForwardWheelEvent(). 847146 3 conrad_shultz 2013-03-04 18:12:37 -0800 (In reply to comment #2) > (From update of attachment 190571 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190571&action=review > > I chatted with Sam about this, and ultimately he convinced me that it is just a little to weird to hardcode behavior in WebCore that honors PlatformWheelEventPhaseBegan for horizontal swipes but not for vertical swipes. Ultimately it is just hardcoding a little too much client-specific preference down in client-agnostic code. Instead, you should add new SPI to to WebKit2 that indicates which directions should ignore or honor the BeganPhase when deciding who will handle the wheel event. Perhaps something like WKPageShouldForwardScrollEventsAtTopWithPhase(WKScrollPhase), and then you will have to define a new enum for WKScrollPhase, and of course you'll have three other functions for bottom, left, and right. Then that info can be plumbed down to WebCore, and you can consult that state information in the you are editing in this patch. > > Sorry to make your life a little harder here! But I think this will be the best. I agree, but this runs into a problem. All the various enums representing phase (e.g. WebWheelEvent::Phase, PlatformWheelEventPhase, and the platform-specific NSEventPhase) have PhaseNone == 0. Unfortunately, perfectly valid events (e.g. legacy mouse wheels, momentum scroll events) may be of PhaseNone depending on the platform, which make an API based on phase as currently implemented rather problematic. (E.g. if a client says setShouldForwardScrollEventsAtTopWithPhase(PhaseNone), how does WebKit distinguish between "forward events of PhaseNone" and "forward no events.") I chatted with Beth a little about this and we came up with three possible, all slightly suboptimal, solutions: 1) Define a new purpose-built enum that parallels the existing enums but begins with 1 instead of 0. This seems undesirable since it would greatly increase the likelihood of confusion on the part of developers and creates the (outside) possibility that the underlying real enum could expand to the point of overflowing this new enum. 2) Use the existing enum types in conjunction with a bool to accomplish a similar goal. This seems undesirable since it would greatly complicate the API and just feels hackish. 3) Remove the concept of event phase from the API entirely and focus instead on the logical conditions under which events should be forwarded. For example, I propose something like: enum class WKWheelEventForwardingCondition { WKWheelEventForwardNever, WKWheelEventForwardUnlessNavigating, WKWheelEventForwardAlways } (The second option would impact only horizontal wheel events and would allow back/forward swiping but not rubber-banding.) I'm inclined toward the third option as one that would provide the best balance between API simplicity and flexibility. Sam - thoughts? Other suggestions? > > > Source/WebCore/page/scrolling/ScrollingTree.cpp:327 > > +bool ScrollingTree::wheelEventMayBeHandledExternally(const PlatformWheelEvent& wheelEvent) > > What about shouldForwardWheelEvent(). How about shouldForwardWheelEventToClient(), to distinguish from, e.g., thunking to the main thread? 190571 2013-02-27 12:06:25 -0800 2013-02-28 14:39:55 -0800 Patch bug-111001-20130227120238.patch text/plain 4482 conrad_shultz U3VidmVyc2lvbiBSZXZpc2lvbjogMTQzOTkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZmI3ZDBiNzBmODhhZjI1 YTMzNmM4MDMzZDVhNDdmYTViNjRiMmNjYy4uOGU2ZGMyYWJmMjllOGNlNTk5MmQ1NmI5Zjc2YWIx ODcxZGEwMmE2MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDEzLTAyLTI3ICBDb25y YWQgU2h1bHR6ICA8Y29ucmFkX3NodWx0ekBhcHBsZS5jb20+CisKKyAgICAgICAgRGlzYWJsaW5n IHJ1YmJlci1iYW5kaW5nIGF0IHBhZ2UgdG9wL2JvdHRvbSBzaG91bGQgbm90IGJlIGxpbWl0ZWQg dG8gaW5pdGlhbCBzY3JvbGwgZXZlbnRzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMTEwMDEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIHBhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ1RyZWUuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6U2Nyb2xsaW5nVHJlZTo6dHJ5VG9IYW5kbGVXaGVlbEV2ZW50KToKKyAg ICAgICAgd2lsbFdoZWVsRXZlbnRTdGFydFN3aXBlR2VzdHVyZSAtPiB3aGVlbEV2ZW50TWF5QmVI YW5kbGVkRXh0ZXJuYWxseS4KKyAgICAgICAgKFdlYkNvcmU6OlNjcm9sbGluZ1RyZWU6OndoZWVs RXZlbnRNYXlCZUhhbmRsZWRFeHRlcm5hbGx5KToKKyAgICAgICAgUmVuYW1lZCBmcm9tIHdpbGxX aGVlbEV2ZW50U3RhcnRTd2lwZUdlc3R1cmU7IG9ubHkgY29uc2lkZXIgZXZlbnQgcGhhc2UgaW4g dGhlIGhvcml6b250YWwKKyAgICAgICAgZGlyZWN0aW9uLgorCisgICAgICAgICogcGFnZS9zY3Jv bGxpbmcvU2Nyb2xsaW5nVHJlZS5oOgorICAgICAgICAoU2Nyb2xsaW5nVHJlZSk6CisgICAgICAg IHdpbGxXaGVlbEV2ZW50U3RhcnRTd2lwZUdlc3R1cmUgLT4gd2hlZWxFdmVudE1heUJlSGFuZGxl ZEV4dGVybmFsbHkuCisKKyAgICAgICAgKiBwYWdlL3Njcm9sbGluZy9tYWMvU2Nyb2xsaW5nQ29v cmRpbmF0b3JNYWMubW06CisgICAgICAgIChXZWJDb3JlOjpTY3JvbGxpbmdDb29yZGluYXRvck1h Yzo6aGFuZGxlV2hlZWxFdmVudCk6CisgICAgICAgIERpdHRvLgorCiAyMDEzLTAyLTI1ICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgogCiAgICAgICAgIExvdHMgb2YgdW5u ZWNlc3NhcnkgRGlkTGF5b3V0IG5vdGlmaWNhdGlvbnMgd2hlbiBzY3JvbGxpbmcgem9vbWVkIHBh Z2Ugd2l0aCBpZnJhbWVzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL3Njcm9sbGlu Zy9TY3JvbGxpbmdUcmVlLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL1Njcm9s bGluZ1RyZWUuY3BwCmluZGV4IDJjNzY2MDQ4YzU2Y2ZhYjIwMDUwODI5OTg3YzJkNDMxMjBlNGM3 ZGUuLmFkNDRhYjc3ZGM3Mzk3MmE2NWExYmM3MjQ4ZGMxOWI0YTNkMmI5YTggMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ1RyZWUuY3BwCisrKyBiL1Nv dXJjZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ1RyZWUuY3BwCkBAIC04NSw3ICs4 NSw3IEBAIFNjcm9sbGluZ1RyZWU6OkV2ZW50UmVzdWx0IFNjcm9sbGluZ1RyZWU6OnRyeVRvSGFu ZGxlV2hlZWxFdmVudChjb25zdCBQbGF0Zm9ybVdoCiAgICAgICAgIH0KICAgICB9CiAKLSAgICBp ZiAod2lsbFdoZWVsRXZlbnRTdGFydFN3aXBlR2VzdHVyZSh3aGVlbEV2ZW50KSkKKyAgICBpZiAo d2hlZWxFdmVudE1heUJlSGFuZGxlZEV4dGVybmFsbHkod2hlZWxFdmVudCkpCiAgICAgICAgIHJl dHVybiBEaWROb3RIYW5kbGVFdmVudDsKIAogICAgIFNjcm9sbGluZ1RocmVhZDo6ZGlzcGF0Y2go YmluZCgmU2Nyb2xsaW5nVHJlZTo6aGFuZGxlV2hlZWxFdmVudCwgdGhpcywgd2hlZWxFdmVudCkp OwpAQCAtMzI0LDE2ICszMjQsMTQgQEAgdm9pZCBTY3JvbGxpbmdUcmVlOjpzZXRSdWJiZXJCYW5k c0F0VG9wKGJvb2wgcnViYmVyQmFuZHNBdFRvcCkKICAgICBtX3J1YmJlckJhbmRzQXRUb3AgPSBy dWJiZXJCYW5kc0F0VG9wOwogfQogCi1ib29sIFNjcm9sbGluZ1RyZWU6OndpbGxXaGVlbEV2ZW50 U3RhcnRTd2lwZUdlc3R1cmUoY29uc3QgUGxhdGZvcm1XaGVlbEV2ZW50JiB3aGVlbEV2ZW50KQor Ym9vbCBTY3JvbGxpbmdUcmVlOjp3aGVlbEV2ZW50TWF5QmVIYW5kbGVkRXh0ZXJuYWxseShjb25z dCBQbGF0Zm9ybVdoZWVsRXZlbnQmIHdoZWVsRXZlbnQpCiB7Ci0gICAgaWYgKHdoZWVsRXZlbnQu cGhhc2UoKSAhPSBQbGF0Zm9ybVdoZWVsRXZlbnRQaGFzZUJlZ2FuKQotICAgICAgICByZXR1cm4g ZmFsc2U7Ci0KICAgICBNdXRleExvY2tlciBsb2NrKG1fc3dpcGVTdGF0ZU11dGV4KTsKIAotICAg IGlmICh3aGVlbEV2ZW50LmRlbHRhWCgpID4gMCAmJiBtX21haW5GcmFtZVBpbm5lZFRvVGhlTGVm dCAmJiBtX2NhbkdvQmFjaykKKyAgICBib29sIGlzUGhhc2VCZWdhbiA9IHdoZWVsRXZlbnQucGhh c2UoKSA9PSBQbGF0Zm9ybVdoZWVsRXZlbnRQaGFzZUJlZ2FuOworICAgIGlmIChpc1BoYXNlQmVn YW4gJiYgd2hlZWxFdmVudC5kZWx0YVgoKSA+IDAgJiYgbV9tYWluRnJhbWVQaW5uZWRUb1RoZUxl ZnQgJiYgbV9jYW5Hb0JhY2spCiAgICAgICAgIHJldHVybiB0cnVlOwotICAgIGlmICh3aGVlbEV2 ZW50LmRlbHRhWCgpIDwgMCAmJiBtX21haW5GcmFtZVBpbm5lZFRvVGhlUmlnaHQgJiYgbV9jYW5H b0ZvcndhcmQpCisgICAgaWYgKGlzUGhhc2VCZWdhbiAmJiB3aGVlbEV2ZW50LmRlbHRhWCgpIDwg MCAmJiBtX21haW5GcmFtZVBpbm5lZFRvVGhlUmlnaHQgJiYgbV9jYW5Hb0ZvcndhcmQpCiAgICAg ICAgIHJldHVybiB0cnVlOwogICAgIGlmICh3aGVlbEV2ZW50LmRlbHRhWSgpID4gMCAmJiBtX21h aW5GcmFtZVBpbm5lZFRvVGhlVG9wICYmICFtX3J1YmJlckJhbmRzQXRUb3ApCiAgICAgICAgIHJl dHVybiB0cnVlOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9zY3JvbGxpbmcvU2Ny b2xsaW5nVHJlZS5oIGIvU291cmNlL1dlYkNvcmUvcGFnZS9zY3JvbGxpbmcvU2Nyb2xsaW5nVHJl ZS5oCmluZGV4IGFkN2JjZWIyMjljYzI0MWExNzc1YTE1MDA2ZGU5NjQ2ODc2ZGVkMzEuLmY4ODU1 MDhkZTJmZTNiMThlZjEyZjNhNDY2MWJjNDliZWI0YTBmZTggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ1RyZWUuaAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9wYWdlL3Njcm9sbGluZy9TY3JvbGxpbmdUcmVlLmgKQEAgLTEwMiw3ICsxMDIsNyBAQCBwdWJs aWM6CiAgICAgYm9vbCBydWJiZXJCYW5kc0F0VG9wKCk7CiAgICAgdm9pZCBzZXRSdWJiZXJCYW5k c0F0VG9wKGJvb2wpOwogCi0gICAgYm9vbCB3aWxsV2hlZWxFdmVudFN0YXJ0U3dpcGVHZXN0dXJl KGNvbnN0IFBsYXRmb3JtV2hlZWxFdmVudCYpOworICAgIGJvb2wgd2hlZWxFdmVudE1heUJlSGFu ZGxlZEV4dGVybmFsbHkoY29uc3QgUGxhdGZvcm1XaGVlbEV2ZW50Jik7CiAKICAgICB2b2lkIHNl dFNjcm9sbGluZ1BlcmZvcm1hbmNlTG9nZ2luZ0VuYWJsZWQoYm9vbCBmbGFnKTsKICAgICBib29s IHNjcm9sbGluZ1BlcmZvcm1hbmNlTG9nZ2luZ0VuYWJsZWQoKTsKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL21hYy9TY3JvbGxpbmdDb29yZGluYXRvck1hYy5tbSBi L1NvdXJjZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL21hYy9TY3JvbGxpbmdDb29yZGluYXRvck1h Yy5tbQppbmRleCBlMDlmYjc2ZDVmZDgxZWQ2MDQ3MTgyMDNiMzE5YTRhYzVjYTg1MDdlLi40MjBi MTNjNzM4YTYwMDMyMzY1MTczZTI3ZTU5ZmMxYzA2YTEyMWEzIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wYWdlL3Njcm9sbGluZy9tYWMvU2Nyb2xsaW5nQ29vcmRpbmF0b3JNYWMubW0KKysr IGIvU291cmNlL1dlYkNvcmUvcGFnZS9zY3JvbGxpbmcvbWFjL1Njcm9sbGluZ0Nvb3JkaW5hdG9y TWFjLm1tCkBAIC0yNDIsNyArMjQyLDcgQEAgYm9vbCBTY3JvbGxpbmdDb29yZGluYXRvck1hYzo6 aGFuZGxlV2hlZWxFdmVudChGcmFtZVZpZXcqLCBjb25zdCBQbGF0Zm9ybVdoZWVsRXYKICAgICBB U1NFUlQoaXNNYWluVGhyZWFkKCkpOwogICAgIEFTU0VSVChtX3BhZ2UpOwogCi0gICAgaWYgKG1f c2Nyb2xsaW5nVHJlZS0+d2lsbFdoZWVsRXZlbnRTdGFydFN3aXBlR2VzdHVyZSh3aGVlbEV2ZW50 KSkKKyAgICBpZiAobV9zY3JvbGxpbmdUcmVlLT53aGVlbEV2ZW50TWF5QmVIYW5kbGVkRXh0ZXJu YWxseSh3aGVlbEV2ZW50KSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwogCiAgICAgU2Nyb2xsaW5n VGhyZWFkOjpkaXNwYXRjaChiaW5kKCZTY3JvbGxpbmdUcmVlOjpoYW5kbGVXaGVlbEV2ZW50LCBt X3Njcm9sbGluZ1RyZWUuZ2V0KCksIHdoZWVsRXZlbnQpKTsK