Bug 118176

Summary: constrainScrollPositionForOverhang needs to handle scrollOrigin correctly
Product: WebKit Reporter: Tim Horton <thorton>
Component: Layout and RenderingAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, buildbot, cmarcelo, commit-queue, hyatt, jamesr, luiz, rniwa, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 118494    
Attachments:
Description Flags
preliminary
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
patch
none
patch
none
patch with more bits
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
patch with a sign flipped
andersca: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion none

Description Tim Horton 2013-06-28 02:35:02 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-06-28 02:35:51 PDT
<rdar://problem/14301271>
Comment 2 Tim Horton 2013-06-28 03:22:27 PDT
Created attachment 205691 [details]
preliminary
Comment 3 WebKit Commit Bot 2013-06-28 03:24:58 PDT
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.
Comment 4 Tim Horton 2013-06-28 03:29:01 PDT
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.
Comment 5 Build Bot 2013-06-28 05:30:02 PDT
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
Comment 6 Build Bot 2013-06-28 05:30:04 PDT
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
Comment 7 Build Bot 2013-06-28 09:53:29 PDT
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
Comment 8 Build Bot 2013-06-28 09:53:31 PDT
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
Comment 9 Tim Horton 2013-07-01 16:49:18 PDT
Created attachment 205845 [details]
patch

up next, a changelog and a test
Comment 10 Tim Horton 2013-07-01 17:54:10 PDT
I have a nice test but https://bugs.webkit.org/show_bug.cgi?id=118269
Comment 11 Tim Horton 2013-07-01 18:47:05 PDT
Created attachment 205849 [details]
patch
Comment 12 Tim Horton 2013-07-01 18:50:21 PDT
Created attachment 205851 [details]
patch with more bits
Comment 13 Build Bot 2013-07-01 20:18:33 PDT
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
Comment 14 Build Bot 2013-07-01 20:18:35 PDT
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
Comment 15 Tim Horton 2013-07-01 21:48:05 PDT
Hmm, I forgot to re-test banners after making the adjustments to unbreak those other three tests. I'll see what's up.
Comment 16 Tim Horton 2013-07-01 22:29:05 PDT
Created attachment 205865 [details]
patch with a sign flipped

made a typo during cleanup
Comment 17 Build Bot 2013-07-02 00:00:11 PDT
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
Comment 18 Build Bot 2013-07-02 00:00:14 PDT
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
Comment 19 Tim Horton 2013-07-02 00:13:27 PDT
(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...
Comment 20 Tim Horton 2013-07-02 11:15:47 PDT
http://trac.webkit.org/changeset/152303