Bug 129330 - [New Multicolumn] Add support for column-span:all
Summary: [New Multicolumn] Add support for column-span:all
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-25 13:28 PST by Morten Stenshorne
Modified: 2014-04-21 10:35 PDT (History)
17 users (show)

See Also:


Attachments
Test case (929 bytes, text/html)
2014-02-25 13:28 PST, Morten Stenshorne
no flags Details
Patch (351.81 KB, patch)
2014-02-26 15:48 PST, Morten Stenshorne
no flags Details | Formatted Diff | Diff
Patch (381.75 KB, patch)
2014-04-08 12:14 PDT, Morten Stenshorne
no flags Details | Formatted Diff | Diff
Patch (358.51 KB, patch)
2014-04-15 10:27 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch that should get Windows building too. (359.08 KB, patch)
2014-04-15 10:56 PDT, Dave Hyatt
hyatt: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (929.90 KB, application/zip)
2014-04-15 12:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (983.63 KB, application/zip)
2014-04-15 12:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (1.01 MB, application/zip)
2014-04-15 13:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (933.96 KB, application/zip)
2014-04-15 13:13 PDT, Build Bot
no flags Details
Reset the results for client-rects.html now that the flow thread grows to the height of the number of columns. (363.85 KB, patch)
2014-04-15 14:51 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Removed the test case with the subpixel difference, since it's not wrong, just a bit different. (368.20 KB, patch)
2014-04-15 15:28 PDT, Dave Hyatt
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Morten Stenshorne 2014-02-25 13:28:04 PST
Created attachment 225179 [details]
Test case

Add support for -webkit-column-span:all to the new ("region based") multicol implementation.
Comment 1 Morten Stenshorne 2014-02-26 15:05:14 PST
Corresponding bug in Blink: https://code.google.com/p/chromium/issues/detail?id=347325
Comment 2 Morten Stenshorne 2014-02-26 15:48:46 PST
Created attachment 225314 [details]
Patch
Comment 3 Morten Stenshorne 2014-02-26 15:49:51 PST
Comment on attachment 225314 [details]
Patch

This isn't finished yet. Just submitting this patch for Dave to have a look.
Comment 4 Dave Hyatt 2014-04-04 11:22:17 PDT
This is done! New patch will be coming from Morten once 122754 lands, and then we'll get it reviewed.
Comment 5 Morten Stenshorne 2014-04-08 12:14:21 PDT
Created attachment 228865 [details]
Patch
Comment 6 Morten Stenshorne 2014-04-08 12:28:40 PDT
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.
Comment 7 Dave Hyatt 2014-04-15 10:27:15 PDT
Created attachment 229375 [details]
Patch
Comment 8 Dave Hyatt 2014-04-15 10:28:16 PDT
I uploaded an unchanged patch that adds the spanner placeholder to the XCode project.
Comment 9 Dave Hyatt 2014-04-15 10:56:00 PDT
Created attachment 229379 [details]
Patch that should get Windows building too.
Comment 10 Build Bot 2014-04-15 12:01:10 PDT
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
Comment 11 Build Bot 2014-04-15 12:01:18 PDT
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
Comment 12 Build Bot 2014-04-15 12:40:24 PDT
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
Comment 13 Build Bot 2014-04-15 12:40:29 PDT
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
Comment 14 Build Bot 2014-04-15 13:05:39 PDT
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
Comment 15 Build Bot 2014-04-15 13:05:47 PDT
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
Comment 16 Build Bot 2014-04-15 13:13:33 PDT
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
Comment 17 Build Bot 2014-04-15 13:13:39 PDT
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
Comment 18 Dave Hyatt 2014-04-15 14:35:51 PDT
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.)
Comment 19 Dave Hyatt 2014-04-15 14:51:53 PDT
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.
Comment 20 Dave Hyatt 2014-04-15 14:52:46 PDT
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.
Comment 21 Dave Hyatt 2014-04-15 14:53:44 PDT
I fixed the populate() issue by manually setting childrenInline to false before inserting the flow thread. This stops makeChildrenNonInline from being called.
Comment 22 Dave Hyatt 2014-04-15 15:28:45 PDT
Created attachment 229409 [details]
Removed the test case with the subpixel difference, since it's not wrong, just a bit different.
Comment 23 Dave Hyatt 2014-04-15 16:26:29 PDT
Comment on attachment 229409 [details]
Removed the test case with the subpixel difference, since it's not wrong, just a bit different.

r=me
Comment 24 Dave Hyatt 2014-04-15 16:26:51 PDT
Landed in r167335.
Comment 25 Carlos Alberto Lopez Perez 2014-04-21 10:35:24 PDT
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.