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

Sat May 22 23:39:01 PDT 2010

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

mitz at webkit.org changed:

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

--- Comment #34 from mitz at webkit.org  2010-05-22 23:39:00 PST ---
(From update of attachment 53324)
> +    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.

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