Bug 182004 – [css-multicol] Support percentages in column-gap

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

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).

[EWS Watchlist]

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.

[Javier Fernandez]

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 ?

[Manuel Rego Casasnovas]

Created attachment 332263 [details]
Patch

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

Created attachment 332268 [details]
Patch

[Javier Fernandez]

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));

[Manuel Rego Casasnovas]

Created attachment 332367 [details]
Patch for landing

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

Created attachment 332373 [details]
Rebased patch

[WebKit Commit Bot]

Comment on attachment 332373 [details]
Rebased patch

Clearing flags on attachment: 332373

Committed r227676: <https://trac.webkit.org/changeset/227676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36908657>

[zalan]

This patch caused a security issue -> bug 184724

[Manuel Rego Casasnovas]

(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. :-/

[zalan]

(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

[Manuel Rego Casasnovas]

(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!