[CSSRegions] Fix Element::getBoundingClientRect and Element::getClientRects for content flow

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Add tests and fix the following case: Element::getBoundingClientRect and Element::getClientRects return the correct values for elements inside a flow.

[Alexandru Chiculita]

Created attachment 105000 [details]
Patch V1

Inital patch was done by Mihnea, I've just updated to use the RenderFlowThread::renderRegionForLine that was added in a previous patch and added some more tests.

[Dave Hyatt]

Comment on attachment 105000 [details]
Patch V1

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.

[Alexandru Chiculita]

Comment on attachment 105000 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=105000&action=review

>> Source/WebCore/rendering/RenderFlowThread.cpp:536
>> +                        -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.

Thanks for the explanation! I think this should work in a similar way as the repaint method, so I will refactor it tomorrow.

[Alexandru Chiculita]

Created attachment 105041 [details]
Patch V2

[Dave Hyatt]

Comment on attachment 105041 [details]
Patch V2

r=me

[WebKit Review Bot]

Comment on attachment 105041 [details]
Patch V2

Clearing flags on attachment: 105041

Committed r93728: <http://trac.webkit.org/changeset/93728>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.