Bug 66641

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.

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.

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.

Created attachment 105041 [details] Patch V2

Comment on attachment 105041 [details] Patch V2 r=me

Comment on attachment 105041 [details] Patch V2 Clearing flags on attachment: 105041 Committed r93728: <http://trac.webkit.org/changeset/93728>

All reviewed patches have been landed. Closing bug.