Bug 108323 – Improve the logic in WebCore::scrollOffsetForFixedPosition()

• Component: Layout and Rendering
• Status: NEW

[Arvid Nilsson]

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.

[Arvid Nilsson]

Created attachment 185466 [details]
Patch

[Arvid Nilsson]

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.

[Arvid Nilsson]

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());

[Arvid Nilsson]

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.

[Arvid Nilsson]

Created attachment 185472 [details]
Patch

Here's an alternate approach that tries to reuse the code in ScrollView instead of duplicating it

[Kenneth Rohde Christiansen]

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

[Arvid Nilsson]

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

[Build Bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16267108

[Early Warning System Bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16266146

[Early Warning System Bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16267115

[EFL EWS Bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16271155

[Peter Beverloo (cr-android ews)]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16273130

[WebKit Review Bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16266161

[kov's GTK+ EWS bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16269143

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16278240

[Build Bot]

Comment on attachment 185472 [details]
Patch

Attachment 185472 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16299398

[Levi Weintraub]

I'm confused which patch I should be looking at... The first one that passes the build bots?

[Tien-Ren Chen]

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

[Tien-Ren Chen]

visualization for the whole mess: http://goo.gl/XERxA

[Arvid Nilsson]

Comment on attachment 185472 [details]
Patch

Marking as obsolete to avoid confusion.

[Dongseong Hwang]

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?

[Arvid Nilsson]

(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).

[Jeremy Moskovich]

Anillson: Do you think this is the same issue as crbug.com/129746 ?

[Darin Adler]

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.

[Ahmad Saleem]

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!