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

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

({dev-doc-complete})

Trunk
mozilla61
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 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).
Priority: -- → P3
JFYI, this is now implemented in Chromium, WebKit and Edge.
(Assignee)

Updated

a year ago
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+
(Assignee)

Comment 10

a year 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.)
(Assignee)

Comment 11

a year ago
Addressed the comment nit.
Attachment #8962206 - Attachment is obsolete: true
Attachment #8964201 - Flags: review+
(Assignee)

Comment 12

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

Comment 14

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

Comment 15

a year 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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
status-firefox57: affected → ---
You need to log in before you can comment on or make changes to this bug.