Bug 111413

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.

Created attachment 191423 [details] Patch

Comment on attachment 191423 [details] 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.

(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.

(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

(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.

Comment on attachment 191423 [details] Patch 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

Comment on attachment 191423 [details] 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.

(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.

(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.