Bug 108323 - Improve the logic in WebCore::scrollOffsetForFixedPosition()
Summary: Improve the logic in WebCore::scrollOffsetForFixedPosition()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arvid Nilsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-30 03:54 PST by Arvid Nilsson
Modified: 2022-09-23 15:13 PDT (History)
24 users (show)

See Also:


Attachments
Patch (3.91 KB, patch)
2013-01-30 04:34 PST, Arvid Nilsson
darin: review-
Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2013-01-30 05:12 PST, Arvid Nilsson
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Nilsson 2013-01-30 03:54:28 PST
The code in WebCore::scrollOffsetForFixedPosition() is correct if the layout viewport is equal in size to the visual viewport. In desktop ports of WebKit, these are always equal.

However, there are features in WebCore::ScrollView to separate these two, a strategy popular with mobile ports of WebKit.

virtual void ScrollView::setFixedVisibleContentRect(const IntRect& visibleContentRect)

and

void ScrollView::setFixedLayoutSize(const IntSize&)
void ScrollView::setUseFixedLayout(bool enable)

can be used to cause the visual viewport and the layout viewport to differ. In that case, WebCore::scrollOffsetForFixedPosition() can return incorrect results for RTL pages, causing left property to be broken with position:fixed elements.

See also my comments https://bugs.webkit.org/show_bug.cgi?id=59204 for some background.
Comment 1 Arvid Nilsson 2013-01-30 04:34:56 PST
Created attachment 185466 [details]
Patch
Comment 2 Arvid Nilsson 2013-01-30 04:40:57 PST
Comment on attachment 185466 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:77
> +        scrollPosition = maxValue - scrollOrigin;

I believe the reason the old code worked for desktop ports is that

maxValue = (contentsSize - visibleContentsSize) and scrollOrigin = (contentsSize - fixedLayoutSize)

When the layout viewport (fixedLayoutSize) is equal to the visual viewport (visibleContentsSize), which is the case when ScrollView::useFixedLayoutSize is false, my proposed expressions reduce to what used to be in there before.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:84
> +    // mobile ports to use fixedElementsLayoutRelativeToFrame based on emerging consensus on how position:fixed should work on mobile.

Maybe someone from the Mac OS X port has an idea if and how the dragFactor code could be extrapolated to the case where layout viewport != visual viewport. Otherwise I think it would be OK to just preserve the current behavior.
Comment 3 Arvid Nilsson 2013-01-30 04:44:20 PST
Comment on attachment 185466 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:70
>  static int fixedPositionScrollOffset(int visibleContentSize, int contentsSize, int scrollPosition, int scrollOrigin, float frameScaleFactor, bool fixedElementsLayoutRelativeToFrame)

Another option, instead of duplicating the logic in ScrollView.cpp, would be to pass the FrameView* to the scrollOffsetForFixedPosition() method and use the following expression instead:

    IntPoint newScrollPosition = scrollPoint.shrunkTo(frameView->maximumScrollPosition());
    newScrollPosition = newScrollPosition.expandedTo(frameView->minimumScrollPosition());
Comment 4 Arvid Nilsson 2013-01-30 04:55:11 PST
Sorry about the layout vs. visual viewport terminology, I got it from http://www.quirksmode.org/blog/archives/2010/06/a_tale_of_two_v_1.html. See also http://www.quirksmode.org/blog/archives/2012/10/budding_consens.html for the reason why I figured dragFactor may not be important for mobile ports.
Comment 5 Arvid Nilsson 2013-01-30 05:12:47 PST
Created attachment 185472 [details]
Patch

Here's an alternate approach that tries to reuse the code in ScrollView instead of duplicating it
Comment 6 Kenneth Rohde Christiansen 2013-01-30 05:15:42 PST
Comment on attachment 185472 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:95
> +    IntRect visibleContentRect = frameView->visibleContentRect();

keep in mind that visibleContentRect for Qt/EFL etc is actually the visiblecontentrect and not the layout rect. I dont know how that works for BB
Comment 7 Arvid Nilsson 2013-01-30 05:18:04 PST
(In reply to comment #6)
> (From update of attachment 185472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185472&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:95
> > +    IntRect visibleContentRect = frameView->visibleContentRect();
> 
> keep in mind that visibleContentRect for Qt/EFL etc is actually the visiblecontentrect and not the layout rect. I dont know how that works for BB

Yes I know, that's the whole idea of this patch, to cater to the case where the visibleContentRect != layoutRect. The layout rect is already encoded in the scrollOrigin (scrollOrigin = contentsSize - layoutRect) so we don't need to explicitly include it in the calculations.
Comment 8 Build Bot 2013-01-31 02:24:05 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16267108
Comment 9 Early Warning System Bot 2013-01-31 02:33:07 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16266146
Comment 10 Early Warning System Bot 2013-01-31 02:37:05 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16267115
Comment 11 EFL EWS Bot 2013-01-31 02:46:29 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16271155
Comment 12 Peter Beverloo (cr-android ews) 2013-01-31 03:05:37 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16273130
Comment 13 WebKit Review Bot 2013-01-31 03:16:45 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16266161
Comment 14 kov's GTK+ EWS bot 2013-01-31 03:52:48 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16269143
Comment 15 WebKit Review Bot 2013-01-31 04:52:14 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16280159
Comment 16 Build Bot 2013-01-31 08:13:16 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16278240
Comment 17 Build Bot 2013-01-31 22:30:07 PST
Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16299398
Comment 18 Levi Weintraub 2013-02-04 17:16:55 PST
I'm confused which patch I should be looking at... The first one that passes the build bots?
Comment 19 Tien-Ren Chen 2013-02-04 18:02:42 PST
(In reply to comment #0)
> The code in WebCore::scrollOffsetForFixedPosition() is correct if the layout viewport is equal in size to the visual viewport. In desktop ports of WebKit, these are always equal.
> 
> However, there are features in WebCore::ScrollView to separate these two, a strategy popular with mobile ports of WebKit.
> 
> virtual void ScrollView::setFixedVisibleContentRect(const IntRect& visibleContentRect)
> 
> and
> 
> void ScrollView::setFixedLayoutSize(const IntSize&)
> void ScrollView::setUseFixedLayout(bool enable)
> 
> can be used to cause the visual viewport and the layout viewport to differ. In that case, WebCore::scrollOffsetForFixedPosition() can return incorrect results for RTL pages, causing left property to be broken with position:fixed elements.
> 
> See also my comments https://bugs.webkit.org/show_bug.cgi?id=59204 for some background.

To make things more confusing, there are actually 3 viewports which can be all different at the same time.

1. The fixed layout size, which controls the size of the top containing block (RenderView I believe). Which also affects the layout of fixed-position elements when fixedElementsLayoutRelativeToFrame == false. This value is only effective on mobile builds and is determined by the viewport tag.

2. The visible content rect, which indicates the current visible part of the frame content, in absolute coordinate. For chromium-android (and for other platforms now migrating to applyPageScaleFactorInCompositor == true) this rect will be affected by the current page scale factor. (visibleContentSize = windowSize / pageScaleFactor)

This rect can be over-scrolled negatively or positively on iOS. (correct me if I misunderstood)

This affects the layout of fixed-position elements when fixedElementsLayoutRelativeToFrame == true

3. The viewport constrained visible content rect. This one is similar to visible content rect, but will be clamped to zero or max scroll position, so fixed-position element won't be over-scrolled. For chromium-android, we have a more aggressive plan known as "desktop compatibility viewport" so we have more complicated control to the layout of fixed-position elements. 

My opinion is that we should make FrameView::viewportConstrainedVisibleContentRect the only authoritative value for fixed-position layout when fixedElementsLayoutRelativeToFrame == true. (and RenderBox::containingBlockLogical(Width|Height)ForPositioned should use it instead of FrameView::visibleContentRect as well).

I don't 100% understand how RenderBox::containingBlockLogical(Width|Height)ForPositioned works. Perhaps we can completely hide these behavioral differences in FrameView so we just feed the standard value to viewportConstrainedVisibleContentRect when fixedElementsLayoutRelativeToFrame == false.
Comment 20 Tien-Ren Chen 2013-02-04 18:43:06 PST
visualization for the whole mess: http://goo.gl/XERxA
Comment 21 Arvid Nilsson 2013-02-04 20:53:26 PST
Comment on attachment 185472 [details]
Patch

Marking as obsolete to avoid confusion.
Comment 22 Dongseong Hwang 2013-02-04 21:32:04 PST
Hi, folks!

Please who can explain what is drag factor and which ports use it?

In my understanding,
applyDragFactorForFixedPosition = scrollPosition x (1 + (1/frameScale - 1) / (1 - visibleContentsSize / contentsSize))
How this complex formula can be derived?
Comment 23 Arvid Nilsson 2013-02-06 01:21:25 PST
(In reply to comment #19)
> (In reply to comment #0)
> > The code in WebCore::scrollOffsetForFixedPosition() is correct if the layout viewport is equal in size to the visual viewport. In desktop ports of WebKit, these are always equal.
> > 
> > However, there are features in WebCore::ScrollView to separate these two, a strategy popular with mobile ports of WebKit.
> > 
> > virtual void ScrollView::setFixedVisibleContentRect(const IntRect& visibleContentRect)
> > 
> > and
> > 
> > void ScrollView::setFixedLayoutSize(const IntSize&)
> > void ScrollView::setUseFixedLayout(bool enable)
> > 
> > can be used to cause the visual viewport and the layout viewport to differ. In that case, WebCore::scrollOffsetForFixedPosition() can return incorrect results for RTL pages, causing left property to be broken with position:fixed elements.
> > 
> > See also my comments https://bugs.webkit.org/show_bug.cgi?id=59204 for some background.
> 
> To make things more confusing, there are actually 3 viewports which can be all different at the same time.
> 
> 1. The fixed layout size, which controls the size of the top containing block (RenderView I believe). Which also affects the layout of fixed-position elements when fixedElementsLayoutRelativeToFrame == false. This value is only effective on mobile builds and is determined by the viewport tag.
> 
> 2. The visible content rect, which indicates the current visible part of the frame content, in absolute coordinate. For chromium-android (and for other platforms now migrating to applyPageScaleFactorInCompositor == true) this rect will be affected by the current page scale factor. (visibleContentSize = windowSize / pageScaleFactor)
> 
> This rect can be over-scrolled negatively or positively on iOS. (correct me if I misunderstood)
> 
> This affects the layout of fixed-position elements when fixedElementsLayoutRelativeToFrame == true
> 
> 3. The viewport constrained visible content rect. This one is similar to visible content rect, but will be clamped to zero or max scroll position, so fixed-position element won't be over-scrolled. For chromium-android, we have a more aggressive plan known as "desktop compatibility viewport" so we have more complicated control to the layout of fixed-position elements. 
> 
> My opinion is that we should make FrameView::viewportConstrainedVisibleContentRect the only authoritative value for fixed-position layout when fixedElementsLayoutRelativeToFrame == true. (and RenderBox::containingBlockLogical(Width|Height)ForPositioned should use it instead of FrameView::visibleContentRect as well).
> 
> I don't 100% understand how RenderBox::containingBlockLogical(Width|Height)ForPositioned works. Perhaps we can completely hide these behavioral differences in FrameView so we just feed the standard value to viewportConstrainedVisibleContentRect when fixedElementsLayoutRelativeToFrame == false.

Thanks for your comments! I didn't think of the viewport constrained visible content rect as a separate thing, but a natural consequence of overscrolling. You're right that this is the rect we want to keep fixed pos elements inside, and I believe that is what the scrollOffsetForFixedPosition code was trying to do (but failed for RTL pages).
Comment 24 Jeremy Moskovich 2013-02-18 04:00:22 PST
Anillson: Do you think this is the same issue as crbug.com/129746 ?
Comment 25 Darin Adler 2014-07-12 16:48:47 PDT
Comment on attachment 185466 [details]
Patch

This function no longer exists. The concept from this patch might still make sense, but review- to get this out of the review queue.
Comment 26 Ahmad Saleem 2022-09-23 15:13:39 PDT
I tried to land this using Chromium / Blink patch in following PR:

https://github.com/WebKit/WebKit/pull/4529

(It compiles... thanks to Alan for his help)

It leads to following issues:

1) Failure in one of subtest of scrollbars/rtl/div-absolute.html test case, even if I try to copy latest from Chromium / Blink, there are still failures:

FAIL Bug 91756: Test if the widths of RTL elements are the same as the widths of the LTR elements when they include absolutely-positioned children. assert_equals: expected 206 but got 221

2) Failure in WPT testcase - imported/w3c/web-platform-tests/css/css-scroll-snap/snap-inline-block.html and following subtests fail:

FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on y expected 300 but got 315
FAIL Snaps correctly for horizontal-tb writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected 115 but got 0
FAIL Snaps correctly for vertical-lr writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected 300 but got 0
FAIL Snaps correctly for vertical-rl writing mode with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on y expected 165 but got 180
FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' alignment assert_equals: aligns correctly on x expected -500 but got -315
FAIL Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' alignment assert_equals: aligns correctly on x expected -315 but got -500

Fixing them are beyond my expertise, so I am going to close my PR and note down the information for someone else to fix it. Thanks!