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

Levi Weintraub
Reported 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.
Attachments
Test Case (488 bytes, text/html)
2011-04-22 09:30 PDT, Levi Weintraub 		no flags
Details
Patch (53.46 KB, patch)
2011-04-22 10:10 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2011-04-22 09:30:36 PDT
Created attachment 90712 [details] Test Case
Levi Weintraub
Comment 2 2011-04-22 10:10:10 PDT
Created attachment 90716 [details] Patch
Levi Weintraub
Comment 3 2011-04-22 10:46:39 PDT
Could someone please take a look? This is a particularly nasty bug for RTL.
Eric Seidel (no email)
Comment 4 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.
Levi Weintraub
Comment 5 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>
Levi Weintraub
Comment 6 2011-04-22 11:53:23 PDT
All reviewed patches have been landed. Closing bug.
Arvid Nilsson
Comment 7 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?
Arvid Nilsson
Comment 8 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.
Arvid Nilsson
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.