Bug 118176 - constrainScrollPositionForOverhang needs to handle scrollOrigin correctly
Summary: constrainScrollPositionForOverhang needs to handle scrollOrigin correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks: 118494
  Show dependency treegraph
 
Reported: 2013-06-28 02:35 PDT by Tim Horton
Modified: 2013-07-09 01:05 PDT (History)
12 users (show)

See Also:


Attachments
preliminary (5.57 KB, patch)
2013-06-28 03:22 PDT, Tim Horton
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (731.03 KB, application/zip)
2013-06-28 05:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (610.44 KB, application/zip)
2013-06-28 09:53 PDT, Build Bot
no flags Details
patch (5.37 KB, patch)
2013-07-01 16:49 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (12.60 KB, patch)
2013-07-01 18:47 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch with more bits (13.28 KB, patch)
2013-07-01 18:50 PDT, Tim Horton
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (553.83 KB, application/zip)
2013-07-01 20:18 PDT, Build Bot
no flags Details
patch with a sign flipped (13.28 KB, patch)
2013-07-01 22:29 PDT, Tim Horton
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (652.18 KB, application/zip)
2013-07-02 00:00 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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