111413 2013-03-05 00:27:51 -0800 Support wheel event even when FrameView does not have scrollbars. 2013-03-05 20:59:14 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 110066 1 dongseong.hwang dongseong.hwang allan.jensen bdakin buildbot kenneth rniwa simon.fraser oldest_to_newest 847373 0 dongseong.hwang 2013-03-05 00:27:51 -0800 Currently, when FrameView does not have scrollbars, FrameView does not support wheel event. However, our mouse can have wheel button without scrollbars in FrameView. We can emulate wheel event as delegates scrolling does. So this patch make FrameView support wheel event even when FrameView does not have scrollbars. In addition, EFL WK2 will turn off delegates scrolling, so this patch will be needed. 847381 1 dongseong.hwang 2013-03-05 00:32:27 -0800 This policy has been so long time. I quote as follows from r37159. void ScrollView::wheelEvent(PlatformWheelEvent& e) { - if (!allowsScrolling() || platformWidget()) + // We don't allow mouse wheeling to happen in a ScrollView that has had its scrollbars explicitly disabled. + if (!canHaveScrollbars() || platformWidget()) return; I assume there is no ports that do not have scrollbars without having delegates scrolling, because I think that wheel event is too essential to be disabled. 847388 2 191423 dongseong.hwang 2013-03-05 00:38:00 -0800 Created attachment 191423 Patch 847422 3 191423 allan.jensen 2013-03-05 01:28:02 -0800 Comment on attachment 191423 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191423&action=review > Source/WebCore/page/FrameView.cpp:3894 > - if (delegatesScrolling()) { > - IntSize offset = scrollOffset(); > - IntSize newOffset = IntSize(offset.width() - wheelEvent.deltaX(), offset.height() - wheelEvent.deltaY()); > - if (offset != newOffset) { > - ScrollView::scrollTo(newOffset); > - scrollPositionChanged(); > - frame()->loader()->client()->didChangeScrollOffset(); > - } > + if (delegatesScrolling() || !canHaveScrollbars()) { > + IntPoint position = scrollPosition(); > + IntPoint newPosition = position - IntSize(wheelEvent.deltaX(), wheelEvent.deltaY()); > + newPosition = adjustScrollPositionWithinRange(newPosition); > + if (position != newPosition) > + scrollTo(IntSize(newPosition.x(), newPosition.y())); Is scrollPositionChanged() and frame()->loader()->client()->didChangeScrollOffset() no longer needed? Two, are you sure every port that disables scrollbars wants the wheel-event redirected to scrollTo? The deleted comment below must have been there for a reason. 847451 4 dongseong.hwang 2013-03-05 01:51:46 -0800 (In reply to comment #3) > (From update of attachment 191423 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191423&action=review > > > Source/WebCore/page/FrameView.cpp:3894 > > - if (delegatesScrolling()) { > > - IntSize offset = scrollOffset(); > > - IntSize newOffset = IntSize(offset.width() - wheelEvent.deltaX(), offset.height() - wheelEvent.deltaY()); > > - if (offset != newOffset) { > > - ScrollView::scrollTo(newOffset); > > - scrollPositionChanged(); > > - frame()->loader()->client()->didChangeScrollOffset(); > > - } > > + if (delegatesScrolling() || !canHaveScrollbars()) { > > + IntPoint position = scrollPosition(); > > + IntPoint newPosition = position - IntSize(wheelEvent.deltaX(), wheelEvent.deltaY()); > > + newPosition = adjustScrollPositionWithinRange(newPosition); > > + if (position != newPosition) > > + scrollTo(IntSize(newPosition.x(), newPosition.y())); > Thanks for review :) > Is scrollPositionChanged() and frame()->loader()->client()->didChangeScrollOffset() no longer needed? Good point! FrameView::scrollTo does the same thing. void FrameView::scrollTo(const IntSize& newOffset) { LayoutSize offset = scrollOffset(); ScrollView::scrollTo(newOffset); if (offset != scrollOffset()) scrollPositionChanged(); frame()->loader()->client()->didChangeScrollOffset(); } > Two, are you sure every port that disables scrollbars wants the wheel-event redirected to scrollTo? The deleted comment below must have been there for a reason. I'm not sure, so I need feedback. As I mentioned #c1, I guess it seems not to affect badly... The deleted comment had been even in 2008. 847460 5 allan.jensen 2013-03-05 02:03:11 -0800 (In reply to comment #4) > I'm not sure, so I need feedback. As I mentioned #c1, I guess it seems not to affect badly... The deleted comment had been even in 2008. No, it is only a year old see bug 78731 and http://trac.webkit.org/changeset/107838 847462 6 allan.jensen 2013-03-05 02:05:01 -0800 (In reply to comment #5) > (In reply to comment #4) > > I'm not sure, so I need feedback. As I mentioned #c1, I guess it seems not to affect badly... The deleted comment had been even in 2008. > > No, it is only a year old see bug 78731 and http://trac.webkit.org/changeset/107838 Nah, sorry. That was only moving code around. 847627 7 191423 buildbot 2013-03-05 06:42:43 -0800 Comment on attachment 191423 Patch Attachment 191423 did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17009118 New failing tests: editing/selection/selection-invalid-offset.html 847819 8 191423 simon.fraser 2013-03-05 10:47:56 -0800 Comment on attachment 191423 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191423&action=review > Source/WebCore/page/FrameView.cpp:3889 > + if (delegatesScrolling() || !canHaveScrollbars()) { This seems like a behavior change, which some platforms don't want. 848425 9 dongseong.hwang 2013-03-05 20:45:53 -0800 (In reply to comment #7) > (From update of attachment 191423 [details]) > Attachment 191423 [details] did not pass mac-ews (mac): > Output: http://webkit-commit-queue.appspot.com/results/17009118 > > New failing tests: > editing/selection/selection-invalid-offset.html I think it is flaky, because IMO this test is not related to wheel event. (In reply to comment #8) > (From update of attachment 191423 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191423&action=review > > > Source/WebCore/page/FrameView.cpp:3889 > > + if (delegatesScrolling() || !canHaveScrollbars()) { > > This seems like a behavior change, which some platforms don't want. Thank you feedback. could you let me know which specific platforms would have problem? And I'm curious if it does not make sense to support wheel event when FrameView can not have scrollbars. otherwise, what is relevant way to support wheel event? If turning off, EFL WK2 can not deal with wheel event because EFL WK2 does not have scrollbars. 848429 10 dongseong.hwang 2013-03-05 20:59:14 -0800 (In reply to comment #9) > If turning off, EFL WK2 can not deal with wheel event because EFL WK2 does not have scrollbars. s/If turning off/If turning off delegates scrolling/ I'm trying to turn off delegates scrolling in EFL and Qt WK2 now, because I think works of Mac and Chromium can cover the requirement of delegated scrolling now. 191423 2013-03-05 00:38:00 -0800 2013-03-05 10:47:56 -0800 Patch bug-111413-20130305173333.patch text/plain 2691 dongseong.hwang U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ0NzEwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZjRmNGQ5ODZhODk5ODdh MzRiYzFkMmFlYzMwNzBkZmFmMzk5ZWQ2Ni4uYTkxODUyYjQ5OGYwOWYxYTRkNmYxZDhmYmQxMjRm NzE4NWNjNzEwNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDEzLTAzLTA1ICBIdWFu ZyBEb25nc3VuZyAgPGx1eHRlbGxhQGNvbXBhbnkxMDAubmV0PgorCisgICAgICAgIFN1cHBvcnQg d2hlZWwgZXZlbnQgZXZlbiB3aGVuIEZyYW1lVmlldyBkb2VzIG5vdCBoYXZlIHNjcm9sbGJhcnMu CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTE0MTMK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDdXJyZW50 bHksIHdoZW4gRnJhbWVWaWV3IGRvZXMgbm90IGhhdmUgc2Nyb2xsYmFycywgRnJhbWVWaWV3IGRv ZXMgbm90IHN1cHBvcnQKKyAgICAgICAgd2hlZWwgZXZlbnQuIEhvd2V2ZXIsIG91ciBtb3VzZSBj YW4gaGF2ZSB3aGVlbCBidXR0b24gd2l0aG91dCBzY3JvbGxiYXJzIGluCisgICAgICAgIEZyYW1l Vmlldy4gV2UgY2FuIGVtdWxhdGUgd2hlZWwgZXZlbnQgYXMgZGVsZWdhdGVzIHNjcm9sbGluZyBk b2VzLiBTbyB0aGlzCisgICAgICAgIHBhdGNoIG1ha2UgRnJhbWVWaWV3IHN1cHBvcnQgd2hlZWwg ZXZlbnQgZXZlbiB3aGVuIEZyYW1lVmlldyBkb2VzIG5vdCBoYXZlCisgICAgICAgIHNjcm9sbGJh cnMuCisKKyAgICAgICAgSW4gYWRkaXRpb24sIEVGTCBXSzIgd2lsbCB0dXJuIG9mZiBkZWxlZ2F0 ZXMgc2Nyb2xsaW5nLCBzbyB0aGlzIHBhdGNoIHdpbGwgYmUKKyAgICAgICAgbmVlZGVkLgorCisg ICAgICAgIE5vIG5ldyB0ZXN0cy4gQ292ZXJlZCBieSBleGlzdGluZyB0ZXN0cywgYmVjYXVzZSBF RkwgYW5kIFF0IHVzZSB0aGUgc2FtZQorICAgICAgICBjb2RlIHRoYXQgZW11bGF0ZXMgd2hlZWwg ZXZlbnQuCisKKyAgICAgICAgKiBwYWdlL0ZyYW1lVmlldy5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpGcmFtZVZpZXc6OndoZWVsRXZlbnQpOgorCiAyMDEzLTAzLTA0ICBIdWFuZyBEb25nc3VuZyAg PGx1eHRlbGxhQGNvbXBhbnkxMDAubmV0PgogCiAgICAgICAgIERvbid0IG92ZXJ1c2UgU2Nyb2xs Vmlldzo6ZGVsZWdhdGVzU2Nyb2xsaW5nKCkuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9w YWdlL0ZyYW1lVmlldy5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5jcHAKaW5k ZXggMzU4NWRkY2I3OWYyYmQzOWU4NGYwOWQ2OTYxZTA2OTkxNjk2ZmY0MC4uODFjYjI2YWUzNzhm YjhmZGQ3Mzk0MTUyZDkwYThhYjM5MTgwYmRhMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv cGFnZS9GcmFtZVZpZXcuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BhZ2UvRnJhbWVWaWV3LmNw cApAQCAtMzg4NiwyMSArMzg4NiwxNSBAQCBib29sIEZyYW1lVmlldzo6d2hlZWxFdmVudChjb25z dCBQbGF0Zm9ybVdoZWVsRXZlbnQmIHdoZWVsRXZlbnQpCiAgICAgICAgIHJldHVybiBmYWxzZTsK ICNlbmRpZgogCi0gICAgaWYgKGRlbGVnYXRlc1Njcm9sbGluZygpKSB7Ci0gICAgICAgIEludFNp emUgb2Zmc2V0ID0gc2Nyb2xsT2Zmc2V0KCk7Ci0gICAgICAgIEludFNpemUgbmV3T2Zmc2V0ID0g SW50U2l6ZShvZmZzZXQud2lkdGgoKSAtIHdoZWVsRXZlbnQuZGVsdGFYKCksIG9mZnNldC5oZWln aHQoKSAtIHdoZWVsRXZlbnQuZGVsdGFZKCkpOwotICAgICAgICBpZiAob2Zmc2V0ICE9IG5ld09m ZnNldCkgewotICAgICAgICAgICAgU2Nyb2xsVmlldzo6c2Nyb2xsVG8obmV3T2Zmc2V0KTsKLSAg ICAgICAgICAgIHNjcm9sbFBvc2l0aW9uQ2hhbmdlZCgpOwotICAgICAgICAgICAgZnJhbWUoKS0+ bG9hZGVyKCktPmNsaWVudCgpLT5kaWRDaGFuZ2VTY3JvbGxPZmZzZXQoKTsKLSAgICAgICAgfQor ICAgIGlmIChkZWxlZ2F0ZXNTY3JvbGxpbmcoKSB8fCAhY2FuSGF2ZVNjcm9sbGJhcnMoKSkgewor ICAgICAgICBJbnRQb2ludCBwb3NpdGlvbiA9IHNjcm9sbFBvc2l0aW9uKCk7CisgICAgICAgIElu dFBvaW50IG5ld1Bvc2l0aW9uID0gcG9zaXRpb24gLSBJbnRTaXplKHdoZWVsRXZlbnQuZGVsdGFY KCksIHdoZWVsRXZlbnQuZGVsdGFZKCkpOworICAgICAgICBuZXdQb3NpdGlvbiA9IGFkanVzdFNj cm9sbFBvc2l0aW9uV2l0aGluUmFuZ2UobmV3UG9zaXRpb24pOworICAgICAgICBpZiAocG9zaXRp b24gIT0gbmV3UG9zaXRpb24pCisgICAgICAgICAgICBzY3JvbGxUbyhJbnRTaXplKG5ld1Bvc2l0 aW9uLngoKSwgbmV3UG9zaXRpb24ueSgpKSk7CiAgICAgICAgIHJldHVybiB0cnVlOwogICAgIH0K IAotICAgIC8vIFdlIGRvbid0IGFsbG93IG1vdXNlIHdoZWVsaW5nIHRvIGhhcHBlbiBpbiBhIFNj cm9sbFZpZXcgdGhhdCBoYXMgaGFkIGl0cyBzY3JvbGxiYXJzIGV4cGxpY2l0bHkgZGlzYWJsZWQu Ci0gICAgaWYgKCFjYW5IYXZlU2Nyb2xsYmFycygpKQotICAgICAgICByZXR1cm4gZmFsc2U7Ci0K ICNpZiAhUExBVEZPUk0oV1gpCiAgICAgaWYgKHBsYXRmb3JtV2lkZ2V0KCkpCiAgICAgICAgIHJl dHVybiBmYWxzZTsK