Mats Palmgren (inactive) Assignee
	Description • 7 years ago

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

	Manuel Rego Casasnovas
	Comment 1 • 6 years ago

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

	Daniel Holbert [:dholbert]
	Comment 6 • 6 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 7 • 6 years ago

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

	Daniel Holbert [:dholbert]
	Comment 8 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 9 • 6 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 10 • 6 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 11 • 6 years ago

Addressed the comment nit.

	Mats Palmgren (inactive) Assignee
	Comment 12 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 13 • 6 years ago

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

	Pulsebot
	Comment 14 • 6 years ago

Pushed by ecoal95@gmail.com:
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).

	Cosmin Sabou [:CosminS]
	Comment 15 • 6 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 16 • 6 years ago

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.