[Webkit-unassigned] [Bug 91655] overflow:scroll elements should support rubber-banding

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 20 10:30:39 PDT 2014


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
 Attachment #236832|review?                     |review+
               Flag|                            |

--- Comment #6 from Darin Adler <darin at apple.com>  2014-08-20 10:30:40 PST ---
(From update of attachment 236832)
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.

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list