Bug 238854

<rdar://problem/91438700>

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.

Created attachment 462435 [details]
Patch

@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.

(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.

Created attachment 463084 [details]
Patch

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

and bug 41796 as well.

(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.

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

Created attachment 463560 [details]
Patch

Created attachment 463589 [details]
Patch

Comment on attachment 463589 [details]
Patch

Can we add passing testcases?

Created attachment 463601 [details]
Patch

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

Created attachment 463604 [details]
Patch

Created attachment 463605 [details]
Patch

(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.

Created attachment 463624 [details]
Patch

Created attachment 463626 [details]
Patch

Created attachment 463629 [details]
Patch

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?

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)

Created attachment 463661 [details]
Patch

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!

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.

Created attachment 463671 [details]
Patch

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].