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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91655





--- Comment #9 from Beth Dakin <bdakin at apple.com>  2014-08-20 11:53:16 PST ---
(In reply to comment #6)
> (From update of attachment 236832 [details])
> 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?
> 

Good question. I  believe that the answer is that this is very, very old code, and it made sense at he time it was written because nothing else was expected to happen during a scroll event if there is no delta to handle. The code moved a bunch of times, but I tracked it back to http://trac.webkit.org/changeset/40675 It's even older than that actually, but I don't think it's worth continuing to chase it. I think it's just from a time before complex scrolling.


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

I like this idea! But I am a little confused about what I should do now. It would be pretty easy for me to add PlatformWheelEvent::delta(ScrollEventAxis) right now, and I would be happy to do that. But it seems like you are envisioning something much more far-reaching, and maybe I should not act on this yet? Please advise. :-)

> > 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));
> 

Agreed. Still not sure if I should act on this now.

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

Agreed. I'll make this more readable.

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

Good eye! Fixed.

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

Yes, I'll give this some thought. 

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

Fixed.

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

Actually, I think that both can use scrollableContentSize now that I'm really thinking about it. I will confirm, and add a comment if it really needs to be different.

Thanks Darin!

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