Bug 129330 – [New Multicolumn] Add support for column-span:all

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Morten Stenshorne]

Created attachment 225179 [details]
Test case

Add support for -webkit-column-span:all to the new ("region based") multicol implementation.

[Morten Stenshorne]

Corresponding bug in Blink: https://code.google.com/p/chromium/issues/detail?id=347325

[Morten Stenshorne]

Created attachment 225314 [details]
Patch

[Morten Stenshorne]

Comment on attachment 225314 [details]
Patch

This isn't finished yet. Just submitting this patch for Dave to have a look.

[Dave Hyatt]

This is done! New patch will be coming from Morten once 122754 lands, and then we'll get it reviewed.

[Morten Stenshorne]

Created attachment 228865 [details]
Patch

[Morten Stenshorne]

I'm done! :) Well, not quite. Apart from addressing review issues, this patch is missing one important part that I need help with: I've added new source files, and I don't know how to properly make changes in Source/WebCore/WebCore.xcodeproj/project.pbxproj , which I assume is what you use on Mac. Some magical-looking hex codes in this file helped me resist the temptation of hand-editing this.

[Dave Hyatt]

Created attachment 229375 [details]
Patch

[Dave Hyatt]

I uploaded an unchanged patch that adds the spanner placeholder to the XCode project.

[Dave Hyatt]

Created attachment 229379 [details]
Patch that should get Windows building too.

[Build Bot]

Comment on attachment 229379 [details]
Patch that should get Windows building too.

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

New failing tests:
fast/multicol/newmulticol/client-rects.html
fast/multicol/newmulticol/compare-with-old-impl/span-as-immediate-child-complex-splitting.html

[Build Bot]

Created attachment 229389 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 229379 [details]
Patch that should get Windows building too.

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

New failing tests:
fast/multicol/newmulticol/client-rects.html
fast/multicol/newmulticol/compare-with-old-impl/span-as-immediate-child-complex-splitting.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 229379 [details]
Patch that should get Windows building too.

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

New failing tests:
fast/multicol/newmulticol/client-rects.html
fast/multicol/newmulticol/compare-with-old-impl/span-as-immediate-child-complex-splitting.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 229379 [details]
Patch that should get Windows building too.

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

New failing tests:
fast/multicol/newmulticol/client-rects.html
fast/multicol/newmulticol/compare-with-old-impl/span-as-immediate-child-complex-splitting.html

[Build Bot]

Created attachment 229397 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

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

[Dave Hyatt]

Comment on attachment 229379 [details]
Patch that should get Windows building too.

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

> Source/WebCore/rendering/RenderBlockFlow.cpp:121
> -    moveAllChildrenTo(flowThread, true);
>      RenderBlock::addChild(flowThread);
> +    flowThread->populate();

This part is wrong. By delaying the population of the flow thread until after the flow thread has been inserted into the flow thread parent, you cause an anonymous block to be created for no reason to enclose all of the inline content. This inline content is going into the flow thread, so we don't want to wastefully wrap it in an unnecessary anonymous block.

(This issue was caught by the client-rects.html layout test on OS X.)

[Dave Hyatt]

Created attachment 229406 [details]
Reset the results for client-rects.html now that the flow thread grows to the height of the number of columns.

[Dave Hyatt]

There is a subpixel layout difference with the spanners that is causing 

span-as-immediate-child-complex-splitting.html

to fail. Looking into that now.

[Dave Hyatt]

I fixed the populate() issue by manually setting childrenInline to false before inserting the flow thread. This stops makeChildrenNonInline from being called.

[Dave Hyatt]

Created attachment 229409 [details]
Removed the test case with the subpixel difference, since it's not wrong, just a bit different.

[Dave Hyatt]

Comment on attachment 229409 [details]
Removed the test case with the subpixel difference, since it's not wrong, just a bit different.

r=me

[Dave Hyatt]

Landed in r167335.

[Carlos Alberto Lopez Perez]

One question.... did you tested the -expected layout test results for the GTK port (file LayoutTests/platform/gtk/fast/multicol/newmulticol/client-rects-expected.txt added in r167335) ?

Because, since it has been added, it has been giving failures on the GTK port. 

The diff is the following: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r167336%20%2846517%29/fast/multicol/newmulticol/

I'm going to rebaseline it to the actual result unless you see something wrong with that diff.