Bug 182004

Summary: [css-multicol] Support percentages in column-gap
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, ews-watchlist, jfernandez, simon.fraser, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=799413
Bug Depends on: 182044    
Bug Blocks: 180290    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing
none
Rebased patch none

Manuel Rego Casasnovas
Reported 2018-01-23 12:41:13 PST
Chromium and Edge are the only browser supporting percentages in "column-gap" at this point. However all browsers support percentage in "grid-column-gap", which will be renamed to "column-gap" (bug #180290). So it seems it'd be a nice moment to add support in Multicolumn too. The Multicolumn spec has not any special text regarding this (https://drafts.csswg.org/css-multicol/#column-gap), however it's worth noticing the text in css-align spec (https://drafts.csswg.org/css-align-3/#column-row-gap): "Percentages resolve to zero when specified against a content-based size (such as the logical width of a float or the auto logical height of a block-level grid container)." Note that there's some ongoing discussion with the CSS WG to clarify it: https://github.com/w3c/csswg-drafts/issues/509#issuecomment-355242101 I believe the text in the spec needs to be changed for "column-gap", as the width is always definite during layout, so we have to resolve the percentages. At least I don't see how in Blink/WebKit we could detect the cases that try to cover the new text in the spec, things like the width of a floated element is only indefinite during intrinsic size computation (when the percentages are treated as 0) but during layout we've a definite width to resolve percentages against it. IMHO this should work like percentages widths work on regular blocks. And that's what has been implemented in both Chromium and Edge.
Attachments
Patch (24.09 KB, patch)
2018-01-24 05:23 PST, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (40.47 KB, patch)
2018-01-25 07:19 PST, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (38.89 KB, patch)
2018-01-25 08:17 PST, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch for landing (40.41 KB, patch)
2018-01-26 05:11 PST, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Rebased patch (40.41 KB, patch)
2018-01-26 06:33 PST, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 2018-01-24 05:23:52 PST
Created attachment 332149 [details] Patch Patch is ready but the test http://w3c-test.org/css/css-multicol/multicol-gap-percentage-001.html, but we need to update some paths of WPT tests before being able to import it (see bug #182044).
EWS Watchlist
Comment 2 2018-01-24 05:25:08 PST
Attachment 332149 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:25: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Javier Fernandez
Comment 3 2018-01-24 06:18:39 PST
Comment on attachment 332149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332149&action=review > Source/WebCore/ChangeLog:25 > + No new tests (OOPS!). Explain why we don't need new tests. > Source/WebCore/page/FrameView.cpp:719 > + LayoutUnit columnGap; Perhaps we can use a ternary conditional expression and avoid a double assignment. Something like this: GapLength columnGapLength = documentOrBodyRenderer->style().columnGap(); LayoutUnit columnGap = columnGapLength.isNormal() ? documentOrBodyRenderer->style().fontDescription().computedPixelSize() : valueForLength(columnGapLength.length(), downcast<RenderBox>(documentOrBodyRenderer)->availableLogicalWidth()) > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:95 > + if (from.isNormal() || to.isNormal()) I think we can implement this in 1 line. > Source/WebCore/rendering/style/StyleMultiColData.h:53 > + GapLength columnGap; I think we are increasing the RenderStyle size here. Shouldn't we update the SameSizeAsRenderStyle assert accordingly ?
Manuel Rego Casasnovas
Comment 4 2018-01-25 07:19:17 PST
Created attachment 332263 [details] Patch
Manuel Rego Casasnovas
Comment 5 2018-01-25 07:20:47 PST
Comment on attachment 332149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332149&action=review Thanks for the review. >> Source/WebCore/ChangeLog:25 >> + No new tests (OOPS!). > > Explain why we don't need new tests. As I explained in a comment, this was not ready for review at that point, I was still importing some new tests from css-multicol WPT suite. >> Source/WebCore/page/FrameView.cpp:719 >> + LayoutUnit columnGap; > > Perhaps we can use a ternary conditional expression and avoid a double assignment. Something like this: > > GapLength columnGapLength = documentOrBodyRenderer->style().columnGap(); > LayoutUnit columnGap = columnGapLength.isNormal() ? documentOrBodyRenderer->style().fontDescription().computedPixelSize() : valueForLength(columnGapLength.length(), downcast<RenderBox>(documentOrBodyRenderer)->availableLogicalWidth()) Done. >> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:95 >> + if (from.isNormal() || to.isNormal()) > > I think we can implement this in 1 line. Done. >> Source/WebCore/rendering/style/StyleMultiColData.h:53 >> + GapLength columnGap; > > I think we are increasing the RenderStyle size here. Shouldn't we update the SameSizeAsRenderStyle assert accordingly ? It seems it's not needed, we're adding a boolean in GapLength, but we're removing the normalGap bool too.
Manuel Rego Casasnovas
Comment 6 2018-01-25 08:17:06 PST
Created attachment 332268 [details] Patch
Javier Fernandez
Comment 7 2018-01-26 02:36:04 PST
Comment on attachment 332268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332268&action=review > Source/WebCore/css/StyleBuilderConverter.h:1545 > + auto& primitiveValue = downcast<CSSPrimitiveValue>(value); What about using a ternary conditional statement here ? We would avoid declaring the primitiveValue variable, which is used just once. return downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNormal ? GapLength() : GapLength(convertLength(styleResolver, value));
Manuel Rego Casasnovas
Comment 8 2018-01-26 05:11:49 PST
Created attachment 332367 [details] Patch for landing
Manuel Rego Casasnovas
Comment 9 2018-01-26 05:12:59 PST
Comment on attachment 332268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332268&action=review Thanks for the review. >> Source/WebCore/css/StyleBuilderConverter.h:1545 >> + auto& primitiveValue = downcast<CSSPrimitiveValue>(value); > > What about using a ternary conditional statement here ? We would avoid declaring the primitiveValue variable, which is used just once. > > return downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNormal ? GapLength() : GapLength(convertLength(styleResolver, value)); Done.
Manuel Rego Casasnovas
Comment 10 2018-01-26 06:33:50 PST
Created attachment 332373 [details] Rebased patch
WebKit Commit Bot
Comment 11 2018-01-26 06:57:52 PST
Comment on attachment 332373 [details] Rebased patch Clearing flags on attachment: 332373 Committed r227676: <https://trac.webkit.org/changeset/227676>
WebKit Commit Bot
Comment 12 2018-01-26 06:57:53 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 13 2018-01-26 06:58:22 PST
<rdar://problem/36908657>
alan
Comment 14 2018-05-09 09:22:54 PDT
This patch caused a security issue -> bug 184724
Manuel Rego Casasnovas
Comment 15 2018-05-09 09:57:08 PDT
(In reply to zalan from comment #14) > This patch caused a security issue -> bug 184724 Sorry but I don't have permissions to see that bug. :-/
alan
Comment 16 2018-05-09 10:05:08 PDT
(In reply to Manuel Rego Casasnovas from comment #15) > (In reply to zalan from comment #14) > > This patch caused a security issue -> bug 184724 > > Sorry but I don't have permissions to see that bug. :-/ Fix got landed here https://trac.webkit.org/changeset/231379
Manuel Rego Casasnovas
Comment 17 2018-05-09 10:11:36 PDT
(In reply to zalan from comment #16) > (In reply to Manuel Rego Casasnovas from comment #15) > > (In reply to zalan from comment #14) > > > This patch caused a security issue -> bug 184724 > > > > Sorry but I don't have permissions to see that bug. :-/ > Fix got landed here https://trac.webkit.org/changeset/231379 Yeah it makes sense, sorry for missing it in the original patch. Thanks for fixing it!
Note You need to log in before you can comment on or make changes to this bug.