Bug 91655 – overflow:scroll elements should support rubber-banding

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Drackett]

In Lion and Mountain Lion elements that scroll in the native UI allow you to scroll past the end. This is not the case for elements with overflow: scroll set. This makes them inconsistent with the rest of the platform even though they visually look the same.

[Beth Dakin]

*** Bug 136072 has been marked as a duplicate of this bug. ***

[Beth Dakin]

Created attachment 236832 [details]
Patch

There is at least one follow-up bug here, which is that it doesn't work properly for direction:rtl.

[Beth Dakin]

Comment on attachment 236832 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236832&action=review

> Source/WebCore/ChangeLog:11
> +        We cannot return early here is there is no scroll delta. There wonât be a scroll 

I fixed the encoding stuff locally.

[Brent Fulgham]

Comment on attachment 236832 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236832&action=review

I don't see any obvious reason why these changes would break latching. Do you have a case where it seems broken? These changes look good to me.

>> Source/WebCore/ChangeLog:11
>> +        We cannot return early here is there is no scroll delta. There wonât be a scroll 
> 
> I fixed the encoding stuff locally.

"is there is" maybe should be "if there is"?

[Brent Fulgham]

This looks great on my system, and seems to work properly. I don't see any problems with my manual latching examples stuff.

I can't wait for <iframe> to work as well!  :-)

[Darin Adler]

Comment on attachment 236832 [details]
Patch

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.

[Build Bot]

Comment on attachment 236832 [details]
Patch

Attachment 236832 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5902400791511040

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-div.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-select-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-iframe-with-handler.html
platform/mac/fast/scrolling/scroll-latched-nested-div.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-div-with-handler.html

[Build Bot]

Created attachment 236886 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Beth Dakin]

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

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 236832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236832&action=review
> 
> I don't see any obvious reason why these changes would break latching. Do you have a case where it seems broken? These changes look good to me.
> 
> >> Source/WebCore/ChangeLog:11
> >> +        We cannot return early here is there is no scroll delta. There wonât be a scroll 
> > 
> > I fixed the encoding stuff locally.
> 
> "is there is" maybe should be "if there is"?

Good catch! Thanks Brent!

[Beth Dakin]

(In reply to comment #7)
> (From update of attachment 236832 [details])
> Attachment 236832 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/5902400791511040
> 
> New failing tests:
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe-with-handler.html
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe-with-handler.html
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-div.html
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-select-with-handler.html
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-iframe-latched-iframe-with-handler.html
> platform/mac/fast/scrolling/scroll-latched-nested-div.html
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-select-latched-mainframe-with-handler.html
> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-div-with-handler.html

I believe that the WK2 tests just need updated results. Most of the differences are due to the fact that the 'end' phase is now handled. I am still investigating platform/mac/fast/scrolling/scroll-latched-nested-div.html which is confusing me a little.

[Brent Fulgham]

(In reply to comment #7)
The only test failure that looks "real" to me is this one:

> platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-div-with-handler.html

[Brent Fulgham]

(In reply to comment #12)
> (In reply to comment #7)
> The only test failure that looks "real" to me is this one:
> 
> > platform/mac-wk2/tiled-drawing/scrolling/fast-scroll-div-latched-div-with-handler.html

Sorry: I meant to say "scroll-latched-nested-div.html".

[Beth Dakin]

Created attachment 236934 [details]
Patch

Here's a new patch that addresses most of Darin's comments, re-baselines/fixes the failing tests, AND it disables the feature for WK1 for the time being. I filed https://bugs.webkit.org/show_bug.cgi?id=136131 so that WK1 can be addressed in a follow-up patch.

[Beth Dakin]

Thanks Sam! http://trac.webkit.org/changeset/172832

[Carlos Alberto Lopez Perez]

(In reply to comment #15)
> Thanks Sam! http://trac.webkit.org/changeset/172832

This broke both the GTK and EFL builds:

GTK: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/50324/steps/compile-webkit/logs/stdio
EFL: http://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release%20%28Build%29/builds/6112/steps/compile-webkit/logs/stdio

[Gyuyoung Kim]

(In reply to comment #16)
> (In reply to comment #15)
> > Thanks Sam! http://trac.webkit.org/changeset/172832
> 
> This broke both the GTK and EFL builds:
> 
> GTK: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/50324/steps/compile-webkit/logs/stdio
> EFL: http://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release%20%28Build%29/builds/6112/steps/compile-webkit/logs/stdio

r172842 fixed this build break for EFL and GTK ports.
http://trac.webkit.org/changeset/172842

[Jon Lee]

This caused an iOS build failure, tracked by bug 136157.