[Webkit-unassigned] [Bug 23556] On RTL pages, horizontal scrollbar is missing

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62608|review?                     |review-
               Flag|                            |




--- Comment #38 from mitz at webkit.org  2010-07-26 15:21:32 PST ---
(From update of attachment 62608)
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.

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