[Webkit-unassigned] [Bug 85856] [Chromium] Cannot show the left side of an RTL element with a vertical scrollbar

[bugzilla-daemon at webkit.org]

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


Julien Chaffraix <jchaffraix at webkit.org> changed:

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




--- Comment #44 from Julien Chaffraix <jchaffraix at webkit.org>  2012-10-22 14:28:23 PST ---
(From update of attachment 158240)
View in context: https://bugs.webkit.org/attachment.cgi?id=158240&action=review

> Source/WebCore/ChangeLog:12
> +        layout offsets.

'right' is used 5 times and the way it's written is confusing: I had a hard time saying if you are talking about the direction and just make them 'right'.

>> Source/WebCore/ChangeLog:27
>> +        (WebCore::RenderBlock::paintChildren): Move the paint rectangles of RTL children right.
> 
> It feels wrong that we're moving the paint rectangles.  Why is it better to move the paint offset rather than changing the RenderObject's offset?

I agree with Tony here: we should probably patch the offsets at layout time and store them updated instead of requiring this code. Your approach only invites bugs in our code as we can easily forget a code path. I couldn't find any in the repaint code, but hit-testing looks suspicious as only RenderBlock was updated (but neither RenderReplaced nor RenderLayer was).

> Source/WebCore/dom/Element.cpp:421
> +        if (renderer->style() && renderer->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft())

Can we really have renderer->style() be NULL? Do you know when this occurs?

> Source/WebCore/rendering/RenderBlock.cpp:1681
> +            addOverflowFromChild(positionedObject, IntSize(positionedObject->x(), positionedObject->y()));

This should be LayoutSize.

> Source/WebCore/rendering/RenderBox.cpp:1576
> +    if (o->style() && o->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() && o->isBox())

Again why do you NULL check style() here when none of the surrounding code does this check?

Also it should be |styleToUse|, we cache style() (IIRC it is because region may have made this call more expensive).

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