Bug 116677

Summary: Improve multicol intrinsic width calculation
Product: WebKit Reporter: Morten Stenshorne <mstensho>
Component: Layout and RenderingAssignee: Morten Stenshorne <mstensho>
Severity: Normal CC: buildbot, cdumez, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, gyuyoung.kim, hyatt, kondapallykalyan, rakuco, rniwa, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
TC - Intrinsic multicol width, both column count and width specified
TC - Intrinsic multicol width, column count specified
TC - Intrinsic multicol width, column width specified
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
Patch none

Description Morten Stenshorne 2013-05-23 08:28:52 PDT
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.
Comment 1 Morten Stenshorne 2013-05-23 08:33:45 PDT
Created attachment 202718 [details]
TC - Intrinsic multicol width, both column count and width specified
Comment 2 Morten Stenshorne 2013-05-23 08:34:36 PDT
Created attachment 202719 [details]
TC - Intrinsic multicol width, column count specified
Comment 3 Morten Stenshorne 2013-05-23 08:35:03 PDT
Created attachment 202720 [details]
TC - Intrinsic multicol width, column width specified
Comment 4 Morten Stenshorne 2013-05-23 08:41:23 PDT
Created attachment 202721 [details]
Comment 5 Build Bot 2013-05-24 00:30:19 PDT
Comment on attachment 202721 [details]

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

New failing tests:
Comment 6 Build Bot 2013-05-24 00:30:21 PDT
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
Comment 7 Morten Stenshorne 2013-05-24 01:40:06 PDT
The svg/batik/text/textFeatures.svg failure looks bogus. The two others I'm responsible for.
Comment 8 Morten Stenshorne 2013-05-24 01:48:34 PDT
Created attachment 202788 [details]
Comment 9 Dave Hyatt 2013-08-05 13:44:54 PDT
Comment on attachment 202788 [details]

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.
Comment 10 Morten Stenshorne 2013-08-05 14:49:46 PDT
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.
Comment 11 Morten Stenshorne 2013-08-27 13:35:55 PDT
Created attachment 209792 [details]
Comment 12 Dave Hyatt 2013-08-27 13:45:14 PDT
Comment on attachment 209792 [details]

Comment 13 WebKit Commit Bot 2013-08-27 16:16:13 PDT
Comment on attachment 209792 [details]

Clearing flags on attachment: 209792

Committed r154714: <http://trac.webkit.org/changeset/154714>
Comment 14 WebKit Commit Bot 2013-08-27 16:16:19 PDT
All reviewed patches have been landed.  Closing bug.