Bug 101862 – [CSSRegions] Incorrect computed height for content with region-break-before

• Component: CSS
• Status: RESOLVED FIXED

[Mihnea Ovidenie]

When content to be flowed into an auto-height region has a -webkit-region-break-before: always, the height for the auto-height region (first in region chain) is computed incorrectly.

[Mihnea Ovidenie]

Created attachment 173514 [details]
Patch

[Julien Chaffraix]

Comment on attachment 173514 [details]
Patch

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

r=me

> LayoutTests/ChangeLog:13
> +        * fast/regions/autoheight-breakbefore-wrongheight-expected.html: Added.
> +        * fast/regions/autoheight-breakbefore-wrongheight.html: Added.

I would prefer this test to be a check-layout.js test as it would convey the intent better, on top of being faster to run.

> Source/WebCore/rendering/RenderFlowThread.cpp:879
> +
> +        // If the current offset if greater than the break offset, bail out and skip the current region.
> +        if (currentRegionOffsetInFlowThread >= offsetBreakInFlowThread) {
> +            ++regionIter;
> +            break;
> +        }

Note that if you don't change the for loop's end condition, this could also be pulled out of the loop and written:

if (regionIter != m_regionList.end())
   ++regionIter;

The upside of that being that it is guaranteed to run (even if you refactor the inner loop), on top of being run only once instead of once per loop iteration. The downside being that it's a bit less clear so I don't have a strong preference for either form.

[Mihnea Ovidenie]

Created attachment 173845 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173845 [details]
Patch for landing

Clearing flags on attachment: 173845

Committed r134395: <http://trac.webkit.org/changeset/134395>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.