Bug 74020 – [CSS Regions] Absolutely positioned elements that span RTL regions of different widths are positioned incorrectly

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Alan Stearns]

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.

[Michelangelo De Simone]

Still repros on today's nightly (r151543)

[Anton Obzhirov]

If Mihnea and Simon don't mind I can have a look on this one.

[Simon Pena]

(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.

[Anton Obzhirov]

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.

[Anton Obzhirov]

Created attachment 205196 [details]
WIP patch

[Anton Obzhirov]

Created attachment 206144 [details]
Patch

[Dave Hyatt]

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.

[Anton Obzhirov]

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.

[Anton Obzhirov]

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?

[Anton Obzhirov]

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.

[Brent Fulgham]

CSS Regions were removed in Bug 174978.