Bug 102099

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

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

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

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

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

Created attachment 182126 [details] WIP Patch

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 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 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 :).

Created attachment 182794 [details] Patch

Created attachment 182960 [details] Patch

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 on attachment 182960 [details] Patch r=me

Comment on attachment 182960 [details] Patch Clearing flags on attachment: 182960 Committed r140014: <http://trac.webkit.org/changeset/140014>

All reviewed patches have been landed. Closing bug.