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)
Core
Layout: Block and Inline
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)
5.65 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
Details | Diff | Splinter Review | |
1.06 KB,
patch
|
Details | Diff | Splinter Review | |
9.39 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
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).
Updated•7 years ago
|
Priority: -- → P3
Comment 1•6 years ago
|
||
JFYI, this is now implemented in Chromium, WebKit and Edge.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mats
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8962205 -
Flags: review?(dholbert)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8962206 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
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.
Attachment #8962206 -
Flags: review?(dholbert) → review+
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.)
Attachment #8962205 -
Flags: review?(dholbert) → review?(xidorn+moz)
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. :))
Attachment #8962205 -
Flags: review?(xidorn+moz) → review?(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 :)
Attachment #8962205 -
Flags: review?(emilio) → review+
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.)
Assignee | ||
Comment 11•6 years ago
|
||
Addressed the comment nit.
Attachment #8962206 -
Attachment is obsolete: true
Attachment #8964201 -
Flags: review+
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...
Flags: needinfo?(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.
Flags: needinfo?(emilio)
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).
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
status-firefox57:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•