Closed Bug 1398537 Opened 7 years ago Closed 6 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 1 obsolete file)

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).
Priority: -- → P3
JFYI, this is now implemented in Chromium, WebKit and Edge.
Assignee: nobody → mats
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.
Attachment #8962206 - Flags: review?(dholbert) → review+
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.)
Attachment #8962205 - Flags: review?(dholbert) → review?(xidorn+moz)
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. :))
Attachment #8962205 - Flags: review?(xidorn+moz) → review?(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 :)
Attachment #8962205 - Flags: review?(emilio) → review+
(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.)
Addressed the comment nit.
Attachment #8962206 - Attachment is obsolete: true
Attachment #8964201 - Flags: review+
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...
Flags: needinfo?(emilio)
Sure thing, https://github.com/servo/servo/pull/20499. I had to fix it up a bit to avoid breaking Servo code.
Flags: needinfo?(emilio)
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).
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.
You need to log in before you can comment on or make changes to this bug.