[webkit-reviews] review denied: [Bug 66641] [CSSRegions] Fix Element::getBoundingClientRect and Element::getClientRects for content flow : [Attachment 105000] Patch V1

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 66641: [CSSRegions] Fix Element::getBoundingClientRect and
Element::getClientRects for content flow
https://bugs.webkit.org/show_bug.cgi?id=66641

Attachment 105000: Patch V1
https://bugs.webkit.org/attachment.cgi?id=105000&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105000&action=review


> Source/WebCore/rendering/RenderFlowThread.cpp:536
> +    // Transform state contains the coordinates of the original box in
RenderFlowThread coordinates.
> +    transformState.move(-flippedRegionRect.x() +
renderRegion->borderAndPaddingLogicalLeft(),
> +			   -flippedRegionRect.y() +
renderRegion->borderAndPaddingLogicalTop());

This does not look correct to me. It looks like you're adding logical values to
physical values here.

There are two tricky bits to worry about with writing modes. The first is when
the writing mode is vertical. In that case the logicalXXX methods return
horizontal values rather than vertical and vice versa. In other words x becomes
y and y becomes x. The second thing you have to deal with is the "flipped
blocks" writing modes. These are horizontal-bt and vertical-rl. For flipped
writing modes, the x-axis for local block coordinates increases from right to
left (so 0 is on the right), and for horizontal-bt the y-axis increases from
bottom to top. This is inverted compared to the most common writing mode,
horizontal-tb (most languages). flipForWritingMode is the method that deals
with flipped blocks. It doesn't have anything to do with horizontal vs.
vertical.

Therefore in the code above you grabbed the local region rect, which will be
flipped if the RenderFlowThread is vertical-rl, so when you did flip for
writing mode you put it in physical coordinates. Moving the transform state by
logical values based off the region's writing mode doesn't make much sense to
me, since you're adding logical values to physical values. It's not quite clear
to me what this code is trying to do.