Bug 102099 – [CSS Regions] Content flows incorrectly in autoheight regions with min/max-height set

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mike Sierra]

Created attachment 173911 [details]
result #1: dynamic resizing

Copy source example into local file.

[Mike Sierra]

Created attachment 173912 [details]
result #2: after page reload

[Mike Sierra]

Created attachment 173913 [details]
result #3: when max-height is 0

[Mike Sierra]

Copy the HTML example into a local file & load with Canary or Webkit nightly. The content threads through several CSS regions, each of which is thresholded with a min-/max-height. Resize the window and you see many cases in which text jumps to the next region before it has filled the max-height of the previous region (result#1). In some cases, reloading the page causes regions to break differently (result #2). Also, try removing the min-height using the link at the top and resizing. With min-height:0, content often skips regions, leaving them empty (result #3). Content sometimes accumulates in the last region in the chain (result #3).

[Mike Sierra]

apologies: couldn't access the "description" field after adding screen-shot attachments. Prior comment describes the bug.

[Mike Sierra]

I may have mixed up screen shots for results #1 & #2, but note they both render poorly.

[Andrei Bucur]

Created attachment 182126 [details]
WIP Patch

[Alexandru Chiculita]

I think this might be actually related to https://bugs.webkit.org/show_bug.cgi?id=103738 . I've added a comment explaining what happens on that bug.

[Alexandru Chiculita]

Comment on attachment 182126 [details]
WIP Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:623
> +    for (RenderRegionList::iterator iter = m_regionList.begin(); iter != m_regionList.end(); ++iter) {

This will be called after each block, so iterating on the regions will add another regions lookup.

> Source/WebCore/rendering/RenderFlowThread.cpp:642
> +        closeUnbrokeAutoHeightRegions(height);

This is just going to work on block elements, not on individual lines.

> Source/WebCore/rendering/RenderFlowThread.cpp:839
> +            else if (region->needsOverrideLogicalContentHeightComputation())
> +                regionLogicalHeight = region->maxPageLogicalHeight();

You don't need to make the regions that big. You could just make sure that every time you move from one line to the next one, you are not bypassing the max-height of the region. At that point you could "soft-break" the region using the previous offset in the flow and move the line to the next region.

> Source/WebCore/rendering/RenderFlowThread.h:158
> +    void closeUnbrokeAutoHeightRegions(LayoutUnit flowHeight);

I don't like the naming as regions are not broken :)

[Andrei Bucur]

Comment on attachment 182126 [details]
WIP Patch

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

>> Source/WebCore/rendering/RenderFlowThread.cpp:623
>> +    for (RenderRegionList::iterator iter = m_regionList.begin(); iter != m_regionList.end(); ++iter) {
> 
> This will be called after each block, so iterating on the regions will add another regions lookup.

computeOverflowStateForRegions is called only for the flow threads, not all the blocks.

>> Source/WebCore/rendering/RenderFlowThread.cpp:642
>> +        closeUnbrokeAutoHeightRegions(height);
> 
> This is just going to work on block elements, not on individual lines.

I don't understand what you mean by that. Could you please be more explicit?

>> Source/WebCore/rendering/RenderFlowThread.cpp:839
>> +                regionLogicalHeight = region->maxPageLogicalHeight();
> 
> You don't need to make the regions that big. You could just make sure that every time you move from one line to the next one, you are not bypassing the max-height of the region. At that point you could "soft-break" the region using the previous offset in the flow and move the line to the next region.

From my experience with implementing region styling for font size where something similar was required, you risk filling the code with all kind of if cases (e.g. you need to look at determineStartPosition and when putting end lines back in the line boxes collection of a the block etc.). I think this is cleaner.

>> Source/WebCore/rendering/RenderFlowThread.h:158
>> +    void closeUnbrokeAutoHeightRegions(LayoutUnit flowHeight);
> 
> I don't like the naming as regions are not broken :)

Mkay :).

[Andrei Bucur]

Created attachment 182794 [details]
Patch

[Andrei Bucur]

Created attachment 182960 [details]
Patch

[Andrei Bucur]

Comment on attachment 182960 [details]
Patch

Modified autoheight-maxheight-simple-nobreak.html to use different values for max-height in the region chain. This should test the case when all the regions start with the same height in the normal layout phase.

[Dave Hyatt]

Comment on attachment 182960 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 182960 [details]
Patch

Clearing flags on attachment: 182960

Committed r140014: <http://trac.webkit.org/changeset/140014>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.