Tested on: Sfari 3.2.1 - XP, Midori 0.12 - Ubuntu (libwebkit 1.01.4+r40220) Google Chrome 1.0.154.43 - XP In every page I browsed where <body dir="rtl"> or body{direction:rtl} is defined, there is no horizontal scrollbar when the window is narrower then the page width. To reproduce - browse any Hebrew/Arabic page (you can try mine - http://tombigel.com or http://tendu.tombigel.com)
Just a fix for last comment, tombigel.com is in english so it doesn't help here... sorry, force of habbit. http://tendu.tombigel.com/he is in hebrew
Confirmed with r40246.
Duplicate of bug 3352?
I don't think so. This bug is a specific bug for RTL pages. the same page without RTL definition renders just fine (you can press the "english" link in tendu.tombigel.com, and see, same HTML, english content, no direction:rtl on <body>)
Created attachment 28754 [details] Simpler example, toggling rtl toggles also the bottom scrollbar
Created attachment 29675 [details] A workaround fix As far as I investigated on my MacBook, it is an edge-case problem caused by discrepancy (sorry, I cannot figure out any better words) between FrameView and RenderBlock. FrameView trims the left side of its children when their offsetLeft is negative. (It is an expected behavior.) On the other hand, RenderBlock::determineHorizontalPosition may set a negative value to the offsetLeft value of an RTL element wider than the window width. (It is also an expected behavior.) So, when FrameView has a child RTL element wider than the window width, RenderBlock::determineHorizontalPosition set a negative value to the offsetLeft value of the RTL element and FrameView trims its left side without displaying a horizontal scrollbar. Even Though I'm not confident this is a right fix, I wrote a workaround fix and a layout test for this case.
Created attachment 29676 [details] a workaround fix 2 Sorry, I attached a wrong fix.
Are there any persons who can review my fix?
still stuck in review? :/
I just emailed hyatt and mitz directly asking for review.
The patch sounds conceptually right to me, but it would be nice to have input from an expert.
(In reply to comment #11) > The patch sounds conceptually right to me, but it would be nice to have input > from an expert. I agree on both counts.
Comment on attachment 29676 [details] a workaround fix 2 I don't really understand why only the <body> would be special cased here. This may need more discussion, as I really don't understand the intent of this patch.
What do other browsers do in this situation (quirks and strict mode)? It's not clear to me that shrinking the width of an element is the right thing to do here.
(In reply to comment #14) > What do other browsers do in this situation (quirks and strict mode)? It's not > clear to me that shrinking the width of an element is the right thing to do > here. Sorry for my very slow response. To show the test file in my fix 2 with IE8 and Firefox 3, their "document.getElementById('test').offsetLeft" values become negative. So, my workaround, which moves the <body> element so that the above value always become non-negative, is wrong in terms of browser compatibility. I'm finding other solutions for this bug. Regards, Hironori Bono
Created attachment 43559 [details] A workaround fix 3 (In reply to comment #15) > I'm finding other solutions for this bug. I have updated my fix to increase the left margin of an RTL element to prevent its child elements (whose directions are also RTL) from being clipped. Even tough I'm not confident this is the right fix, is it possible to review this change and give me your comments? Regards, Hironori Bono
Comment on attachment 43559 [details] A workaround fix 3 I'm not a reviewer, but here are my comments. Hyatt or Dan should probably give their opinion on whether there's a better fix. > Index: WebCore/ChangeLog > =================================================================== > + // Update the minimal left position. Can you add some more details and point to the bug URL from the comment. > + m_rtlMinChildPosition = min(chPos, m_rtlMinChildPosition); ASSERT(m_rtlMinChildPosition <= 0); > + // Reset the minimal left position for RTL child elements before start layouting children. // Reset before preforming child layout. > + m_rtlMinChildPosition = 0; > + // Override the RenderBox::marginLeft(). // Override RenderBox::marginLeft(). > Property changes on: LayoutTests/fast/text/international/rtl-unexpected-underflow.html > ___________________________________________________________________ > Name: svn:executable > + * Is this OK?
It's also http://crbug.com/10533
style-queue successfully ran check-webkit-style on attachment 43559 [details] without any errors
Comment on attachment 43559 [details] A workaround fix 3 Please try to pare down the comments. It's not necessary to have a comment (often lengthy) every time int m_rtlMinChildPosition appears. This hurts readability. Try to limit your comments to saying *why* the code is doing something, rather than *what* it is doing. "what" should be apparent from the code. I also agree with Jeremy's comment that the "less than or equal to 0" condition is best documented with an ASSERT, in which case I expect the multiple comments explaining that it is less than or equal to 0 are not necessary. r- to clean this up. The change otherwise looks ok to me, assuming it matches what other browsers do, but it may help to have a rendering expert chime in.
Created attachment 45958 [details] Workaround fix 4 Jeremy and Maciej, Thank you for your review and sorry for my slow response. (In reply to comment #17) > (From update of attachment 43559 [details]) > I'm not a reviewer, but here are my comments. > Hyatt or Dan should probably give their opinion on whether there's a better > fix. > > > Index: WebCore/ChangeLog > > =================================================================== > > + // Update the minimal left position. > Can you add some more details and point to the bug URL from the comment. Thank you for noticing this. I have added some more comments why we need the leftmost position to fix this issue. > > + m_rtlMinChildPosition = min(chPos, m_rtlMinChildPosition); > ASSERT(m_rtlMinChildPosition <= 0); Done. > > + // Reset the minimal left position for RTL child elements before start layouting children. > // Reset before preforming child layout. > > + m_rtlMinChildPosition = 0; > > > + // Override the RenderBox::marginLeft(). > // Override RenderBox::marginLeft(). Done. > > Property changes on: LayoutTests/fast/text/international/rtl-unexpected-underflow.html > > ___________________________________________________________________ > > Name: svn:executable > > + * > Is this OK? Oops, I forgot changing the file permission and line-endings when I copied this file from Windows. I have changed them to remove this property. (In reply to comment #20) > (From update of attachment 43559 [details]) > Please try to pare down the comments. It's not necessary to have a comment > (often lengthy) every time int m_rtlMinChildPosition appears. This hurts > readability. Try to limit your comments to saying *why* the code is doing > something, rather than *what* it is doing. "what" should be apparent from the > code. I also agree with Jeremy's comment that the "less than or equal to 0" > condition is best documented with an ASSERT, in which case I expect the > multiple comments explaining that it is less than or equal to 0 are not > necessary. > > r- to clean this up. The change otherwise looks ok to me, assuming it matches > what other browsers do, but it may help to have a rendering expert chime in. Thank you for your comments. I tend to write more garbage comments when I'm less confident of it. I deleted some garbage comments in my previous change. Nevertheless, I'm still not so confident of this change because 'offsetLeft' values are not so consistent among browsers and I'm not sure what values I should rely on. (This change makes Safari look similar with other browsers, though.) Regards, Hironori Bono
style-queue ran check-webkit-style on attachment 45958 [details] without any errors.
This will affect computed style. Is it really how this works in other browsers?
Comment on attachment 45958 [details] Workaround fix 4 This doesn’t look like the right approach to fixing this bug. There is no apparent reason why it should involve increasing the size of RenderBlock, and this seems to account only for block children of blocks in normal flow. Changing the margin like has all sorts of consequences, e.g. affecting computed style. I don’t understand why any of this is necessary unless the body element is RTL. Looking at what Firefox does, it seems to be the only consideration. This patch also doesn’t address the initial position of the horizontal scrollbar on a RTL page. It maintains a lot of asymmetry between the RTL and LTR cases. I think the correct approach should be similar to a case that already behaves correctly: RTL blocks with overflow: scroll. The logic for those is in RenderLayer. They mirror the behavior of LTR blocks with overflow: scroll. RenderView should behave the same with respect to the direction of the body element. Currently it has a LTR bias: docWidth() considers the rightmost position and the right edges of child boxes. layout() adds overflow to the right but never to the left, etc. The correct place to fix this is mainly in RenderView, but some changes to FrameView and Frame will probably be needed too. I suggest studying the RenderLayer code.
Thank you for your comments and sorry for my slow response. I have been reading the rendering code of WebKit to find better solutions for this bug. (I noticed my change was not a good one and this was the reason why I wasn't confident of it. ) This suggestion definitely helps it. Regards, Hironori Bono (In reply to comment #24) > (From update of attachment 45958 [details]) > This doesn’t look like the right approach to fixing this bug. There is no > apparent reason why it should involve increasing the size of RenderBlock, and > this seems to account only for block children of blocks in normal flow. > Changing the margin like has all sorts of consequences, e.g. affecting computed > style. I don’t understand why any of this is necessary unless the body element > is RTL. Looking at what Firefox does, it seems to be the only consideration. > This patch also doesn’t address the initial position of the horizontal > scrollbar on a RTL page. It maintains a lot of asymmetry between the RTL and > LTR cases. > > I think the correct approach should be similar to a case that already behaves > correctly: RTL blocks with overflow: scroll. The logic for those is in > RenderLayer. They mirror the behavior of LTR blocks with overflow: scroll. > RenderView should behave the same with respect to the direction of the body > element. Currently it has a LTR bias: docWidth() considers the rightmost > position and the right edges of child boxes. layout() adds overflow to the > right but never to the left, etc. > > The correct place to fix this is mainly in RenderView, but some changes to > FrameView and Frame will probably be needed too. I suggest studying the > RenderLayer code.
<rdar://problem/5710505>
Created attachment 53150 [details] A proposed fix 5 Sorry for my slow update. I have updated my change to change the RenderView class instead of changing the RenderBlock class as suggested by mitz@webkit.org. (This change also changes the RenderBox class not only to synchronize the text direction of a RenderView object with the one of a <body> element, but also to paint the background of a RenderView object with the color of the <body> element.) Unfortunately, this change uses a pixel test because I haven't find any good ideas to write a text test for this bug. Would it be possible to review the updated change and give me your comments? Regards, Hironori Bono
Comment on attachment 53150 [details] A proposed fix 5 Obligatory disclaimer: I'm not a reviewer, but here are my comments. > Index: WebCore/rendering/RenderBox.cpp > =================================================================== > --- WebCore/rendering/RenderBox.cpp (revision 57464) > +++ WebCore/rendering/RenderBox.cpp (working copy) > @@ -182,9 +182,12 @@ void RenderBox::styleDidChange(StyleDiff > + if (isBody()) { > document()->setTextColor(style()->color()); > + if (view() && view()->style()) Can view() or view()->style() ever be NULL? Same goes for other places in the patch. > void RenderBox::updateBoxModelInfoFromStyle() > @@ -608,7 +611,11 @@ void RenderBox::paintRootBoxDecorations( > + // The bounding rectangle of an RTL view is (min(leftmostPosition(), 0), 0, docWidth(), docHeight()) > + // So, we also include the leftmost position for RTL views so we can fill its entire canvas. Great comment. > + if (view() && view()->style()->direction() == RTL && view()->leftmostPosition() < 0) Again, are you sure you need to null-check view() ? > @@ -117,6 +117,14 @@ void RenderView::layout() > + if (style() && style()->direction() == RTL && leftmostPosition() < 0) { Using a temp. variable may ease readability here a bit since you use the same expression in the return statement.
Created attachment 53324 [details] A proposed fix 6 Jeremy, Thank you for your comments. I have updated my patch to replaced null-checks with ASSERT() expressions and to use temporary variables. Regards, Hironori Bono (In reply to comment #28) > (From update of attachment 53150 [details]) > Obligatory disclaimer: I'm not a reviewer, but here are my comments. > > > Index: WebCore/rendering/RenderBox.cpp > > =================================================================== > > --- WebCore/rendering/RenderBox.cpp (revision 57464) > > +++ WebCore/rendering/RenderBox.cpp (working copy) > > @@ -182,9 +182,12 @@ void RenderBox::styleDidChange(StyleDiff > > + if (isBody()) { > > document()->setTextColor(style()->color()); > > + if (view() && view()->style()) > > Can view() or view()->style() ever be NULL? Same goes for other places in the > patch. > > > void RenderBox::updateBoxModelInfoFromStyle() > > @@ -608,7 +611,11 @@ void RenderBox::paintRootBoxDecorations( > > + // The bounding rectangle of an RTL view is (min(leftmostPosition(), 0), 0, docWidth(), docHeight()) > > + // So, we also include the leftmost position for RTL views so we can fill its entire canvas. > Great comment. > > > + if (view() && view()->style()->direction() == RTL && view()->leftmostPosition() < 0) > Again, are you sure you need to null-check view() ? > > > @@ -117,6 +117,14 @@ void RenderView::layout() > > + if (style() && style()->direction() == RTL && leftmostPosition() < 0) { > Using a temp. variable may ease readability here a bit since you use the same > expression in the return statement.
It seems this change has not been reviewed by WebKit reviewers for more than a month. Someone who has better connection with WebKit reviewers should take over this issue rather than I continue working for it? Regards, Hironori Bono
it's pretty sad (and surprising!) nobody is willing to review such as important fix (possibly the most needed RTL related one). i do hope you won't give up though :/
I'm not a reviewer, but I took a brief look. I'm not sure why we would scroll the renderview all the way over to the left in this case - in firefox it appears that this does not occur (i.e. when loading the test page in firefox the scrollbar is all the way to the right).
(In reply to comment #32) > I'm not a reviewer, but I took a brief look. I'm not sure why we would scroll the renderview all the way over to the left in this case - in firefox it appears that this does not occur (i.e. when loading the test page in firefox the scrollbar is all the way to the right). Thank you for your comment. Even though I notice this issue, I personally would like to file another bug for this scrollbar issue rather than integrating its fix into this change. (I'm afraid it makes this change more complicated and may make it less popular among WebKit reviewers.) Regards, Hironori Bono
Comment on attachment 53324 [details] A proposed fix 6 > + if (isBody()) { > document()->setTextColor(style()->color()); > + ASSERT(view() && view()->style()); 1. In principle, rather than asserting a conjunction, you should assert each condition separately, so that if the assertion fails, you know which condition was false. 2. In this case, and in similar cases in this patch, I find that it is pointless to assert that a pointer is non-null if you are immediately going to dereference it. > + view()->style()->setDirection(style()->direction()); It’s wrong to mutate the RenderView’s style like this. In general, it is almost never a good idea to change a RenderStyle anywhere other than in the style selector. This holds true in this case as well. After this code runs, if the <html> element’s style is recomputed, it may inherit the wrong direction from the root, e.g. <html> <body style="direction: rtl;"> <script> document.body.offsetTop; // force style recalc and layout document.documentElement.style.color = "red"; document.body.offsetTop; alert(getComputedStyle(document.documentElement).direction); </script> </body> </html> would alert "rtl" instead of "ltr". It is also not clear to me that this behavior should be limited to when the <body> element has direction: rtl. What about the case where the <html> element has it but <body> is ltr? > + if (style()->direction() == RTL) { > + int left = leftmostPosition(); > + if (left < 0) { > + setHasOverflowClip(true); I don’t understand this. What side effect of hasOverflowClip() are you trying to achieve? It seems really dangerous to set hasOverflowClip() for the root, and very strange to touch this flag during layout.
Thank you for your comments. Unfortunately, I have totally lost confidence that I can continue working for this issue. (These comments clearly show I don't have good knowledge about WebKit.) I wish someone who has better knowledge about WebKit takes over this issue. Regards, Hironori Bono (In reply to comment #34) > (From update of attachment 53324 [details]) > > + if (isBody()) { > > document()->setTextColor(style()->color()); > > + ASSERT(view() && view()->style()); > > 1. In principle, rather than asserting a conjunction, you should assert each condition separately, so that if the assertion fails, you know which condition was false. > 2. In this case, and in similar cases in this patch, I find that it is pointless to assert that a pointer is non-null if you are immediately going to dereference it. > > > + view()->style()->setDirection(style()->direction()); > > It’s wrong to mutate the RenderView’s style like this. In general, it is almost never a good idea to change a RenderStyle anywhere other than in the style selector. This holds true in this case as well. After this code runs, if the <html> element’s style is recomputed, it may inherit the wrong direction from the root, e.g. > > <html> > <body style="direction: rtl;"> > <script> > document.body.offsetTop; // force style recalc and layout > document.documentElement.style.color = "red"; > document.body.offsetTop; > alert(getComputedStyle(document.documentElement).direction); > </script> > </body> > </html> > > would alert "rtl" instead of "ltr". > > It is also not clear to me that this behavior should be limited to when the <body> element has direction: rtl. What about the case where the <html> element has it but <body> is ltr? > > > + if (style()->direction() == RTL) { > > + int left = leftmostPosition(); > > + if (left < 0) { > > + setHasOverflowClip(true); > > I don’t understand this. What side effect of hasOverflowClip() are you trying to achieve? It seems really dangerous to set hasOverflowClip() for the root, and very strange to touch this flag during layout.
(In reply to comment #34) > It is also not clear to me that this behavior should be limited to when the <body> element has direction: rtl. What about the case where the <html> element has it but <body> is ltr? I tested this a while back and found that indeed, in Firefox, the body element’s direction is the only one that matters.
Created attachment 62608 [details] patch w/ layout test The patch works for Chromium in 3 platforms, but it does not work well in MAC port where ScrollView is a platformWidget. The problem in MAC port is: For RTL page, the horizontal scrollbar should be at the very right when page is loaded at first time. I am re-setting the scroll position in (void)setScrollOriginX:(int)scrollOriginX inside WebDynamicScrollBarsView.mm. Please refer to FIXME in the file. But apparently, it is not the right place to reset it. Resetting the scroll position there makes the scroll position reset to its original position for RTL page when page reload, page zoom, page resize, and might also window.scrollX (although I do not understand why there is write operation in this read-only access), which are all wrong. Please refer to the FIXMEs in the layout test file.
Comment on attachment 62608 [details] patch w/ layout test Thanks for tackling this bug! I think the ScrollView-based approach is a good one. > Index: WebCore/ChangeLog > + Add frame horizontal scroll bar for RTL page. This is not a very good description of the change. This bug also doesn’t have a very good title, so maybe we can fix both. > + https://bugs.webkit.org/show_bug.cgi?id=23556 > + > + For RTL page, save left layout overflow and include it into the document size during layout. Use the left layout overflow when scroll and paint Extra space (or missing newline) up there. > + the page. Behavior on LTR page should be untouched since left layout > + overflow is set as 0 for LTR page. > + > + Test: fast/dom/horizontal_scrollbar_in_rtl.html We typically use dashes, not underscores, in test names. I think this could use more than one test. For example, test a RTL page with right overflow. > + // Do not move setScrollOriginX() to other places, otherwise, > + // content painted and scroll might not work correctly when page resize. I don’t understand this comment. Is it warning me not to move the code somewhere else? I don’t think that’s necessary. > + ScrollView::setScrollOriginX(abs(root->leftLayoutOverflow())); No need to use abs() here. leftLayoutOverflow() is always non-positive, so you can just use -root->leftLayoutOverflow(). > + // Adjust scroll position to above the minimum value. y-axis should not be > + // negative. The minimum value for x-axis is the left layout overflow, > + // which should be zero for non RTL page and the negative of m_scrollOriginX > + // for RTL page. “non RTL” is “LTR” :-) At the platform level, I think you don’t need to refer to the page, but just say that m_scrollOriginX sets the minimum. Perhaps for symmetry you can define a minimumScrollPosition() method. > - IntSize maxScrollPosition(contentsWidth() - visibleWidth(), contentsHeight() - visibleHeight()); > + IntSize maxScrollPosition(contentsWidth() - visibleWidth() - m_scrollOriginX, contentsHeight() - visibleHeight()); > + maxScrollPosition.clampNegativeToZero(); Can this just use the maximumScrollPosition() method? > IntSize scroll = desiredOffset.shrunkTo(maxScrollPosition); > - scroll.clampNegativeToZero(); > - > + // Adjust scroll position to above the minimum value. y-axis should not be > + // negative. The minimum value for x-axis is the left layout overflow, > + // which should be zero for non RTL page and the negative of m_scrollOriginX > + // for RTL page. > + scroll = scroll.expandedTo(IntSize(-m_scrollOriginX, 0)); > + Same comments about the comment and same suggestion to define a minimumScrollPosition() method. > +void ScrollView::setScrollOriginX(int x) > +{ > + m_scrollOriginX = x; > +#if PLATFORM(MAC) > + platformSetScrollOriginX(); > +#endif > +} This should check for platformWidget(), not do a compile-time PLATFORM check. Do you even need to set m_scrollOriginX if there is a platform widget, or can you just change platformSetScrollOriginX() to take a parameter and pass x to it? > + void setScrollOriginX(int x); Please omit the “x” here. > + int m_scrollOriginX; // Only non-zero for RTL page. This comment is not very helpful. It might help if it explained what the value means and what it does, but I think it’s not really necessary to comment on member variables. > + void platformSetScrollOriginX(); I suggest that this method take a parameter (see above). > + // leftmostPosition() is not always 0 for LTR page, for example http://www.apple.com/startpage/. This is not a helpful comment, and the example page will change in the future. This clips out left overflow in LTR pages, but where is the part that clips out right overflow in RTL pages? > + int leftOverflow = m_bodyIsRTL ? std::min(0, leftmostPosition()) : 0; > + addLayoutOverflow(IntRect(leftOverflow, 0, docWidth(leftOverflow), docHeight())); … > -int RenderView::docWidth() const > +int RenderView::docWidth(int leftOverflow) const > { > - int w = rightmostPosition(); > + int w = rightmostPosition() - leftOverflow; I would like to suggest something a bit different: add a docLeft() method that returns 0 for LTR pages and leftmostPosition() for RTL pages; then change the docWidth() computation of w to return rightmostPosition() for LTR pages and width() - leftmostPosition() for RTL pages. I think this will also clip out right overflow for those pages. > + void setBodyIsRTL(bool bodyIsRTL) { m_bodyIsRTL = bodyIsRTL; } In recent years, I have tried to make all booleans be about LTR, not RTL, so this would be …bodyIsLTR (with the reverse meaning). Another option is to have a setPageDirection() method that takes a TextDirection type argument. > + In MAC port, set and get the original x-axis scroll position. It’s Mac, not MAC (also in other places in this patch). Really good! I can’t quite understand what’s going on on Mac without applying the patch and trying it out, which I am going to do later.
Thanks for the quick review and comments! I've updated the patch according to all comments except adding test case of RTL page with right overflow which I will be working on. Please see my detail reply below. Thanks (In reply to comment #38) > (From update of attachment 62608 [details]) > Thanks for tackling this bug! I think the ScrollView-based approach is a good one. > > > Index: WebCore/ChangeLog > > > + Add frame horizontal scroll bar for RTL page. > > This is not a very good description of the change. This bug also doesn’t have a very good title, so maybe we can fix both. Changed to use the updated bug title. > > > + https://bugs.webkit.org/show_bug.cgi?id=23556 > > + > > + For RTL page, save left layout overflow and include it into the document size during layout. Use the left layout overflow when scroll and paint > > Extra space (or missing newline) up there. Done. > > > + the page. Behavior on LTR page should be untouched since left layout > > + overflow is set as 0 for LTR page. > > + > > + Test: fast/dom/horizontal_scrollbar_in_rtl.html > > We typically use dashes, not underscores, in test names. I think this could use more than one test. For example, test a RTL page with right overflow. Done update test name. Will work on adding test RTL page with right overflow. > > > + // Do not move setScrollOriginX() to other places, otherwise, > > + // content painted and scroll might not work correctly when page resize. > > I don’t understand this comment. Is it warning me not to move the code somewhere else? I don’t think that’s necessary. removed. > > > + ScrollView::setScrollOriginX(abs(root->leftLayoutOverflow())); > > No need to use abs() here. leftLayoutOverflow() is always non-positive, so you can just use -root->leftLayoutOverflow(). Done. > > > + // Adjust scroll position to above the minimum value. y-axis should not be > > + // negative. The minimum value for x-axis is the left layout overflow, > > + // which should be zero for non RTL page and the negative of m_scrollOriginX > > + // for RTL page. > > “non RTL” is “LTR” :-) > > At the platform level, I think you don’t need to refer to the page, but just say that m_scrollOriginX sets the minimum. > > Perhaps for symmetry you can define a minimumScrollPosition() method. Added minimumScrollPosition() and adjustScrollPositionWithinRange(). > > > - IntSize maxScrollPosition(contentsWidth() - visibleWidth(), contentsHeight() - visibleHeight()); > > + IntSize maxScrollPosition(contentsWidth() - visibleWidth() - m_scrollOriginX, contentsHeight() - visibleHeight()); > > + maxScrollPosition.clampNegativeToZero(); > > Can this just use the maximumScrollPosition() method? I did not combine them before since one is IntSize and the other is IntPoint. I changed it to use adjustScrollPositionWithinRange() and did conversion between IntSize and IntPoint on input parameter and return value. > > > IntSize scroll = desiredOffset.shrunkTo(maxScrollPosition); > > - scroll.clampNegativeToZero(); > > - > > + // Adjust scroll position to above the minimum value. y-axis should not be > > + // negative. The minimum value for x-axis is the left layout overflow, > > + // which should be zero for non RTL page and the negative of m_scrollOriginX > > + // for RTL page. > > + scroll = scroll.expandedTo(IntSize(-m_scrollOriginX, 0)); > > + > > Same comments about the comment and same suggestion to define a minimumScrollPosition() method. Done. > > > +void ScrollView::setScrollOriginX(int x) > > +{ > > + m_scrollOriginX = x; > > +#if PLATFORM(MAC) > > + platformSetScrollOriginX(); > > +#endif > > +} > > This should check for platformWidget(), not do a compile-time PLATFORM check. Do you even need to set m_scrollOriginX if there is a platform widget, or can you just change platformSetScrollOriginX() to take a parameter and pass x to it? Done. > > > + void setScrollOriginX(int x); > > Please omit the “x” here. Done. > > > + int m_scrollOriginX; // Only non-zero for RTL page. > > This comment is not very helpful. It might help if it explained what the value means and what it does, but I think it’s not really necessary to comment on member variables. Change the comments to explain what the value is and how it is used. > > > + void platformSetScrollOriginX(); > > I suggest that this method take a parameter (see above). Done. > > > + // leftmostPosition() is not always 0 for LTR page, for example http://www.apple.com/startpage/. > > This is not a helpful comment, and the example page will change in the future. > > This clips out left overflow in LTR pages, but where is the part that clips out right overflow in RTL pages? Changed the comment to be "clip out left overflow in LTR page". right overflow is clipped by using "width() - leftmostposition()" as docWidth as you suggested below. > > > + int leftOverflow = m_bodyIsRTL ? std::min(0, leftmostPosition()) : 0; > > + addLayoutOverflow(IntRect(leftOverflow, 0, docWidth(leftOverflow), docHeight())); > > … > > > -int RenderView::docWidth() const > > +int RenderView::docWidth(int leftOverflow) const > > { > > - int w = rightmostPosition(); > > + int w = rightmostPosition() - leftOverflow; > > I would like to suggest something a bit different: add a docLeft() method that returns 0 for LTR pages and leftmostPosition() for RTL pages; then change the docWidth() computation of w to return rightmostPosition() for LTR pages and width() - leftmostPosition() for RTL pages. I think this will also clip out right overflow for those pages. Added docLeft(). And Changed the docWidth() computation as you suggested. > > > + void setBodyIsRTL(bool bodyIsRTL) { m_bodyIsRTL = bodyIsRTL; } > > In recent years, I have tried to make all booleans be about LTR, not RTL, so this would be …bodyIsLTR (with the reverse meaning). Another option is to have a setPageDirection() method that takes a TextDirection type argument. Changed to bodyIsLTR. > > + In MAC port, set and get the original x-axis scroll position. > > It’s Mac, not MAC (also in other places in this patch). Done. > > Really good! I can’t quite understand what’s going on on Mac without applying the patch and trying it out, which I am going to do later. Thanks a lot for helping me to figure it out.
Created attachment 62753 [details] patch w/ layout test updated patch per Mitz's suggestions (except adding test case for RTL page with right overflow which I will work on). You do not need to review the patch. I will upload another one for review after it is working for Mac port.
Created attachment 65719 [details] Another patch just for reference When I was going to commit my patch, I found Xiaomei has already done this. Sadly.... What a waste of my time. I should have added myself to the cc list. Anyway, there is a little difference between xiaomei's. Maybe it will be a little useful to her/him as a reference.
(In reply to comment #41) > Created an attachment (id=65719) [details] > Another patch just for reference > > When I was going to commit my patch, I found Xiaomei has already done this. Sadly.... What a waste of my time. I should have added myself to the cc list. > Anyway, there is a little difference between xiaomei's. Maybe it will be a little useful to her/him as a reference. Thanks for the upload. Looks like the patch wont work for Safari in Mac either. We need to get this fixed (to pass layout test) to check the patch in, otherwise, we need to make this as non-safari-in-mac patch.
Created attachment 66098 [details] patch w/ layout test The patch is not completed for Mac port: 1. For RTL page, the horizontal scroll bar is at the very left (instead of very right as in Firefox and Chromium with the patch) when page first loaded. 2. For RTL page, horizontal scrollbar does not behave completely correct for zoom in and out. For example, zooming in and then out should keep the horizontal scrollbar at the same position as it was before zooming. But it is not the case for Safari in Mac. See test file horizontal-scrollbar-in-rtl.html for detail.
Attachment 66098 [details] did not build on qt: Build output: http://queues.webkit.org/results/3845187
Created attachment 66128 [details] patch w/ layout test fix compilation error in QT.
Looks like Dan is busy. Wondering could David and Simon review the patch? I believe this fix is pretty important for RTL users. Although it does not work completely for Safari in Mac, it is a big improvement comparing with cutting off the left part of the content. I am hoping the patch could be submitted and someone from Apple knowing the codebase better to work on the issues in Mac port. (Or I could make this not-for-platformWidget-scrollview patch if that is desired). Thanks in advance and appreciate your attention.
*** Bug 15489 has been marked as a duplicate of this bug. ***
in some cultures, silence is approval (maybe that's the case here ;) )
Comment on attachment 66128 [details] patch w/ layout test The scroll stuff looks fine. I don't like the m_bodyIsLTR variable though. If a different direction is explicitly specified on the root element, this should supercede the body's direction. In addition, XML documents will typically set the direction on the root. Since the time this patch has been worked on, I committed a fix that propagates both direction and writing-mode on the root element (<html>) up to the RenderView's style. This means the RenderView itself now matches the direction and writing-mode of the <html>. Rather than doing this m_bodyIsLTR hack, I think we should probably expand the propagation code I added to also propagate from the body. It should only do this though if the <html> element didn't explicitly have its direction set. You can see this code in CSSStyleSelector::styleForDocument and then it also dynamically updates the style in styleDidChange. You may want to consider an initial checkin that just works with the RenderView's direction (using test cases that set RTL on <html>) and work on the propagation code from the <body> in a followup patch.
Actually I have to fix the body propagation issue for writing-mode anyway, so I can just handle that part for you in a separate bug.
I filed https://bugs.webkit.org/show_bug.cgi?id=48157 to cover the <body> propagation issue. Once I fix that, the RenderView will have the correct directionality set right on its style.
Created attachment 71805 [details] patch w/ layout test The patch is not completed for Mac port (if PLATFORM(MAC) && defined __OBJC__) where ScrollView is defined as PlatformWidget. In which, 1. For RTL page, the horizontal scroll bar is at the very left (instead of very right as in Firefox and Chromium with the patch) when page first loaded. Ideally, scrollbar should be at the very right when page first loaded. But it should *not* reset to this original position when page reload or zooming. 2. For RTL page, horizontal scrollbar does not behave completely correct for zoom in and out. For example, zooming in and then out should keep the horizontal scrollbar at the same position as it was before zooming. But it is not the case for Safari in Mac. See test file horizontal-scrollbar-in-rtl.html for detail. I need some help to figure out where are the correct places to make above 2 cases work. Or maybe case 1. is not possible for Safari in Mac?
xji: I may be mistaken but looks like your patch regresses bug 24118
(In reply to comment #53) > xji: I may be mistaken but looks like your patch regresses bug 24118 Yes, you are right. After picking up r70456 where dhyatt did propagation direction from <body>, the testing URL http://www.dentalspace.co.il works correctly with horizontal scrollbar. And the test case you posted (https://bug-24118-attachments.webkit.org/attachment.cgi?id=72065) works correctly with horizontal scrollbar. But the problem happens when you switch body direction. Seems the style change did not propagate correctly to RenderView. It is the same case if the original body direction is set as "ltr" in your test case.
Created attachment 74099 [details] Snapshot Snapshot of current work, this now appears to work on Safari with caveats listed below. Known issues with this patch: * History save/restore of scrollbar position doesn't work correctly. * fast/dom/horizontal-scrollbar-in-rtl.html layout test reloads itself in a loop and thus can't be run manually in Safari. The scrollbar thumb isn't updated when switching document direction programmatically though this appears to be an existing bug with style propagation. Probably best to address this in a separate bug.
Created attachment 74100 [details] Snapshot Rebased on ToT + style fix
The reason that save/restore of the scrollbar position isn't working appears to be that the call to [documentView scrollPoint:] which I added in [WebDynamicScrollBarsView refreshInitialScrollbarPosition] is firing off a notification which runs through the same code path as if the user had moved the scrollbar. This has 2 undesirable effects: * Looks like this may dispatch a JS scroll event. * FrameView::setWasScrolledByUser() is called. The call to FrameView::setWasScrolledByUser() causes the code in HistoryController::restoreScrollPositionAndViewState() to think that the user has moved the scrollbar manually (view->wasScrolledByUser() returns true) and so doesn't override it with the saved position. Relevant part of stack trace: #0 0x101881f0c in WebCore::FrameView::setWasScrolledByUser at FrameView.cpp:2054 #1 0x1018039b5 in WebCore::EventHandler::setFrameWasScrolledByUser at EventHandler.cpp:2767 #2 0x101803a8c in WebCore::EventHandler::sendScrollEvent at EventHandler.cpp:2758 #3 0x101884f39 in WebCore::FrameView::scrollPositionChanged at FrameView.cpp:1262 #4 0x10188503f in WebCore::FrameView::scrollPositionChangedViaPlatformWidget at FrameView.cpp:1257 #5 0x100f7c516 in -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] at WebHTMLView.mm:1208 #6 0x7fff8706f84e in _nsnote_callback #7 0x7fff832d2a90 in __CFXNotificationPost #8 0x7fff832bf008 in _CFXNotificationPostNotification #9 0x7fff870667b8 in -[NSNotificationCenter postNotificationName:object:userInfo:] #10 0x7fff862e3479 in -[NSView _postBoundsChangeNotification] #11 0x7fff863f8073 in -[NSView translateOriginToPoint:] #12 0x7fff863bb07a in -[NSClipView _immediateScrollToPoint:] #13 0x100f1e9f7 in -[WebClipView _immediateScrollToPoint:] at WebClipView.mm:99 #14 0x7fff863bac05 in -[NSClipView scrollToPoint:] #15 0x7fff863f7bc6 in -[NSScrollView scrollClipView:toPoint:] #16 0x7fff86335bbc in -[NSClipView _scrollTo:animate:] #17 0x7fff863b2af9 in -[NSClipView _scrollPoint:fromView:] #18 0x7fff863b2a7a in -[NSView scrollPoint:] #19 0x100f329f6 in -[WebDynamicScrollBarsView refreshInitialScrollbarPosition] at WebDynamicScrollBarsView.mm:171 Does anyone have any suggestions on the proper way to set the initial scrollbar position? If not, I'll try to add state to FrameView to identify this situation.
Created attachment 74197 [details] snapshot add patch fixing scrollbar repositioning when document's direction changes (only works for non platformWidget). The patch does not work for platformWidget ScrollView in 3 areas: 1. scrollbar reset to right-most position when page reload. (should have a layout test to cover it). 2. scrolling and scrollbar position does not work well in zoom-in/out. The scroll position should be updated so the content stays in relatively the same position. (has layout test). 3. scrollbar does not reposition when document's direction changes, for example, scrollbar should be changed from left-most position to right-most position when document changes from left-to-right to right-to-left directionality. This is related to case 1. (has layout test).
Created attachment 74233 [details] patch dhyatt,mitz: Could you take a look please? Updated patch with following changes: 1)History restore now works correctly, see note below. 2) Minor cleanup of horizontal-scrollbar-in-rtl.html. 3) Added layout test to make sure no scroll events are fired on initial display. I chatted with dhyatt a bit on irc on how the platformWidget code should update the initial scrollbar position. The logical option would be to call FrameView:: setScrollPosition(), however the only place where we know that a horizontal scrollbar has been created is in [WebDynamicScrollView updateScrollers] and calling out to FrameView from there would be a layering violation. The other problem is that FrameView:: setScrollPosition() triggers a JS scroll event, which is not something we want to do when setting the initial scrollbar position. The patch moves the scrollers by calling [documentView scrollPoint:] and works around the problem in comment #57 by short-circuiting the notification callback if it detects that the scroller is having it's initial position set programmatically. If you think there's a better way of doing this, I'd appreciate a suggestion for an alternate approach. Known issues with this patch: 1) scrollbar location not updated when programmatically setting direction for platformWidget code. 2) resize test in horizontal-scrollbar-in-rtl.html fails on mac platform. I think that neither of these are regressions from the current behavior, and if it's OK I'd like to address them in a followup patch.
Attachment 74233 [details] did not build on mac: Build output: http://queues.webkit.org/results/6247022
I think the build error is an issue with the Mac EWS, I've pinged Eric.
I think the ews hit some variant of bug 49212 which affects Snow Leopard machines which run the layout tests repeatedly.
Created attachment 74338 [details] patch w/ layout test updated expected result for fast/block/basic/truncation-rtl.html and ChangeLogs.
> Known issues with this patch: > 2) resize test in horizontal-scrollbar-in-rtl.html fails on mac platform. > resize works. zooming test fails: scrolling and scrollbar position is not updated correctly so the content stays in relatively the same position.
Comment on attachment 74338 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=74338&action=review Looking pretty good now. One more round should do it. I just don't like the directionChanged stuff, but everything else looks fine. > WebCore/page/FrameView.cpp:453 > + if (size == contentsSize() && root->directionChanged()) directionChanged() doesn't seem necessary to me. It seems like you can just check if scrollOriginX changes from zero to non-zero. > WebCore/platform/ScrollView.cpp:1038 > + // FIXME: need corresponding functionality from platformWidget. Does this FIXME still apply? > WebCore/rendering/RenderBox.cpp:316 > + if (viewStyle->direction() != style()->direction()) > + viewRenderer->setDirectionChanged(); As mentioned above, I don't think this directionChanged() boolean should be necessary. > WebCore/rendering/RenderBox.cpp:772 > + // its margins, and including its left layout overflow. > + int bx = tx - marginLeft() + view()->leftLayoutOverflow(); I don't like the addition to the comment. You're still just covering the entire canvas when you include leftLayoutOverflow, so I don't think you needed to change the comment at all. > WebCore/rendering/RenderView.h:166 > + void setDirectionChanged() {m_directionChanged = true;} > + bool directionChanged() {return m_directionChanged;} > + void resetDirectionChanged() {m_directionChanged = false;} Remove if you can. I don't think you should need it. > WebCore/rendering/RenderView.h:182 > + int docWidth(int) const; Include the parameter name here. It isn't obvious what the parameter is, so the name should be there. > WebCore/rendering/RenderView.h:243 > + bool m_directionChanged; Remove.
Created attachment 74752 [details] Patch w/layout tests David: Thanks for the review! The FIXME is indeed still relevant, I'll take care of it and the issues with zooming in a followup patch if that's ok.
Created attachment 74753 [details] Patch
Created attachment 75017 [details] Patch
Diff from previous patch: * Re-added workaround for 4213314 which I accidentally removed in previous versions. * Rebased on ToT.
Comment on attachment 75017 [details] Patch r=me.
Committed r72852: <http://trac.webkit.org/changeset/72852>
http://trac.webkit.org/changeset/72852 might have broken Qt Linux Release The following tests are not passing: fast/block/basic/truncation-rtl.html
This doesn't work in all cases: bug 55889.
Simon, this feature is currently completely broken because of the regression apparently caused by http://trac.webkit.org/changeset/76831/ see Bug 55077 for details.