Bug 74020 - [CSS Regions] Absolutely positioned elements that span RTL regions of different widths are positioned incorrectly
Summary: [CSS Regions] Absolutely positioned elements that span RTL regions of differe...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2011-12-07 12:49 PST by Alan Stearns
Modified: 2013-08-06 05:19 PDT (History)
10 users (show)

See Also:


Attachments
reftest (1.21 KB, application/zip)
2011-12-07 12:49 PST, Alan Stearns
no flags Details
WIP patch (905 bytes, patch)
2013-06-21 10:36 PDT, Anton Obzhirov
no flags Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2013-07-05 06:10 PDT, Anton Obzhirov
hyatt: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Stearns 2011-12-07 12:49:29 PST
Created attachment 118255 [details]
reftest

If you have an absolutely positioned element that spans two regions of different widths and direction is set to RTL, positioning after the first region is set from the left edge instead of the right.
Comment 1 Michelangelo De Simone 2013-06-13 20:58:20 PDT
Still repros on today's nightly (r151543)
Comment 2 Anton Obzhirov 2013-06-14 02:16:38 PDT
If Mihnea and Simon don't mind I can have a look on this one.
Comment 3 Simon Pena 2013-06-14 02:21:31 PDT
(In reply to comment #2)
> If Mihnea and Simon don't mind I can have a look on this one.

Please, go ahead, I am a bit busy and don't think I can take a look too soon.
Comment 4 Anton Obzhirov 2013-06-21 10:33:41 PDT
Update: after some investigation - the code fails to position boxes properly because during calculation of the box rectangle in borderBoxRectInRegion at some point in computeInlineStaticDistance during calculation of logicalRight the region it calculates rect for is not in the range of regions assigned for the box (and it is not in the range because logical height of the box is 0 and only the first one of three regions gets added when region range is set).

So anyway I have WIP patch but not sure about it - may be someone can check that I am going in right direction - I am going to double check the patch after my trip.
Comment 5 Anton Obzhirov 2013-06-21 10:36:13 PDT
Created attachment 205196 [details]
WIP patch
Comment 6 Anton Obzhirov 2013-07-05 06:10:39 PDT
Created attachment 206144 [details]
Patch
Comment 7 Dave Hyatt 2013-07-16 09:45:32 PDT
Comment on attachment 206144 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1541
> +    LayoutUnit oldTop = logicalTop();
> +
> +    setLogicalHeight(LayoutUnit::max() / 2);
> +    updateLogicalHeight();

This seems wrong to me. I fail to see why the height would not be correct, since we finished child layout and just before calling updateRegionsAndShapesAfterChildLayout we even did updateLogicalHeight in RenderBlock.

I think I'm going to need more detail before I'm going to believe this should be necessary.
Comment 8 Anton Obzhirov 2013-07-31 05:56:14 PDT
Comment on attachment 206144 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:1541
>> +    updateLogicalHeight();
> 
> This seems wrong to me. I fail to see why the height would not be correct, since we finished child layout and just before calling updateRegionsAndShapesAfterChildLayout we even did updateLogicalHeight in RenderBlock.
> 
> I think I'm going to need more detail before I'm going to believe this should be necessary.

I checked it again - the reason why height is 0 here because the render block the function is called for is actually empty (it is marked as #content in the test html page and the nested box #green-box is absolutely positioned).
So the #content block region range includes only first region and when later the block is mistakenly considered to be containing block for #green-box during width update. In particular
during computeInlineStaticDistance call logical right for green boxes inside second and third regions gets calculated wrongly because it clamps all regions to the first one.
So my idea was to expand it temporarily to calculate correct range but now I am thinking that I just hide this problem in computeInlineStaticDistance.
Comment 9 Anton Obzhirov 2013-07-31 06:03:52 PDT
So I am considering now another approach - for absolute positioned blocks add check to clamp to the region range of container block (named flow): 

+++ b/Source/WebCore/rendering/RenderBox.cpp
@@ -3047,7 +3047,10 @@ static void computeInlineStaticDistance(Length& logicalLeft, Length& logicalRigh
                     staticPosition -= enclosingBox->logicalWidth();
                 if (region && curr->isRenderBlock()) {
                     const RenderBlock* cb = toRenderBlock(curr);
-                    region = cb->clampToStartAndEndRegions(region);
+                    if (child->isOutOfFlowPositioned())
+                        region = toRenderBlock(containerBlock)->clampToStartAndEndRegions(region);
+                    else
+                        region = cb->clampToStartAndEndRegions(region);
                     RenderBoxRegionInfo* boxInfo = cb->renderBoxRegionInfo(region);
                     if (boxInfo) {
                         if (curr != containerBlock)

What do you think about this new approach?
Comment 10 Anton Obzhirov 2013-08-06 05:19:33 PDT
Some additional information:

--- a/Source/WebCore/rendering/RenderBox.cpp
+++ b/Source/WebCore/rendering/RenderBox.cpp
@@ -3047,7 +3047,10 @@ static void computeInlineStaticDistance(Length& logicalLeft, Length& logicalRigh
                     staticPosition -= enclosingBox->logicalWidth();
                 if (region && curr->isRenderBlock()) {
                     const RenderBlock* cb = toRenderBlock(curr);
-                    region = cb->clampToStartAndEndRegions(region);
+                    if (child->isOutOfFlowPositioned())
+                        region = toRenderBlock(child->container())->clampToStartAndEndRegions(region);
+                    else
+                        region = cb->clampToStartAndEndRegions(region);
                     RenderBoxRegionInfo* boxInfo = cb->renderBoxRegionInfo(region);
                     if (boxInfo) {
                         if (curr != containerBlock)

Now it should always clamp to proper containing block region range during calculating of static offset.

After checking description of computeInlineStaticDistance – as far as I understand it tries to calculate left (LTR) or right (RTL) offsets for the render box
in a region considering all render boxes inside named flow as static (even if they are abs positioned). So far so good and it does calculate
them properly for most of the cases, however the function doesn’t take into account that if all boxes considered static then some boxes will have different sizes (as they get previously unaccounted children (abs positioned children) and hence region ranges for those boxes will be different as well.

For example in this case we have for the content for the flow:

<div id="content">
    <div id="green-box"></div>
</div>

Since #green-box is absolute positioned the height of #content after layout is 0 and the region range contains only first region.
If #green-box was static it would have height of named flow and the range would contain all three regions.

So that’s what I am trying to work around with the proposed change.
In ideal world before computeInlineStaticDistance it should temporarily layout all blocks again considering absolute positioned boxes as static and recalculate all ranges and return back to previous layout ranges after computeInlineStaticDistance.