1398537 - [multicol] Implement support for column-gap:<percentage> in multicol layout

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, enhancement, P3)

Description

[Mats Palmgren (inactive)]

https://logs.csswg.org/irc.w3.org/css/2017-08-04/#e847329
"Resolved: column-gap and row-gap apply to flex, grid, and multicol"

'column-gap' is defined in Box Alignment spec:
https://www.w3.org/TR/css-align-3/#column-row-gap
"Value: 	normal | <length-percentage>"

We don't support <percentage> yet, but bug 1398482 will change that.
http://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/layout/style/nsCSSPropList.h#1513

We should implement <percentage> support in multicol layout here
(and the style part too, if that's more convenient).

Comment 1

[Manuel Rego Casasnovas]

JFYI, this is now implemented in Chromium, WebKit and Edge.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-multicol] Implement percentages for 'column-gap' (Stylo part)

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-multicol] Implement percentages for 'column-gap' (Gecko part)

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-multicol] Implement percentages for 'column-gap' (automated update of WPT results)

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-multicol] Implement percentages for 'column-gap' (automated update of devtools)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8962206 [details] [diff] [review]
part 2 - [css-multicol] Implement percentages for 'column-gap' (Gecko part)

Review of attachment 8962206 [details] [diff] [review]:
-----------------------------------------------------------------

[starting with "part 2" since the gecko code here is more familiar to me]

r=me, one nit:

::: layout/base/nsLayoutUtils.h
@@ +3055,5 @@
>    static uint32_t ParseFontLanguageOverride(const nsAString& aLangTag);
>  
> +  /**
> +   * Resolve a column-gap/row-gap to a definite size.
> +   * @note this method resolves 'normal' to zero.

Could you add at the end of this @note:
 Callers who want different behavior should handle 'normal' on their own.

Without that change, the current @note seems to half-imply that zero is the correct resolved value of column-gap/row-gap:normal in general.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8962205 [details] [diff] [review]
part 1 - [css-multicol] Implement percentages for 'column-gap' (Stylo part)

Review of attachment 8962205 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, though probably emilio or xidorn should sanity-check, because I haven't really written/reviewed any stylo code on the rust side of things before.

(I also suspect this part would needs to land in https://github.com/servo/servo/ and get merged over, rather than landing on mozilla-inbound, though I'm not sure.)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8962205 [details] [diff] [review]
part 1 - [css-multicol] Implement percentages for 'column-gap' (Stylo part)

Review of attachment 8962205 [details] [diff] [review]:
-----------------------------------------------------------------

(er, I guess emilio has more context here [having commented over on bug 1398482], so I probably should've punted the review to him. Though if xidorn gets to this first, that works too. :))

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962205 [details] [diff] [review]
part 1 - [css-multicol] Implement percentages for 'column-gap' (Stylo part)

Review of attachment 8962205 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thank you :)

I assume this comes with tests on the other part. r=me if that's the case :)

Comment 10

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Could you add at the end of this @note:
>  Callers who want different behavior should handle 'normal' on their own.

Done, thanks.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> I assume this comes with tests on the other part. r=me if that's the case :)

Yes, part 3 shows WPT failures that now pass, so I assume they test this :)

(I'll make sure there are also tests for calc()-percentages in bug 1434478,
once the CSSWG has decided how they should behave.)

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-multicol] Implement percentages for 'column-gap' (Gecko part)

Addressed the comment nit.

Comment 12

[Mats Palmgren (inactive)]

Any chance you could help me land this next week if you got some spare time?
Last time I marked one of these stylo changes with checkin-needed the sheriff
screwed it up and burned the tree...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Sure thing, https://github.com/servo/servo/pull/20499. I had to fix it up a bit to avoid breaking Servo code.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7ae048e7422
part 2 - [css-multicol] Implement percentages for 'column-gap' (Gecko part).  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a00e328cf69a
part 3 - [css-multicol] Implement percentages for 'column-gap' (automated update of WPT results).
https://hg.mozilla.org/integration/autoland/rev/a56cd855e314
part 4 - [css-multicol] Implement percentages for 'column-gap' (automated update of devtools).

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a7ae048e7422
https://hg.mozilla.org/mozilla-central/rev/a00e328cf69a
https://hg.mozilla.org/mozilla-central/rev/a56cd855e314

Comment 16

[Chris Mills [:cmills]]

We've updated the browser compat data to reflect this change:

https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap#Browser_compatibility

And added a note to the Fx61 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/61#CSS

The multi-col docs generally need some work, but we will get to these soon.