[Webkit-unassigned] [Bug 23556] Right-to-left pages should be scrollable to reveal left overflow

Tue Jul 27 15:07:22 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=23556

Xiaomei Ji <xji at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xji at chromium.org

--- Comment #39 from Xiaomei Ji <xji at chromium.org>  2010-07-27 15:07:22 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.