Bug 102099 - [CSS Regions] Content flows incorrectly in autoheight regions with min/max-height set
Summary: [CSS Regions] Content flows incorrectly in autoheight regions with min/max-he...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrei Bucur
URL: https://github.com/mike-sierra/webpla...
Keywords: AdobeTracked
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2012-11-13 09:38 PST by Mike Sierra
Modified: 2013-01-17 11:37 PST (History)
9 users (show)

See Also:


Attachments
result #1: dynamic resizing (186.80 KB, image/png)
2012-11-13 09:38 PST, Mike Sierra
no flags Details
result #2: after page reload (187.19 KB, image/png)
2012-11-13 09:39 PST, Mike Sierra
no flags Details
result #3: when max-height is 0 (159.97 KB, image/png)
2012-11-13 09:40 PST, Mike Sierra
no flags Details
WIP Patch (12.42 KB, patch)
2013-01-10 06:36 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (79.16 KB, patch)
2013-01-15 09:45 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (77.45 KB, patch)
2013-01-16 04:36 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Sierra 2012-11-13 09:38:47 PST
Created attachment 173911 [details]
result #1: dynamic resizing

Copy source example into local file.
Comment 1 Mike Sierra 2012-11-13 09:39:51 PST
Created attachment 173912 [details]
result #2: after page reload
Comment 2 Mike Sierra 2012-11-13 09:40:26 PST
Created attachment 173913 [details]
result #3: when max-height is 0
Comment 3 Mike Sierra 2012-11-13 09:45:09 PST
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).
Comment 4 Mike Sierra 2012-11-13 09:50:53 PST
apologies: couldn't access the "description" field after adding screen-shot attachments. Prior comment describes the bug.
Comment 5 Mike Sierra 2012-11-13 10:42:28 PST
I may have mixed up screen shots for results #1 & #2, but note they both render poorly.
Comment 6 Andrei Bucur 2013-01-10 06:36:26 PST
Created attachment 182126 [details]
WIP Patch
Comment 7 Alexandru Chiculita 2013-01-10 10:14:27 PST
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.
Comment 8 Alexandru Chiculita 2013-01-10 13:48:15 PST
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 :)
Comment 9 Andrei Bucur 2013-01-10 14:26:27 PST
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 :).
Comment 10 Andrei Bucur 2013-01-15 09:45:34 PST
Created attachment 182794 [details]
Patch
Comment 11 Andrei Bucur 2013-01-16 04:36:12 PST
Created attachment 182960 [details]
Patch
Comment 12 Andrei Bucur 2013-01-16 05:48:35 PST
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.
Comment 13 Dave Hyatt 2013-01-17 10:54:23 PST
Comment on attachment 182960 [details]
Patch

r=me
Comment 14 WebKit Review Bot 2013-01-17 11:37:09 PST
Comment on attachment 182960 [details]
Patch

Clearing flags on attachment: 182960

Committed r140014: <http://trac.webkit.org/changeset/140014>
Comment 15 WebKit Review Bot 2013-01-17 11:37:13 PST
All reviewed patches have been landed.  Closing bug.