Bug 101862 - [CSSRegions] Incorrect computed height for content with region-break-before
Summary: [CSSRegions] Incorrect computed height for content with region-break-before
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords: AdobeTracked
Depends on:
Blocks:
 
Reported: 2012-11-11 10:24 PST by Mihnea Ovidenie
Modified: 2012-11-13 03:39 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.77 KB, patch)
2012-11-11 12:20 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (6.82 KB, patch)
2012-11-13 00:56 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2012-11-11 10:24:56 PST
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.
Comment 1 Mihnea Ovidenie 2012-11-11 12:20:05 PST
Created attachment 173514 [details]
Patch
Comment 2 Julien Chaffraix 2012-11-12 09:33:25 PST
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.
Comment 3 Mihnea Ovidenie 2012-11-13 00:56:37 PST
Created attachment 173845 [details]
Patch for landing
Comment 4 WebKit Review Bot 2012-11-13 03:39:46 PST
Comment on attachment 173845 [details]
Patch for landing

Clearing flags on attachment: 173845

Committed r134395: <http://trac.webkit.org/changeset/134395>
Comment 5 WebKit Review Bot 2012-11-13 03:39:50 PST
All reviewed patches have been landed.  Closing bug.