Improve multicol intrinsic width calculation

[Morten Stenshorne]

There's no finished spec that describes in detail exactly how to calculate minimum and maximum intrinsic widths for multicol, but what we do currently can be improved. There's also the following ED to guide us: http://dev.w3.org/csswg/css-sizing/#multicol-intrinsic

We already have bug 45138 about intrinsic width and multicol, but since the particular TC there is very tricky to fix, and there are other much more trivial cases to fix, I'm reporting this bug to submit a patch against.

[Morten Stenshorne]

Created attachment 202718 [details]
TC - Intrinsic multicol width, both column count and width specified

[Morten Stenshorne]

Created attachment 202719 [details]
TC - Intrinsic multicol width, column count specified

[Morten Stenshorne]

Created attachment 202720 [details]
TC - Intrinsic multicol width, column width specified

[Morten Stenshorne]

Created attachment 202721 [details]
Patch

[Build Bot]

Comment on attachment 202721 [details]
Patch

Attachment 202721 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/688007

New failing tests:
fast/multicol/positioned-with-constrained-height.html
svg/batik/text/textFeatures.svg
css3/unicode-bidi-isolate-basic.html

[Build Bot]

Created attachment 202777 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Morten Stenshorne]

The svg/batik/text/textFeatures.svg failure looks bogus. The two others I'm responsible for.

[Morten Stenshorne]

Created attachment 202788 [details]
Patch

[Dave Hyatt]

Comment on attachment 202788 [details]
Patch

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

Looks ok, but it might be good to have this code in RenderMultiColumnBlock as well.

> Source/WebCore/rendering/RenderBlock.cpp:5883
> +    if (!style()->hasAutoColumnCount() || !style()->hasAutoColumnWidth()) {
> +        // The min/max intrinsic widths calculated really tell how much space elements need when
> +        // laid out inside the columns. In order to eventually end up with the desired column width,
> +        // we need to convert them to values pertaining to the multicol container.
> +        int columnCount = style()->hasAutoColumnCount() ? 1 : style()->columnCount();
> +        LayoutUnit columnWidth;
> +        LayoutUnit gapExtra = (columnCount - 1) * columnGap();
> +        if (style()->hasAutoColumnWidth())
> +            minLogicalWidth = minLogicalWidth * columnCount + gapExtra;
> +        else {
> +            columnWidth = style()->columnWidth();
> +            minLogicalWidth = min(minLogicalWidth, columnWidth);
> +        }
> +        // FIXME: If column-count is auto here, we should resolve it to calculate the maximum
> +        // intrinsic width, instead of pretending that it's 1. The only way to do that is by
> +        // performing a layout pass, but this is not an appropriate time or place for layout. The
> +        // good news is that if height is unconstrained and there are no explicit breaks, the
> +        // resolved column-count really should be 1.
> +        maxLogicalWidth = max(maxLogicalWidth, columnWidth) * columnCount + gapExtra;
> +    }

In terms of the future multi-column layout, wouldn't this be better off in RenderMultiColumnBlock? It looks like the only reason you put this in RenderBlock is so that the old multi-column layout code can use it. It might be worth going ahead and duplicating the code so that when the time comes to rip out all the multi-column code in RenderBlock, we don't accidentally remove this piece and break RenderMultiColumnBlock.

[Morten Stenshorne]

I take you want me to override computeIntrinsicLogicalWidths() in RenderMultiColumnBlock?

If so, I don't think that's a good idea. This code should stay in RenderBlock, and only there, also for the new multicol code.

The multicol calculations of computeIntrinsicLogicalWidths() need to take place after having calculated the "normal" min/max preferred widths, but before adding space for scrollbars, for instance.

[Morten Stenshorne]

Created attachment 209792 [details]
Patch

[Dave Hyatt]

Comment on attachment 209792 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 209792 [details]
Patch

Clearing flags on attachment: 209792

Committed r154714: <http://trac.webkit.org/changeset/154714>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.