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
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.
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
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.
hm... with ScrollAnimator change, but with or without the ScrollView change mentioned in comment #4, RenderViewTest.OnNavStateChanged passed in my local chrome win build.
(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.
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.
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?
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
(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.
Created attachment 112630 [details] wip
Created attachment 112805 [details] patch w/ layout test
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.
(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.
(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.
(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.
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.
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)?
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.
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
Created attachment 113368 [details] patch w/ layout test fix cr-linux failure.
scottbyer or sail might be able to give an informal review.
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).
(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).
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.
Committed r99616: <http://trac.webkit.org/changeset/99616>
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.
Comment on attachment 113368 [details] patch w/ layout test clear flag.
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.)
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()".
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()".
Committed r100819: <http://trac.webkit.org/changeset/100819>
This change causes a new assertion failure in Debug builds. I am filing a bug for it now.
https://bugs.webkit.org/show_bug.cgi?id=73348 REGRESSION: Assertion when loading a page with a scrollable RenderLayer
This appears to be missing results for Lion: fast/dom/rtl-scroll-to-leftmost-and-resize.html