Chromium and Edge are the only browser supporting percentages in "column-gap" at this point. However all browsers support percentage in "grid-column-gap", which will be renamed to "column-gap" (bug #180290). So it seems it'd be a nice moment to add support in Multicolumn too. The Multicolumn spec has not any special text regarding this (https://drafts.csswg.org/css-multicol/#column-gap), however it's worth noticing the text in css-align spec (https://drafts.csswg.org/css-align-3/#column-row-gap): "Percentages resolve to zero when specified against a content-based size (such as the logical width of a float or the auto logical height of a block-level grid container)." Note that there's some ongoing discussion with the CSS WG to clarify it: https://github.com/w3c/csswg-drafts/issues/509#issuecomment-355242101 I believe the text in the spec needs to be changed for "column-gap", as the width is always definite during layout, so we have to resolve the percentages. At least I don't see how in Blink/WebKit we could detect the cases that try to cover the new text in the spec, things like the width of a floated element is only indefinite during intrinsic size computation (when the percentages are treated as 0) but during layout we've a definite width to resolve percentages against it. IMHO this should work like percentages widths work on regular blocks. And that's what has been implemented in both Chromium and Edge.
Created attachment 332149 [details] Patch Patch is ready but the test http://w3c-test.org/css/css-multicol/multicol-gap-percentage-001.html, but we need to update some paths of WPT tests before being able to import it (see bug #182044).
Attachment 332149 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:25: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332149&action=review > Source/WebCore/ChangeLog:25 > + No new tests (OOPS!). Explain why we don't need new tests. > Source/WebCore/page/FrameView.cpp:719 > + LayoutUnit columnGap; Perhaps we can use a ternary conditional expression and avoid a double assignment. Something like this: GapLength columnGapLength = documentOrBodyRenderer->style().columnGap(); LayoutUnit columnGap = columnGapLength.isNormal() ? documentOrBodyRenderer->style().fontDescription().computedPixelSize() : valueForLength(columnGapLength.length(), downcast<RenderBox>(documentOrBodyRenderer)->availableLogicalWidth()) > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:95 > + if (from.isNormal() || to.isNormal()) I think we can implement this in 1 line. > Source/WebCore/rendering/style/StyleMultiColData.h:53 > + GapLength columnGap; I think we are increasing the RenderStyle size here. Shouldn't we update the SameSizeAsRenderStyle assert accordingly ?
Created attachment 332263 [details] Patch
Comment on attachment 332149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332149&action=review Thanks for the review. >> Source/WebCore/ChangeLog:25 >> + No new tests (OOPS!). > > Explain why we don't need new tests. As I explained in a comment, this was not ready for review at that point, I was still importing some new tests from css-multicol WPT suite. >> Source/WebCore/page/FrameView.cpp:719 >> + LayoutUnit columnGap; > > Perhaps we can use a ternary conditional expression and avoid a double assignment. Something like this: > > GapLength columnGapLength = documentOrBodyRenderer->style().columnGap(); > LayoutUnit columnGap = columnGapLength.isNormal() ? documentOrBodyRenderer->style().fontDescription().computedPixelSize() : valueForLength(columnGapLength.length(), downcast<RenderBox>(documentOrBodyRenderer)->availableLogicalWidth()) Done. >> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:95 >> + if (from.isNormal() || to.isNormal()) > > I think we can implement this in 1 line. Done. >> Source/WebCore/rendering/style/StyleMultiColData.h:53 >> + GapLength columnGap; > > I think we are increasing the RenderStyle size here. Shouldn't we update the SameSizeAsRenderStyle assert accordingly ? It seems it's not needed, we're adding a boolean in GapLength, but we're removing the normalGap bool too.
Created attachment 332268 [details] Patch
Comment on attachment 332268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332268&action=review > Source/WebCore/css/StyleBuilderConverter.h:1545 > + auto& primitiveValue = downcast<CSSPrimitiveValue>(value); What about using a ternary conditional statement here ? We would avoid declaring the primitiveValue variable, which is used just once. return downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNormal ? GapLength() : GapLength(convertLength(styleResolver, value));
Created attachment 332367 [details] Patch for landing
Comment on attachment 332268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332268&action=review Thanks for the review. >> Source/WebCore/css/StyleBuilderConverter.h:1545 >> + auto& primitiveValue = downcast<CSSPrimitiveValue>(value); > > What about using a ternary conditional statement here ? We would avoid declaring the primitiveValue variable, which is used just once. > > return downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNormal ? GapLength() : GapLength(convertLength(styleResolver, value)); Done.
Created attachment 332373 [details] Rebased patch
Comment on attachment 332373 [details] Rebased patch Clearing flags on attachment: 332373 Committed r227676: <https://trac.webkit.org/changeset/227676>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36908657>
This patch caused a security issue -> bug 184724
(In reply to zalan from comment #14) > This patch caused a security issue -> bug 184724 Sorry but I don't have permissions to see that bug. :-/
(In reply to Manuel Rego Casasnovas from comment #15) > (In reply to zalan from comment #14) > > This patch caused a security issue -> bug 184724 > > Sorry but I don't have permissions to see that bug. :-/ Fix got landed here https://trac.webkit.org/changeset/231379
(In reply to zalan from comment #16) > (In reply to Manuel Rego Casasnovas from comment #15) > > (In reply to zalan from comment #14) > > > This patch caused a security issue -> bug 184724 > > > > Sorry but I don't have permissions to see that bug. :-/ > Fix got landed here https://trac.webkit.org/changeset/231379 Yeah it makes sense, sorry for missing it in the original patch. Thanks for fixing it!