Bug 118176 – constrainScrollPositionForOverhang needs to handle scrollOrigin correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tim Horton]

As in https://bugs.webkit.org/show_bug.cgi?id=118175: when paginated right-to-left or bottom-to-top, the vast majority of the content is in the negative overflow even if the view is capable of fitting the entire document. This was deemed too hard to fix, so instead we need to enable effective scrolling "into the negative" in a few small places.

Another such place is constrainScrollPositionForOverhang, which currently is rather scrollOrigin unfriendly in general (fixed position is broken in RTL, etc.)

I've reimplemented it based on rect intersection, which seems to make sense on paper, but I'd be up for a discussion about it. It seems a bit more hackable and less confusing, though is probably slightly more expensive in the normal case.

[Radar WebKit Bug Importer]

<rdar://problem/14301271>

[Tim Horton]

Created attachment 205691 [details]
preliminary

[WebKit Commit Bot]

Attachment 205691 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm', u'Source/WebCore/platform/ScrollableArea.cpp']" exit_code: 1
Source/WebCore/platform/ScrollableArea.cpp:440:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 205691 [details]
preliminary

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:315
> +    IntSize scrollOffsetForFixedChildren = FrameView::scrollOffsetForFixedPosition(viewportRect(), totalContentsSize(), scrollOffset, scrollOrigin(), frameScaleFactor(), false, headerHeight(), footerHeight());

Suddenly, position: fixed in RTL works again :P

Also, scrollOffset is the wrong name.

> Source/WebCore/platform/ScrollableArea.cpp:417
> +    IntRect scrollRect(scrollPosition.x() + scrollOrigin.x(), scrollPosition.y() + scrollOrigin.y() - headerHeight, visibleContentRect.width(), visibleContentRect.height());

There's a more effective way to write this line.

>> Source/WebCore/platform/ScrollableArea.cpp:440
>> +    return constrainScrollPositionForOverhang(visibleContentRect(), totalContentsSize(), scrollPosition, scrollOrigin(), headerHeight(), footerHeight());;
> 
> More than one command on the same line  [whitespace/newline] [4]

What a strange typo.

[Build Bot]

Comment on attachment 205691 [details]
preliminary

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

New failing tests:
compositing/geometry/fixed-position-composited-page-scale-smaller-than-viewport.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html

[Build Bot]

Created attachment 205702 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

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

[Build Bot]

Comment on attachment 205691 [details]
preliminary

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

New failing tests:
compositing/geometry/fixed-position-composited-page-scale-smaller-than-viewport.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html

[Build Bot]

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

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

[Tim Horton]

Created attachment 205845 [details]
patch

up next, a changelog and a test

[Tim Horton]

I have a nice test but https://bugs.webkit.org/show_bug.cgi?id=118269

[Tim Horton]

Created attachment 205849 [details]
patch

[Tim Horton]

Created attachment 205851 [details]
patch with more bits

[Build Bot]

Comment on attachment 205851 [details]
patch with more bits

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

New failing tests:
platform/mac-wk2/tiled-drawing/fixed/four-bars-with-header-and-footer.html

[Build Bot]

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

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

[Tim Horton]

Hmm, I forgot to re-test banners after making the adjustments to unbreak those other three tests. I'll see what's up.

[Tim Horton]

Created attachment 205865 [details]
patch with a sign flipped

made a typo during cleanup

[Build Bot]

Comment on attachment 205865 [details]
patch with a sign flipped

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

New failing tests:
svg/batik/filters/feTile.svg

[Build Bot]

Created attachment 205873 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

[Tim Horton]

(In reply to comment #17)
> (From update of attachment 205865 [details])
> Attachment 205865 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/1019025
> 
> New failing tests:
> svg/batik/filters/feTile.svg

Not. My. Department. Oh, wait...

[Tim Horton]

http://trac.webkit.org/changeset/152303