Bug 70395 – REGRESSION: rtl horizontal scrollbar / resize bug - Body shifts on resize when scrolled all the way to the left

• Component: Layout and Rendering
• Status: RESOLVED FIXED

Created attachment 111555 [details]
Simple Repro File

NOTE: I am not sure about the webkit version. In Chrome, navigator.userAgent has  AppleWebKit/535.1

What steps will reproduce the problem?
1. Open RtlContentShift.html
2. Resize browser so that the content window is less than 500 px wide.
3. Scroll to the left all the way.
4. Resize browser, increasing the width.

Browsers tested:
     Chrome 13.0.782.112: FAIL
     Safari 5: FAIL
     Firefox 7.x: N/A - scrolling appears to be disabled in rtl
     IE 7: MIXED - the body still covers the whole window, but the rest of the layout is not correct
     IE 8/9: OK

What is the expected result?
The body still covers the entire window, since it has width:100%;height:100%

What happens instead?
The body is shifted over by the amount that the window was resized.

This bug originated in the chrome bug database. See http://code.google.com/p/chromium/issues/detail?id=100612&q=rtl%20scroll&colspec=ID%20Stars%20Pri%20Area%20Feature%20Type%20Status%20Summary%20Modified%20Owner%20Mstone%20OS#makechanges

[Xiaomei Ji]

It appears regressed in http://trac.webkit.org/changeset/76395/trunk.

If I remove the condition checking in ScrollAnimator::scrollToOffsetWithoutAnimation(), the test case works ok (tested in chromium, Safari in Mac works fine since it has its own ScrollAnimatorMac.mm, in which, ScollTo() is always called without condition).

But I am not sure removing the condition checking is the right fix.
Would appreciate comments from mitz (who added ScrollView.cpp#L586) and/or weinig (who is more familiar with ScrollAnimation).

The reason of the failure is that:
when horizontal scrollbar is at leftmost, ScrollAnimator::m_currentPosX/Y == 0.

when enlarge browser size, following is part of the call stack:
        ......
 	ScrollView::updateScrollbars(...)
 	ScrollView::setScrollOrigin(...)
 	FrameView::adjustViewSize(...)
        .....

Assume m_scrollOrigin.x was 200 previously, and is 150 now, at inline http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ScrollView.cpp#L586
desiredOffset is the old scroll offset, which is -200, and adjustScrollPositionWithinRange() will adjust it within the range of (-150, 0) using current m_scrollOrigin, and scrollPoint is always >= 0.

After the above line, the scrollPoint passed to ScrollAnimator::scrollToOffsetWithoutAnimation() is (0, 0), since it is the same as m_currentPosX/Y, notifyPositionChanged() will *not* be called, so FrameView::scrollTo() using current m_scrollOrigin etc. are not called, caused wrong/mis-match of scroll position with document body.

It looks to me that ScrollAnimator::m_currentPosX and the parameter passing to ScrollAnimator::scrollToOffsetWithoutAnimation() are positive values even the scroll positions are negative values for RTL pages. I do not know the reason, but if that is the case, ScrollView.cpp#L58 seems doing the right thing, and we probably have to always scroll without condition checking.

[Tony Chang]

For reference, that code was to fix a regression caught by a chromium browser test.  The original bug is here: http://code.google.com/p/chromium/issues/detail?id=70408

[Xiaomei Ji]

The condition at following line seems wrong to me.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ScrollView.cpp#L587
I think we should compare adjustScrollPositionWithinRange(IntPoint(desiredOffset)) with scrollPosition() which are in the same coordinates (both negative for RTL pages).

After which, we do not (and should not) need to check the condition anymore in ScrollAnimator::scrollToOffsetWithoutAnimation().

I will need to run chromium's test case tony mentioned in #3.


Index: ScrollAnimator.cpp
===================================================================
--- ScrollAnimator.cpp  (revision 96967)
+++ ScrollAnimator.cpp  (working copy)
@@ -74,11 +74,11 @@

 void ScrollAnimator::scrollToOffsetWithoutAnimation(const FloatPoint& offset)
 {
-    if (m_currentPosX != offset.x() || m_currentPosY != offset.y()) {
+    //if (m_currentPosX != offset.x() || m_currentPosY != offset.y()) {
         m_currentPosX = offset.x();
         m_currentPosY = offset.y();
         notifyPositionChanged();
-    }
+    //}
 }

 bool ScrollAnimator::handleWheelEvent(const PlatformWheelEvent& e)
Index: ScrollView.cpp
===================================================================
--- ScrollView.cpp      (revision 96967)
+++ ScrollView.cpp      (working copy)
@@ -582,7 +582,8 @@
     }

     IntPoint scrollPoint = adjustScrollPositionWithinRange(IntPoint(desiredOffs
et)) + IntSize(m_scrollOrigin.x(), m_scrollOrigin.y());
-    if (scrollPoint != scrollPosition())
+//    if (scrollPoint != scrollPosition())
+    if (adjustScrollPositionWithinRange(IntPoint(desiredOffset)) != scrollPosit
ion())
         ScrollableArea::scrollToOffsetWithoutAnimation(scrollPoint);

     // Make sure the scrollbar offsets are up to date.

[Xiaomei Ji]

hm... with ScrollAnimator change, but with or without the ScrollView change mentioned in comment #4, RenderViewTest.OnNavStateChanged passed in my local chrome win build.

[Xiaomei Ji]

(In reply to comment #5)
> hm... with ScrollAnimator change, but with or without the ScrollView change mentioned in comment #4, RenderViewTest.OnNavStateChanged passed in my local chrome win build.

For  LTR page,
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ScrollView.cpp#L587 filters those un-necessary scrolls correctly, that is why the test pass even without change to ScrolView.cpp as in comment #4.

[Xiaomei Ji]

Created attachment 112038 [details]
patch

The test here does not add much value because:
1. Mac in safari works fine previously.
2. loading the test itself in Safari in Mac resize the browser window, but the png result seems not reflecting that (is it a DRT problem?). Without png result reflecting window resize, the test is not really useful.
3. Mac buildbot does not run png test. Chrome does, but chrome does not support resize top-level window, so this test wont be really useful to catch regression.

I'd appreciate if you have any suggestions on testing.

[Tony Chang]

Comment on attachment 112038 [details]
patch

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

What do you mean that chromium DRT doesn't support resizing the main window?  It looks like chromium passes the fast/dom/Window/window-resize* tests.

> LayoutTests/fast/dom/rtl-scroll-to-leftmost-and-resize.html:9
> +    window.resizeTo(350, window.innerHeight);

Does the bug repro if you resize the window smaller then resize it back to 800x600?

[WebKit Review Bot]

Comment on attachment 112038 [details]
patch

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

New failing tests:
fast/dom/horizontal-scrollbar-in-rtl.html

[Xiaomei Ji]

(In reply to comment #8)
> (From update of attachment 112038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112038&action=review
> 
> What do you mean that chromium DRT doesn't support resizing the main window?  It looks like chromium passes the fast/dom/Window/window-resize* tests.

Good point!
I did not try chromium DRT, I mean chromium itself does not support top-level window resize. 
If I load this test in chromium directly, it wont resize the window.
I will try DRT.

> 
> > LayoutTests/fast/dom/rtl-scroll-to-leftmost-and-resize.html:9
> > +    window.resizeTo(350, window.innerHeight);
> 
> Does the bug repro if you resize the window smaller then resize it back to 800x600?

the bug repro if I do it manually.

[Xiaomei Ji]

Created attachment 112630 [details]
wip

[Xiaomei Ji]

Created attachment 112805 [details]
patch w/ layout test

[Tony Chang]

Comment on attachment 112805 [details]
patch w/ layout test

I don't know this code well enough to review this patch, however, I have 2 suggestions:
1) Do a refactor patch first that adds the getter methods and makes the member variables private.  This will make the patch much smaller.
2) I don't really like m_scrollOriginChanged.  It's hard to tell when you're supposed to set it and when it's valid to read it.  It would be more clear to pass the bool around where needed.

[Xiaomei Ji]

(In reply to comment #13)
> (From update of attachment 112805 [details])
> I don't know this code well enough to review this patch, however, I have 2 suggestions:
> 1) Do a refactor patch first that adds the getter methods and makes the member variables private.  This will make the patch much smaller.

Good suggestion!

> 2) I don't really like m_scrollOriginChanged.  It's hard to tell when you're supposed to set it and when it's valid to read it.  It would be more clear to pass the bool around where needed.

it would be nice to pass the bool around. but the problem is updateScrollbar() might not be called Synchronously when m_scrollOrigin changes, besides m_scrollOrigin could be changed in RenderLayer as well.

I tried to modify updateScrollbar() with an extra bool parameter, and change the caller side to pass true/false depends on the functionality. But that introduce more un-necessary scrolling.

[Xiaomei Ji]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 112805 [details] [details])
> > I don't know this code well enough to review this patch, however, I have 2 suggestions:
> > 1) Do a refactor patch first that adds the getter methods and makes the member variables private.  This will make the patch much smaller.
> 
> Good suggestion!

BTW, changing m_scrollOrigin to be private is due to the introduction of m_scrollOriginChanged.
the setter will update m_scrollOriginChanged as well. If there is a better way to not introduce m _scrollOriginChanged, m_scrollOrigin does not need to be private.

[Tony Chang]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (From update of attachment 112805 [details] [details] [details])
> > > I don't know this code well enough to review this patch, however, I have 2 suggestions:
> > > 1) Do a refactor patch first that adds the getter methods and makes the member variables private.  This will make the patch much smaller.
> > 
> > Good suggestion!
> 
> BTW, changing m_scrollOrigin to be private is due to the introduction of m_scrollOriginChanged.
> the setter will update m_scrollOriginChanged as well. If there is a better way to not introduce m _scrollOriginChanged, m_scrollOrigin does not need to be private.

I think it's ok to make m_scrollOrigin private in a refactor patch and add m_scrollOriginChanged in the bug fix patch.

If having m_scrollOriginChanged is unavoidable, maybe name it to be more specific like m_scrollOriginChangedDuringResize or something.

[Xiaomei Ji]

Created attachment 113261 [details]
patch w/ layout test

I did not change the name of m_scrollOriginChanged.
resize cause it to change, but resize probably is not the only way cause it to change.

[Tony Chang]

Comment on attachment 113261 [details]
patch w/ layout test

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

Just a drive by review.  I don't really understand how the various Scroll* classes fit together.

> Source/WebCore/platform/ScrollAnimator.cpp:82
>          m_currentPosX = offset.x();
>          m_currentPosY = offset.y();
>          notifyPositionChanged();

Nit: unindent

> Source/WebCore/platform/ScrollableArea.cpp:73
> +    } else
> +        m_scrollOriginChanged = false;

Is this else necessary?  If so, why don't we set m_scrollOriginChanged = false in setScrollOrigin(X|Y)?

[Xiaomei Ji]

Created attachment 113335 [details]
patch w/ layout test

updated per comment #18. Thanks for the quick drive-by review.



(In reply to comment #18)
> (From update of attachment 113261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113261&action=review
> 
> Just a drive by review.  I don't really understand how the various Scroll* classes fit together.
> 
> > Source/WebCore/platform/ScrollAnimator.cpp:82
> >          m_currentPosX = offset.x();
> >          m_currentPosY = offset.y();
> >          notifyPositionChanged();
> 
> Nit: unindent

done.

> 
> > Source/WebCore/platform/ScrollableArea.cpp:73
> > +    } else
> > +        m_scrollOriginChanged = false;
> 
> Is this else necessary?  If so, why don't we set m_scrollOriginChanged = false in setScrollOrigin(X|Y)?

That is a good question.
Look at it again, I think the same as m_scrollOriginChanged is only accessed in ScrollView::updateScrollbars(), its value should be only reset to false in ScrollView::updateScrollbars(), so that ScrollView::updateScrollbars() is able to catch the once changed m_scrollOrigin and set ScrollAnimator::m_currentPosX|Y correctly.

As to the reason why not set m_scrollOriginChanged = false in setScrollOrigin(X|Y) is that: if you setScrollOriginX with a different scrollOriginX value, but then setScrollOriginY with the same scrollOriginY value, resetting m_scrollOriginChanged = false in setScrollOriginY will override the m_scrollOriginChanged = true value set in setScrollOriginX.

[WebKit Review Bot]

Comment on attachment 113335 [details]
patch w/ layout test

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

New failing tests:
fast/block/positioning/auto/vertical-rl/007.html
fast/block/basic/truncation-rtl.html
fast/multicol/vertical-rl/float-avoidance.html
fast/multicol/vertical-rl/column-rules.html
fast/multicol/vertical-rl/column-break-with-balancing.html

[Xiaomei Ji]

Created attachment 113368 [details]
patch w/ layout test

fix cr-linux failure.

[Tony Chang]

scottbyer or sail might be able to give an informal review.

[Scott Byer]

This patch makes sense to me; I like getting the condition check in one place instead of having it in two. It looks correct as far as I understand the scrolling code (which isn't really far enough yet).

[Xiaomei Ji]

(In reply to comment #23)
> This patch makes sense to me; I like getting the condition check in one place instead of having it in two.

which condition check you meant? m_scrollOriginChanged()?

I've emailed Sam (weinig) and Dan (mitz) too, hope they could give it a review.

> It looks correct as far as I understand the scrolling code (which isn't really far enough yet).

[Tony Chang]

Comment on attachment 113368 [details]
patch w/ layout test

This seems safe and well scoped.

Please add suppressions in test_expectations.txt so the Linux/Win bots don't go red.

[Xiaomei Ji]

Committed r99616: <http://trac.webkit.org/changeset/99616>

[Xiaomei Ji]

I do not know what went wrong (I tried test in Safari Mac and remembered it works fine there).
But the bug did reproduce in Safari Mac, and it also reproduce in chromium mac.
r99616 fix the problem in chromium win/linux, but it did not fix it in chromium mac and safari mac.

[Xiaomei Ji]

Comment on attachment 113368 [details]
patch w/ layout test

clear flag.

[Xiaomei Ji]

reopened for Mac port and cr-mac.
(r99616 fix the problem in chromium win/linux, but it did not fix it in chromium mac and safari mac.)

[Xiaomei Ji]

Created attachment 114594 [details]
WIP

it fixes the problem in Safari in Mac (tried manually).
One thing I am not sure of is that there are several callers of scrollToOffsetWithoutAnimation(), some of them are passing different scroll offsets, but probably not all of them. Without offset checking, it might trigger unnecessary "notifyPositionChanged()".

[Xiaomei Ji]

Created attachment 114803 [details]
patch w/ layout test

One thing I am not sure of is that there are several callers of scrollToOffsetWithoutAnimation(), some of them are passing different scroll offsets, but probably not all of them. Without offset checking, it might trigger unnecessary "notifyPositionChanged()".

[Xiaomei Ji]

Committed r100819: <http://trac.webkit.org/changeset/100819>

[Beth Dakin]

This change causes a new assertion failure in Debug builds. I am filing a bug for it now.

[Beth Dakin]

https://bugs.webkit.org/show_bug.cgi?id=73348 REGRESSION: Assertion when loading a page with a scrollable RenderLayer

[Eric Seidel (no email)]

This appears to be missing results for Lion: fast/dom/rtl-scroll-to-leftmost-and-resize.html