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

Sun May 23 22:30:29 PDT 2010

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


--- Comment #35 from Hironori Bono <hbono at chromium.org>  2010-05-23 22:30:28 PST ---
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.

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