Bug 238854 – [Multicol] Incorrect clipping when a layer is present between the column and the content layer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

Created attachment 456775 [details]
Test case

see test case

[Radar WebKit Bug Importer]

<rdar://problem/91438700>

[Rob Buis]

There seems to be a difference between the clipping div being relative (positioned or static). This is the render tree for test case:

(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLSC --  RenderView at (0,0) size 1728x944 renderer->(0x1430008b0)
B-----LS- --    HTML RenderBlock at (0,0) size 1728x944 renderer->(0x143001860) node->(0x1430011c0)
B-------- --      BODY RenderBody at (8,8) size 1712x928 renderer->(0x143001ad0) node->(0x14303f730)
B-----L-- --        DIV RenderBlock at (0,0) size 206x56 renderer->(0x14301be60) node->(0x14304cc90)
B---YGL-- --          RenderMultiColumnFlowThread at (3,3) size 92x100 renderer->(0x143073150) (layout overflow 0,0 400x100) [fragment containers 0x143073360]
BR----L-- --            DIV RenderBlock at (0,0) size 92x100 renderer->(0x14301c1e0) node->(0x143072fa0) (layout overflow 0,0 400x100) (visual overflow 0,0 400x100) [spans fragment containers in flow 0x143073150 from 0x143073360 to 0x143073360]
B-------- --              DIV RenderBlock at (0,0) size 400x100 renderer->(0x14301c410) node->(0x143073030) [spans fragment containers in flow 0x143073150 from 0x143073360 to 0x143073360]
-------- --                Line: (top: 0 bottom: 18) with leading (top: 0 bottom: 18)
-------- --                RootInlineBox at (0,0) size 167x18 (0x143065040) renderer->(0x14301c410)
-------- --                  InlineTextBox at (0,0) size 167x18 (0x143073580) renderer->(0x14301c550) run(0, 28) "PASS if this text is visible"
I-------- --                #text RenderText renderer->(0x14301c550) node->(0x10341a1b0) length->(28) "PASS if this text is visible"
B---YG--- --*         RenderMultiColumnSet at (3,3) size 200x50 renderer->(0x143073360) (column count 2, size 92x50, gap 16)

This is the render tree for test case with position: relative removed:

(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLSC -+  RenderView at (0,0) size 1728x0 renderer->(0x1430008b0) layout->[normal child]
B-----LS- -+    HTML RenderBlock at (0,0) size 1728x0 renderer->(0x143001860) node->(0x1430011c0) layout->[self][normal child]
B-------- -+      BODY RenderBody at (8,8) size 1712x0 renderer->(0x143001ad0) node->(0x14303f730) layout->[self][normal child]
B-----L-- -+        DIV RenderBlock at (0,0) size 206x3 renderer->(0x14301be60) node->(0x14304cc90) layout->[self][normal child]
B---YGL-- --          RenderMultiColumnFlowThread at (3,3) size 92x100 renderer->(0x143073150) (layout overflow 0,0 400x100) (visual overflow 0,0 400x100) [fragment containers 0x143073360]
B-------- --            DIV RenderBlock at (0,0) size 92x100 renderer->(0x14301c1e0) node->(0x143072fa0) (layout overflow 0,0 400x100) (visual overflow 0,0 400x100) [spans fragment containers in flow 0x143073150 from 0x143073360 to 0x143073360]
B-------- --              DIV RenderBlock at (0,0) size 400x100 renderer->(0x14301c410) node->(0x143073030) [spans fragment containers in flow 0x143073150 from 0x143073360 to 0x143073360]
-------- --                Line: (top: 0 bottom: 18) with leading (top: 0 bottom: 18)
-------- --                RootInlineBox at (0,0) size 167x18 (0x143065040) renderer->(0x14301c410)
-------- --                  InlineTextBox at (0,0) size 167x18 (0x143073580) renderer->(0x14301c550) run(0, 28) "PASS if this text is visible"
I-------- --                #text RenderText renderer->(0x14301c550) node->(0x10341a1b0) length->(28) "PASS if this text is visible"
B---YG--- --*         RenderMultiColumnSet at (3,3) size 200x50 renderer->(0x143073360) (column count 2, size 92x50, gap 16)

RenderFragmentContainer::overflowRectForFragmentedFlowPortion asks visualOverflowRectForBox on the RenderMultiColumnFlowThread. Since the RenderMultiColumnFlowThread in the first case has no visual overflow, there is clipping against columns, and in the second case there is clipping of 400x100, hence the green rect is unclipped as expected.

[Rob Buis]

Created attachment 462435 [details]
Patch

[Rob Buis]

@Alan do you think the patch is close? I wonder if RenderMultiColumnFlowThread always should collect the kid(s) visual overflow and that it is the root cause.

[zalan]

(In reply to Rob Buis from comment #4)
> @Alan do you think the patch is close? I wonder if
> RenderMultiColumnFlowThread always should collect the kid(s) visual overflow
> and that it is the root cause.
I don't know. I'd look into the multicol spec to see if it says something about ink overflow.

[Rob Buis]

Created attachment 463084 [details]
Patch

[Tim Nguyen (:ntim)]

I wonder if that fixes imported/w3c/web-platform-tests/css/css-contain/contain-size-monolithic-002.html [ ImageOnlyFailure ] as well

[Tim Nguyen (:ntim)]

and bug 41796 as well.

[Rob Buis]

(In reply to Tim Nguyen (:ntim) from comment #8)
> and bug 41796 as well.

It fixes the test Cathie attached in bug 41796. contain-size-monolithic-002.html is not fixed by this patch.

[Ahmad Saleem]

It seems this patch didn't landed, is it something on radar?

[Rob Buis]

Created attachment 463560 [details]
Patch

[Rob Buis]

Created attachment 463589 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 463589 [details]
Patch

Can we add passing testcases?

[Rob Buis]

Created attachment 463601 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 463604 [details]
Patch

[Rob Buis]

Created attachment 463605 [details]
Patch

[Rob Buis]

(In reply to Tim Nguyen (:ntim) from comment #13)
> Comment on attachment 463589 [details]
> Patch
> 
> Can we add passing testcases?

Ah, I completely forgot! I have added Alan's test case as well as Cathie's test from bug 41796.

[Rob Buis]

Created attachment 463624 [details]
Patch

[Rob Buis]

Created attachment 463626 [details]
Patch

[Rob Buis]

Created attachment 463629 [details]
Patch

[Darin Adler]

Comment on attachment 463629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463629&action=review

> Source/WebCore/rendering/RenderBlock.cpp:680
> +            if (RenderFragmentedFlow* containingFragmentedFlow = enclosingFragmentedFlow())

Could use “auto*” here, which would help readability. I might even call this local just “flow” to make the two lines easier to read. More explicit might not be important with something with such a narrow scope.

> Source/WebCore/rendering/RenderBox.cpp:4996
> +        childVisualOverflowRect->move(delta);

Can’t we just put “+ delta” on the end of the previous line instead?

> Source/WebCore/rendering/RenderBox.cpp:5003
> +        // Add in visual overflow from the child. Even if the child clips its overflow, it may still

This comment seems to be in the wrong place now; might need to split and reword it to be less confusing.

> LayoutTests/fast/multicol/positioned-split.html:1
> +<div style="-webkit-column-gap: 0;-webkit-column-count:2; -webkit-column-fill:auto; column-count:2; column-fill:auto; border:2px solid black; height:300px;">

Space after the semicolon?

[Tim Nguyen (:ntim)]

Comment on attachment 463629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463629&action=review

> LayoutTests/imported/w3c/web-platform-tests/css/css-multicol/multicol-relative-child-clipping.html:3
> +<link rel="help" href="href=https://drafts.csswg.org/css-multicol/">

Please fix `href="href=`

Also please address naming comments upstream too: https://github.com/web-platform-tests/wpt/pull/37027/files

(Also `multicol-` is probably redundant in the file name for both files)

[Rob Buis]

Created attachment 463661 [details]
Patch

[Rob Buis]

Comment on attachment 463629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463629&action=review

>> LayoutTests/imported/w3c/web-platform-tests/css/css-multicol/multicol-relative-child-clipping.html:3
>> +<link rel="help" href="href=https://drafts.csswg.org/css-multicol/">
> 
> Please fix `href="href=`
> 
> Also please address naming comments upstream too: https://github.com/web-platform-tests/wpt/pull/37027/files
> 
> (Also `multicol-` is probably redundant in the file name for both files)

Fixed all these. Bad luck that I copy and pasted from one of the few WPT tests that has this href mistake!

[Rob Buis]

Comment on attachment 463629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463629&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:680
>> +            if (RenderFragmentedFlow* containingFragmentedFlow = enclosingFragmentedFlow())
> 
> Could use “auto*” here, which would help readability. I might even call this local just “flow” to make the two lines easier to read. More explicit might not be important with something with such a narrow scope.

Done.

>> Source/WebCore/rendering/RenderBox.cpp:4996
>> +        childVisualOverflowRect->move(delta);
> 
> Can’t we just put “+ delta” on the end of the previous line instead?

There does not seem to be a suitable operator overload, sadly.

>> LayoutTests/fast/multicol/positioned-split.html:1
>> +<div style="-webkit-column-gap: 0;-webkit-column-count:2; -webkit-column-fill:auto; column-count:2; column-fill:auto; border:2px solid black; height:300px;">
> 
> Space after the semicolon?

Fixed.

[Rob Buis]

Created attachment 463671 [details]
Patch

[EWS]

Committed 256953@main (3fc32a416db0): <https://commits.webkit.org/256953@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463671 [details].