Bug 59204

Summary: REGRESSION: left property broken with position:fixed elements in RTL documents
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, eric, hyatt, jamesr, mitz, rniwa, sam, simon.fraser, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test Case
none
Patch none

Description Levi Weintraub 2011-04-22 09:27:31 PDT
Copied from Chromium bug 80216 (http://code.google.com/p/chromium/issues/detail?id=80216)

What steps will reproduce the problem?
1. Create an RTL document (<html dir="rtl">); create a position:fixed element within that document; set its "top" and "left" attributes.
2. Scroll left/right within the document.

What is the expected result?

The position:fixed element should float with a constant left offset.

What happens instead?

The position:fixed element moves horizontally as you scroll.
Comment 1 Levi Weintraub 2011-04-22 09:30:36 PDT
Created attachment 90712 [details]
Test Case
Comment 2 Levi Weintraub 2011-04-22 10:10:10 PDT
Created attachment 90716 [details]
Patch
Comment 3 Levi Weintraub 2011-04-22 10:46:39 PDT
Could someone please take a look? This is a particularly nasty bug for RTL.
Comment 4 Eric Seidel (no email) 2011-04-22 11:11:27 PDT
Comment on attachment 90716 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1116
> +    if (!ScrollView::scrollOrigin().x()) {
> +        if (x < 0)
> +            x = 0;
> +        else if (x > maxX)
> +            x = maxX;
> +    } else {
> +        if (x > 0)
> +            x = 0;
> +        else if (x < -maxX)
> +            x = -maxX;
> +    }

Can't we write this more concisely?

> Source/WebCore/page/FrameView.cpp:1151
> +    if (!ScrollView::scrollOrigin().y()) {
> +        if (y < 0)
> +            y = 0;
> +        else if (y > maxY)
> +            y = maxY;
> +    } else {
> +        if (y > 0)
> +            y = 0;
> +        else if (y < -maxY)
> +            y = -maxY;
> +    }

Wow, again.
Comment 5 Levi Weintraub 2011-04-22 11:53:18 PDT
Comment on attachment 90716 [details]
Patch

Clearing flags on attachment: 90716

Committed r84655: <http://trac.webkit.org/changeset/84655>
Comment 6 Levi Weintraub 2011-04-22 11:53:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Arvid Nilsson 2013-01-29 06:29:20 PST
Hi there!

I'm investigating some rendering problems with fixed pos on RTL pages in the BlackBerry port. I don't quite understand why you did this:

 1106    if (!ScrollView::scrollOrigin().x()) {
 1107        if (x < 0)
 1108            x = 0;
 1109        else if (x > maxX)
 1110            x = maxX;
 1111    } else {
 1112        if (x > 0)
 1113            x = 0;
 1114        else if (x < -maxX)
 1115            x = -maxX;
 1116    }

Instead of this:

if (x < minimumScrollPosition().x())
    x = minimumScrollPosition().x();
else if (x > maximumScrollPosition().x())
    x = maximumScrollPosition().x();

or equivalent. For reference, 

IntPoint ScrollView::maximumScrollPosition() const
{
    IntPoint maximumOffset(contentsWidth() - visibleWidth() - scrollOrigin().x(), contentsHeight() - visibleHeight() - scrollOrigin().y());
    maximumOffset.clampNegativeToZero();
    return maximumOffset;
}

IntPoint ScrollView::minimumScrollPosition() const
{
    return IntPoint(-scrollOrigin().x(), -scrollOrigin().y());
}

On an RTL page, there's nothing special about the 0 and -maxX scroll positions AFAICT. For certain combinations of viewport and content size, 0 can be close to the maximumScrollPosition, and -maxX can be close to the minimumScrollPosition, but that would be a coincidence.

Can you tell me some more details about why you chose to clamp to [-maxX, 0] instead of [minimumScrollPosition().x(), maximumScrollPosition().x()] here?
Comment 8 Arvid Nilsson 2013-01-30 02:38:02 PST
Thinking about this some more, after trying (and failing) to reproduce the bug in Chrome, it seems on a desktop port of WebKit like Chrome or Safari, the maximum scroll position for this purpose is always 0 and the minimum is always -maxValue. In a mobile port of WebKit like the BlackBerry port, the layout width varies independently of the visible contents width, while on desktop they are always the same.

The correct formula

if (scrollPosition > maxValue - scrollOrigin)
    scrollPosition = maxValue - scrollOrigin;

expands to

if (scrollPosition > contentsSize - visibleSize - (contentsSize - layoutSize)

which reduces to

scrollPosition > 0

when visibleSize == layoutSize. So Levi's code always yields the correct result for desktop ports.
Comment 9 Arvid Nilsson 2013-01-30 03:55:09 PST
I filed https://bugs.webkit.org/show_bug.cgi?id=108323 to potentially fix the problem I think I see here.