[webkit-reviews] review granted: [Bug 91655] overflow:scroll elements should support rubber-banding : [Attachment 236832] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 91655: overflow:scroll elements should support rubber-banding
https://bugs.webkit.org/show_bug.cgi?id=91655

Attachment 236832: Patch
https://bugs.webkit.org/attachment.cgi?id=236832&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236832&action=review


I am not expert on the structure of the scrolling code, but it all looks pretty
good to me.

> Source/WebCore/dom/Element.cpp:-265
> -    if (!(event.deltaX() || event.deltaY()))
> -	   return true;

I understand why we need to remove this for correct behavior of some new cases.
But how do we now deal with the original problem that led us to write this
code? Will this change introduce a problem? Was the code just mistaken before?

> Source/WebCore/page/EventHandler.cpp:300
> +    bool shouldHandleEvent = (axis == ScrollEventAxis::Vertical &&
wheelEvent->deltaY()) || (axis == ScrollEventAxis::Horizontal &&
wheelEvent->deltaX());

Looking at this code makes me think we should have a more general Axis to
replace ScrollEventAxis. Then I think that we would have versions of various
functions that take an axis parameter. This code would then be more like this:

    shouldHandleEvent = wheelEvent->delta(axis);

I think the Axis concept would be a low level graphics concept used by classes
such as the Point/Size classes. I think that some code, especially scrolling
code, could get a lot less copy and paste-y if we could share the same code for
horizontal and vertical. Might make things more complicated at first until it
eventually makes things simpler.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:161
> +bool ScrollingTreeFrameScrollingNodeMac::allowsHorizontalStretching(const
PlatformWheelEvent& wheelEvent)

If we had an axis type, then allowsHorizontalStretching and
allowsVerticalStretching could be combined into a single function and I think
it would be a lot easier to read.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:165
> +	   bool scrollbarsAllowStretching = hasEnabledHorizontalScrollbar() ||
!hasEnabledVerticalScrollbar();

This would be something like

    bool scrollbarsAllowStretching = hasEnabledScrollbar(axis) ||
!hasEnabledScrollbar(otherAxis(axis));

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:166
> +	   bool eventPreventsStretching = (wheelEvent.phase() ==
PlatformWheelEventPhaseMayBegin || wheelEvent.phase() ==
PlatformWheelEventPhaseBegan) && ((wheelEvent.deltaX() > 0 &&
scrollPosition().x() <= minimumScrollPosition().x()) || (wheelEvent.deltaX() <
0 && scrollPosition().x() >= maximumScrollPosition().x()));

I can’t help thinking that this would be easier to read with some helper
functions. Writing it all out like this on one line makes it really hard to
follow, but I think instead of line breaks it would be best to have helpers.

The whole expression could maybe be a helper, which I think would be really
great if we had an axis argument, since we could share it.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1140
> +	   bool scrollbarsAllowStretching = (((vScroller &&
vScroller->enabled()) || (!hScroller || !hScroller->enabled())));

Seems there is an extra set of parentheses around this.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1141
> +	   bool eventPreventsStretching = (wheelEvent.phase() ==
PlatformWheelEventPhaseMayBegin || wheelEvent.phase() ==
PlatformWheelEventPhaseBegan) && ((wheelEvent.deltaY() > 0 &&
m_scrollableArea->scrolledToTop()) || (wheelEvent.deltaY() < 0 &&
m_scrollableArea->scrolledToBottom()));

I’d love to see some way of sharing more of this with
ScrollingTreeFrameScrollingNodeMac since it’s the same kind of logic.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1160
> +	   bool scrollbarsAllowStretching = (((hScroller &&
hScroller->enabled()) || (!vScroller || !vScroller->enabled())));

Seems there is an extra set of parentheses around this.

> Source/WebCore/rendering/RenderLayer.cpp:2664
> +    int physicalScrollY = scrollPosition().y() + scrollOrigin().y();
> +    if (physicalScrollY < 0)
> +	   stretch.setHeight(physicalScrollY);
> +    else if (totalContentsSize().height() && physicalScrollY >
totalContentsSize().height() - visibleHeight())
> +	   stretch.setHeight(physicalScrollY - (totalContentsSize().height() -
visibleHeight()));
> +
> +    int physicalScrollX = scrollPosition().x() + scrollOrigin().x();
> +    if (physicalScrollX < 0)
> +	   stretch.setWidth(physicalScrollX);
> +    else if (scrollableContentsSize().width() && physicalScrollX >
scrollableContentsSize().width() - visibleWidth())
> +	   stretch.setWidth(physicalScrollX - (scrollableContentsSize().width()
- visibleWidth()));

Again, so clear that the axis technique would work well here so we didn’t have
to repeat all this code twice.

Except that one of these code paragraphs uses totalContentsSize and the other
uses scrollableContentsSize. Not sure why. Probably worth including a comment.