Bug 66641 - [CSSRegions] Fix Element::getBoundingClientRect and Element::getClientRects for content flow
Summary: [CSSRegions] Fix Element::getBoundingClientRect and Element::getClientRects f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on: 117295
Blocks: 57312
  Show dependency treegraph
 
Reported: 2011-08-21 23:12 PDT by Alexandru Chiculita
Modified: 2013-06-06 07:47 PDT (History)
2 users (show)

See Also:


Attachments
Patch V1 (19.88 KB, patch)
2011-08-24 08:26 PDT, Alexandru Chiculita
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch V2 (18.71 KB, patch)
2011-08-24 12:25 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-08-21 23:12:24 PDT
Add tests and fix the following case: Element::getBoundingClientRect and Element::getClientRects return the correct values for elements inside a flow.
Comment 1 Alexandru Chiculita 2011-08-24 08:26:13 PDT
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 2 Dave Hyatt 2011-08-24 09:27:42 PDT
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 3 Alexandru Chiculita 2011-08-24 09:41:39 PDT
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.
Comment 4 Alexandru Chiculita 2011-08-24 12:25:57 PDT
Created attachment 105041 [details]
Patch V2
Comment 5 Dave Hyatt 2011-08-24 13:06:58 PDT
Comment on attachment 105041 [details]
Patch V2

r=me
Comment 6 WebKit Review Bot 2011-08-24 13:21:11 PDT
Comment on attachment 105041 [details]
Patch V2

Clearing flags on attachment: 105041

Committed r93728: <http://trac.webkit.org/changeset/93728>
Comment 7 WebKit Review Bot 2011-08-24 13:21:16 PDT
All reviewed patches have been landed.  Closing bug.