[CSS Regions] Compute correct region ranges for boxes

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Bucur]

Created attachment 202053 [details]
A test case

The region ranges for boxes are currently compute incorrectly in some cases (e.g. the attached file). The layout process should be smart enough to select the correct region for a block by taking into account constraints such as:
- unsplittable boxes (they should always use the start region for layout - see the attached test case)
- overflow content - on a case by case basis the content needs to clamp to the containing block region range

[Andrei Bucur]

Created attachment 203796 [details]
WIP Patch

[Build Bot]

Comment on attachment 203796 [details]
WIP Patch

Attachment 203796 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/662542

[Mihnea Ovidenie]

Comment on attachment 203796 [details]
WIP Patch

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

> Source/WebCore/ChangeLog:9
> +        - the range of a box is always included in the range of its containing block (even for floats); this should be temporary and

Temporary until when?

> Source/WebCore/ChangeLog:21
> +        Test: fast/regions/float-pushed-width-change-2.html

Will the final patch have more tests?

> Source/WebCore/ChangeLog:47
> +        (WebCore::RenderBox::clampToStartAndEndRegions):

This is the same function from RenderBlock but moved here i guess.

> Source/WebCore/rendering/RenderBlock.cpp:7744
> +    ASSERT((startRegion && endRegion) || (!startRegion && !endRegion));

When do you have the (!startRegion && !endRegion) situation, for multicol based regions?

> Source/WebCore/rendering/RenderFlowThread.cpp:375
> +RenderRegion* RenderFlowThread::regionAtBlockOffset(const RenderBlock* block, LayoutUnit offset, bool extendLastRegion, RegionAutoGenerationPolicy autoGenerationPolicy, bool clamp)

Assuming that the block is not null, i would add an ASSERT here.

> Source/WebCore/rendering/RenderFlowThread.cpp:384
> +        region = m_regionList.isEmpty() ? 0 : m_regionList.first();

Can you use here firstRegion() method on the RenderFlowThread?

> Source/WebCore/rendering/RenderFlowThread.cpp:528
> +RenderRegion* RenderFlowThread::mapFromFlowToRegion(const RenderBlock* block, TransformState& transformState) const

What is the meaning of block parameter here? Did you add it to be able to call regionAtBlockOffset or was for another purpose? Could you pass this instead of block below when calling regionAtBlockOffset?

> Source/WebCore/rendering/RenderFlowThread.cpp:684
> +void RenderFlowThread::clearRenderBoxRegioInfo(const RenderBox* box, RenderRegion* startRegion, RenderRegion* endRegion)

Regio -> Region

[Andrei Bucur]

Comment on attachment 203796 [details]
WIP Patch

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

>> Source/WebCore/ChangeLog:9
>> +        - the range of a box is always included in the range of its containing block (even for floats); this should be temporary and
> 
> Temporary until when?

Until we stabilize the regions overflow implementation. Having a child box in a region that's not in the range of its containing block brings more complexity to the code.

>> Source/WebCore/ChangeLog:21
>> +        Test: fast/regions/float-pushed-width-change-2.html
> 
> Will the final patch have more tests?

Yes. This is just WIP.

>> Source/WebCore/ChangeLog:47
>> +        (WebCore::RenderBox::clampToStartAndEndRegions):
> 
> This is the same function from RenderBlock but moved here i guess.

Yes, this functionality is not block specific.

>> Source/WebCore/rendering/RenderBlock.cpp:7744
>> +    ASSERT((startRegion && endRegion) || (!startRegion && !endRegion));
> 
> When do you have the (!startRegion && !endRegion) situation, for multicol based regions?

I initially added this before having the early return when there's no start or end region. It should have covered the case of an empty region chain. It will be removed in the final patch.

>> Source/WebCore/rendering/RenderFlowThread.cpp:375
>> +RenderRegion* RenderFlowThread::regionAtBlockOffset(const RenderBlock* block, LayoutUnit offset, bool extendLastRegion, RegionAutoGenerationPolicy autoGenerationPolicy, bool clamp)
> 
> Assuming that the block is not null, i would add an ASSERT here.

Will do. Actually, I think I'll remove the clamp parameter and if block == 0 then there's no clamping.

>> Source/WebCore/rendering/RenderFlowThread.cpp:384
>> +        region = m_regionList.isEmpty() ? 0 : m_regionList.first();
> 
> Can you use here firstRegion() method on the RenderFlowThread?

Will do.

>> Source/WebCore/rendering/RenderFlowThread.cpp:528
>> +RenderRegion* RenderFlowThread::mapFromFlowToRegion(const RenderBlock* block, TransformState& transformState) const
> 
> What is the meaning of block parameter here? Did you add it to be able to call regionAtBlockOffset or was for another purpose? Could you pass this instead of block below when calling regionAtBlockOffset?

When you map a coordinate local to a box you want to stay inside the region range of that box. If I pass "this" then it's no longer possible.

>> Source/WebCore/rendering/RenderFlowThread.cpp:684
>> +void RenderFlowThread::clearRenderBoxRegioInfo(const RenderBox* box, RenderRegion* startRegion, RenderRegion* endRegion)
> 
> Regio -> Region

Will do.

[Andrei Bucur]

Created attachment 204904 [details]
WIP Patch v2

[Build Bot]

Comment on attachment 204904 [details]
WIP Patch v2

Attachment 204904 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/950320

[Zoltan Horvath]

Comment on attachment 204904 [details]
WIP Patch v2

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

> Source/WebCore/rendering/RenderBox.cpp:124
> +    

too many spaces

> Source/WebCore/rendering/RenderBox.cpp:129
> +    

here too

> LayoutTests/platform/mac/TestExpectations:388
> +fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html
> LayoutTests/platform/mac/TestExpectations:389
> +fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html

I landed the patch from bug #117757 in http://trac.webkit.org/changeset/151703, so you can remove these two skips. Thanks!

[Andrei Bucur]

Created attachment 205095 [details]
Patch

[Andrei Bucur]

Created attachment 205098 [details]
Patch

[Build Bot]

Comment on attachment 205098 [details]
Patch

Attachment 205098 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/895902

[Mihnea Ovidenie]

Comment on attachment 205098 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [CSS Regions] Compute correct region ranges for boxes

Looks good, i have some comments below.

> Source/WebCore/rendering/RenderBlock.cpp:7811
> +    ASSERT(startRegion && endRegion);

setRegionRangeForBox already asserts about startRegion and endRegion, do you still need this one here?

> Source/WebCore/rendering/RenderBlock.cpp:7827
> +    box->computeLogicalHeight(LayoutUnit::max() / 2, logicalTopForChild(box), estimatedValues);

Can you add a comment why are you using LayoutUnit::max() / 2? Looking more at regions code, i see that LayoutUnit::max()/ 2 is used in some places, maybe we can factor it in a meaningfull define/function.

> Source/WebCore/rendering/RenderBlock.cpp:7833
> +    ASSERT(startRegion && endRegion);

Same here, do you still need the assert?

> Source/WebCore/rendering/RenderBox.cpp:147
> +    if (!startRegion || !endRegion)

Should we use an assert instead? Are there any cases in which a box does not have a range with start/end region? If not, i would revisit call sites for getRegionRangeForBox and add asserts where needed.

> Source/WebCore/rendering/RenderBox.cpp:152
> +    // ASSERT(clampToStartAndEndRegions(region) == region);

Can you add a bug for this please?

> Source/WebCore/rendering/RenderBox.cpp:154
> +    // FIXME: Remove once boxes are painted inside their region range.

And for this?

> Source/WebCore/rendering/RenderFlowThread.cpp:388
> +    RenderRegion* region = 0;

I guess we can simplify this function by early return when the region list is empty.

> Source/WebCore/rendering/RenderFlowThread.cpp:405
> +        return region ? clampBox->clampToStartAndEndRegions(region) : 0;

In which cases region is null here?

> Source/WebCore/rendering/RenderFlowThread.cpp:411
> +    return region ? clampBox->clampToStartAndEndRegions(region) : 0;

In which cases is region null here?

> Source/WebCore/rendering/RenderFlowThread.cpp:547
> +    RenderRegion* renderRegion = const_cast<RenderFlowThread*>(this)->regionAtBlockOffset(block, isHorizontalWritingMode() ? center.y() : center.x(), true, DisallowRegionAutoGeneration);

I read you previous explanation about why you need block passed here. However, the only call site for mapFromFlowToRegion is from mapLocalToContainer with the first parameter this == block. Maybe i am missing something..

> Source/WebCore/rendering/RenderFlowThread.cpp:603
> +        return true;

Can you explain this?

> Source/WebCore/rendering/RenderFlowThread.cpp:692
>      if (!hasRegions())

I guess this can be turned into an assert - the call site already tested it.

> Source/WebCore/rendering/RenderFlowThread.cpp:711
> +    if (!hasRegions())

Does it make sense to transform this into an assert? Looks like it is always called from places where you already tested that the flow has regions attached.

> LayoutTests/fast/regions/float-pushed-width-change-2-expected.html:5
> +    @font-face {

Are you using this?

> LayoutTests/fast/regions/float-pushed-width-change-2.html:5
> +   @font-face {

Are you using this declaration?

> LayoutTests/platform/efl/TestExpectations:543
> +# FIXME: The widows and orphans implementation interferes with the region range change relayout. 

Are you adding a bug for this?

[Radu Stavila]

Comment on attachment 205098 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:504
>      return region ? region->pageLogicalWidth() : contentLogicalWidth();

Should you be passing "true" for the extendLastRegion parameter?

[Radu Stavila]

Comment on attachment 205098 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2889
> +        r->layoutIfNeeded();

Why did you take the layoutIfNeeded call out of the if statement?

[Radu Stavila]

Comment on attachment 205098 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:7803
> +    LayoutUnit offsetFromLogicalTopOfFirstRegion = box->offsetFromLogicalTopOfFirstPage();

You're using "box" without validating it.

> Source/WebCore/rendering/RenderBlock.cpp:7815
> +void RenderBlock::estimateRegionRangeForBoxChild(RenderBox* box) const

Shouldn't box be const?

> Source/WebCore/rendering/RenderBlock.cpp:7837
> +void RenderBlock::updateRegionRangeForBoxChild(RenderBox* box) const

Shouldn't box be const?

> Source/WebCore/rendering/RenderBlock.cpp:7857
> +        box->setNeedsLayout(true, MarkOnlyThis);

I believe it would be better to have this method return true if the region range has changed and set the box as needing layout after calling this method. Either that or changing this method's name to indicate it will mark the box as needing layout.

> Source/WebCore/rendering/RenderFlowThread.cpp:695
> +    ASSERT(startRegion && endRegion);

Should box also be validated?

[Andrei Bucur]

Comment on attachment 205098 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:2889
>> +        r->layoutIfNeeded();
> 
> Why did you take the layoutIfNeeded call out of the if statement?

Initially I've placed updateRegionRangeForBoxChild() before the last layout so I took it out of the if. I'll put it back in the final patch.

>> Source/WebCore/rendering/RenderBlock.cpp:7803
>> +    LayoutUnit offsetFromLogicalTopOfFirstRegion = box->offsetFromLogicalTopOfFirstPage();
> 
> You're using "box" without validating it.

This function is called on the child boxes which can't be null. I don't see how a null reference can be passed to this function without being caught much earlier in the layout algorithm.

>> Source/WebCore/rendering/RenderBlock.cpp:7811
>> +    ASSERT(startRegion && endRegion);
> 
> setRegionRangeForBox already asserts about startRegion and endRegion, do you still need this one here?

No. I'll remove it.

>> Source/WebCore/rendering/RenderBlock.cpp:7815
>> +void RenderBlock::estimateRegionRangeForBoxChild(RenderBox* box) const
> 
> Shouldn't box be const?

Very likely it should be. I'll try to make it const in the next version.

>> Source/WebCore/rendering/RenderBlock.cpp:7827
>> +    box->computeLogicalHeight(LayoutUnit::max() / 2, logicalTopForChild(box), estimatedValues);
> 
> Can you add a comment why are you using LayoutUnit::max() / 2? Looking more at regions code, i see that LayoutUnit::max()/ 2 is used in some places, maybe we can factor it in a meaningfull define/function.

I'll add a paragraph about this in the ChangeLog.

>> Source/WebCore/rendering/RenderBlock.cpp:7833
>> +    ASSERT(startRegion && endRegion);
> 
> Same here, do you still need the assert?

No.

>> Source/WebCore/rendering/RenderBlock.cpp:7837
>> +void RenderBlock::updateRegionRangeForBoxChild(RenderBox* box) const
> 
> Shouldn't box be const?

If I leave the setNeedsLayout call there, no.

>> Source/WebCore/rendering/RenderBlock.cpp:7857
>> +        box->setNeedsLayout(true, MarkOnlyThis);
> 
> I believe it would be better to have this method return true if the region range has changed and set the box as needing layout after calling this method. Either that or changing this method's name to indicate it will mark the box as needing layout.

You are right this is a bit black-boxish, but considering you always need to make a layout whenever you call this function, I've placed the code inside it to minimize duplication.

>> Source/WebCore/rendering/RenderBox.cpp:147
>> +    if (!startRegion || !endRegion)
> 
> Should we use an assert instead? Are there any cases in which a box does not have a range with start/end region? If not, i would revisit call sites for getRegionRangeForBox and add asserts where needed.

Inline blocks don't have a range assigned to them. This is just a simplified fix for those cases.

>> Source/WebCore/rendering/RenderBox.cpp:152
>> +    // ASSERT(clampToStartAndEndRegions(region) == region);
> 
> Can you add a bug for this please?

After the patch lands, yes.

>> Source/WebCore/rendering/RenderBox.cpp:154
>> +    // FIXME: Remove once boxes are painted inside their region range.
> 
> And for this?

Same :).

>> Source/WebCore/rendering/RenderFlowThread.cpp:388
>> +    RenderRegion* region = 0;
> 
> I guess we can simplify this function by early return when the region list is empty.

Will do.

>> Source/WebCore/rendering/RenderFlowThread.cpp:405
>> +        return region ? clampBox->clampToStartAndEndRegions(region) : 0;
> 
> In which cases region is null here?

I don't think it can be null here, you are right.

>> Source/WebCore/rendering/RenderFlowThread.cpp:411
>> +    return region ? clampBox->clampToStartAndEndRegions(region) : 0;
> 
> In which cases is region null here?

If extendLastRegion is false the region can be null.

>> Source/WebCore/rendering/RenderFlowThread.cpp:504
>>      return region ? region->pageLogicalWidth() : contentLogicalWidth();
> 
> Should you be passing "true" for the extendLastRegion parameter?

This is what the old code did.

>> Source/WebCore/rendering/RenderFlowThread.cpp:547
>> +    RenderRegion* renderRegion = const_cast<RenderFlowThread*>(this)->regionAtBlockOffset(block, isHorizontalWritingMode() ? center.y() : center.x(), true, DisallowRegionAutoGeneration);
> 
> I read you previous explanation about why you need block passed here. However, the only call site for mapFromFlowToRegion is from mapLocalToContainer with the first parameter this == block. Maybe i am missing something..

The mapLocalToContainer function was refactored a while back and made virtual. This code predates that change. I'll change the parameter.

>> Source/WebCore/rendering/RenderFlowThread.cpp:603
>> +        return true;
> 
> Can you explain this?

Yes, this is an ugly fix to force flow thread children to relayout and compute their ranges. The following code doesn't work when this == the render flow thread because it will always have updated region range info. I think I can use the m_pageLogicalSizeChanged flag again to have this return true only when necessary.

>> Source/WebCore/rendering/RenderFlowThread.cpp:692
>>      if (!hasRegions())
> 
> I guess this can be turned into an assert - the call site already tested it.

Will do.

>> Source/WebCore/rendering/RenderFlowThread.cpp:695
>> +    ASSERT(startRegion && endRegion);
> 
> Should box also be validated?

As I previously said the code is very integrated and it should never happen to call this with a null box.

>> Source/WebCore/rendering/RenderFlowThread.cpp:711
>> +    if (!hasRegions())
> 
> Does it make sense to transform this into an assert? Looks like it is always called from places where you already tested that the flow has regions attached.

Will do.

>> LayoutTests/fast/regions/float-pushed-width-change-2-expected.html:5
>> +    @font-face {
> 
> Are you using this?

No, I'll remove it.

>> LayoutTests/fast/regions/float-pushed-width-change-2.html:5
>> +   @font-face {
> 
> Are you using this declaration?

Same as above.

>> LayoutTests/platform/efl/TestExpectations:543
>> +# FIXME: The widows and orphans implementation interferes with the region range change relayout. 
> 
> Are you adding a bug for this?

When the patch lands, yes.

[Andrei Bucur]

Created attachment 206324 [details]
Patch

[Andrei Bucur]

Created attachment 208540 [details]
Patch

[Dave Hyatt]

Comment on attachment 208540 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 208540 [details]
Patch

Clearing flags on attachment: 208540

Committed r153990: <http://trac.webkit.org/changeset/153990>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.