Summary: | Improve multicol intrinsic width calculation | ||
---|---|---|---|
Product: | WebKit | Reporter: | Morten Stenshorne <mstensho> |
Component: | Layout and Rendering | Assignee: | Morten Stenshorne <mstensho> |
Status: | RESOLVED FIXED | ||
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 | ||
Attachments: |
Description
Morten Stenshorne
2013-05-23 08:28:52 PDT
Created attachment 202718 [details]
TC - Intrinsic multicol width, both column count and width specified
Created attachment 202719 [details]
TC - Intrinsic multicol width, column count specified
Created attachment 202720 [details]
TC - Intrinsic multicol width, column width specified
Created attachment 202721 [details]
Patch
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 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
The svg/batik/text/textFeatures.svg failure looks bogus. The two others I'm responsible for. Created attachment 202788 [details]
Patch
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. 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. Created attachment 209792 [details]
Patch
Comment on attachment 209792 [details]
Patch
r=me
Comment on attachment 209792 [details] Patch Clearing flags on attachment: 209792 Committed r154714: <http://trac.webkit.org/changeset/154714> All reviewed patches have been landed. Closing bug. |