Bug 224185

Summary: Interoperability issue in margin collapsing with overflow:hidden elements
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: Layout and RenderingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, mmaxfield, pdr, simon.fraser, thorton, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 229759    
Bug Blocks:    
Attachments:
Description Flags
Margin collapsing issue
none
Extra case 1
none
Extra case 2
none
Extra case 3
none
Safari-Chrome-LFC
none
Patch
none
Patch zalan: review+, zalan: commit-queue-

Sergio Villar Senin
Reported 2021-04-05 08:18:46 PDT
Created attachment 425153 [details] Margin collapsing issue See the attached example. It's basically a couple of blocks separated by a special block element with overflow:hidden. A non visual overflow in theory creates a new formatting context, and thus should prevent margins between siblings from collapsing. That's how current Chrome and Firefox behave. However WebKit does not check the overflow value at all and thus goes on collapsing margins. It's easy to add a new condition to RenderBox[1] to check for overflow != visible. However that brings additional issues in the case of the body being the element with non visual overflow. Chrome added long time ago some code [2][3][4] that makes a non-visual overflow not creating a new formatting context in cases like the one mentioned above (<body> being the element with non-visual overflow). [1] https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/RenderBox.cpp#4676 [2] https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/dom/document.cc;l=3587;drc=9ba6750ad3912c31b2c411cb309f724293859c41;bpv=1;bpt=1?q=viewportdefiningelement&sq=&ss=chromium%2Fchromium%2Fsrc [3] https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/layout/layout_block.cc;drc=9ba6750ad3912c31b2c411cb309f724293859c41;bpv=1;bpt=1;l=256?q=sethasnonvisibleoverflow&ss=chromium%2Fchromium%2Fsrc [4] https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/layout/layout_object.h;l=1649;drc=9ba6750ad3912c31b2c411cb309f724293859c41;bpv=1;bpt=1?q=isscrollcontainer&sq=&ss=chromium%2Fchromium%2Fsrc
Attachments
Margin collapsing issue (332 bytes, text/html)
2021-04-05 08:18 PDT, Sergio Villar Senin 		no flags
Details
Extra case 1 (696 bytes, text/html)
2021-04-05 10:33 PDT, Sergio Villar Senin 		no flags
Details
Extra case 2 (387 bytes, text/html)
2021-04-05 10:36 PDT, Sergio Villar Senin 		no flags
Details
Extra case 3 (352 bytes, text/html)
2021-04-05 10:37 PDT, Sergio Villar Senin 		no flags
Details
Safari-Chrome-LFC (254.92 KB, image/png)
2021-04-05 18:43 PDT, alan 		no flags
Details
Patch (6.09 KB, patch)
2021-08-26 09:31 PDT, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (7.15 KB, patch)
2021-08-30 11:57 PDT, Sergio Villar Senin 		zalan: review+
zalan: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
alan
Comment 1 2021-04-05 08:24:03 PDT
"A non visual overflow in theory creates a new formatting context" what do you mean by "in theory"? (this is already fixed in LFC see Box::establishesBlockFormattingContext() and BlockFormattingContext::MarginCollapse functions)
Sergio Villar Senin
Comment 2 2021-04-05 08:28:52 PDT
(In reply to zalan from comment #1) > "A non visual overflow in theory creates a new formatting context" > what do you mean by "in theory"? > (this is already fixed in LFC see Box::establishesBlockFormattingContext() > and BlockFormattingContext::MarginCollapse functions) Well I meant that it indeed should create a new one, I was not putting anything in doubt :). Great that it's fixed in LFC. Does it still work properly when the body has a non visual overflow? I've several other test cases I created that showcase differences between WebKit and other engines, how could I test that with LFC?
alan
Comment 3 2021-04-05 08:51:32 PDT
(In reply to Sergio Villar Senin from comment #2) > (In reply to zalan from comment #1) > > "A non visual overflow in theory creates a new formatting context" > > what do you mean by "in theory"? > > (this is already fixed in LFC see Box::establishesBlockFormattingContext() > > and BlockFormattingContext::MarginCollapse functions) > > Well I meant that it indeed should create a new one, I was not putting > anything in doubt :). > > Great that it's fixed in LFC. Does it still work properly when the body has > a non visual overflow? > > I've several other test cases I created that showcase differences between > WebKit and other engines, how could I test that with LFC? It's in a bit of flux atm. :( Could you upload those tests here and I'll check them out?
Sergio Villar Senin
Comment 4 2021-04-05 10:33:51 PDT
Created attachment 425171 [details] Extra case 1 There are 3 containers. The first two look the same in WK, but not in the other engines. The 3rd case is another case in which WK collapses margins while other engines don't
Sergio Villar Senin
Comment 5 2021-04-05 10:36:02 PDT
Created attachment 425172 [details] Extra case 2 Works fine in ToT. It does not work if we modify the code so that non visible overflows create formatting contexts. The fact that the <body> is the one with the overflow:hidden is important, see "Extra case 3".
Sergio Villar Senin
Comment 6 2021-04-05 10:37:22 PDT
Created attachment 425173 [details] Extra case 3 Almost equal to "Extra case 2". The difference is that the overflow:hidden is not the <body>. This example works fine in ToT *and* also if we modify the code to include non visual overflows as creators of new formatting contexts.
Sergio Villar Senin
Comment 7 2021-04-05 10:38:18 PDT
(In reply to zalan from comment #3) > (In reply to Sergio Villar Senin from comment #2) > > (In reply to zalan from comment #1) > > > "A non visual overflow in theory creates a new formatting context" > > > what do you mean by "in theory"? > > > (this is already fixed in LFC see Box::establishesBlockFormattingContext() > > > and BlockFormattingContext::MarginCollapse functions) > > > > Well I meant that it indeed should create a new one, I was not putting > > anything in doubt :). > > > > Great that it's fixed in LFC. Does it still work properly when the body has > > a non visual overflow? > > > > I've several other test cases I created that showcase differences between > > WebKit and other engines, how could I test that with LFC? > It's in a bit of flux atm. :( Could you upload those tests here and I'll > check them out? Done. Just ping me on Slack/mail etc... if you need more info (also I might not have explained myself correctly).
alan
Comment 8 2021-04-05 10:57:53 PDT
(In reply to Sergio Villar Senin from comment #7) > (In reply to zalan from comment #3) > > (In reply to Sergio Villar Senin from comment #2) > > > (In reply to zalan from comment #1) > > > > "A non visual overflow in theory creates a new formatting context" > > > > what do you mean by "in theory"? > > > > (this is already fixed in LFC see Box::establishesBlockFormattingContext() > > > > and BlockFormattingContext::MarginCollapse functions) > > > > > > Well I meant that it indeed should create a new one, I was not putting > > > anything in doubt :). > > > > > > Great that it's fixed in LFC. Does it still work properly when the body has > > > a non visual overflow? > > > > > > I've several other test cases I created that showcase differences between > > > WebKit and other engines, how could I test that with LFC? > > It's in a bit of flux atm. :( Could you upload those tests here and I'll > > check them out? > > Done. Just ping me on Slack/mail etc... if you need more info (also I might > not have explained myself correctly). Thanks a lot! Will look into this later today and let you know.
alan
Comment 9 2021-04-05 18:43:59 PDT
Created attachment 425229 [details] Safari-Chrome-LFC So it looks LFC collapses these margins properly.
alan
Comment 10 2021-04-05 18:46:49 PDT
(In reply to zalan from comment #9) > Created attachment 425229 [details] > Safari-Chrome-LFC > > So it looks LFC collapses these margins properly. and all the other cases are working as well.
Sergio Villar Senin
Comment 11 2021-04-06 01:02:51 PDT
(In reply to zalan from comment #10) > (In reply to zalan from comment #9) > > Created attachment 425229 [details] > > Safari-Chrome-LFC > > > > So it looks LFC collapses these margins properly. > and all the other cases are working as well. Nice! BTW this is the patch I was working on: @@ -497,7 +497,8 @@ bool RenderBlock::isSelfCollapsingBlock() const if (logicalHeight() > 0 || isTable() || borderAndPaddingLogicalHeight() || style().logicalMinHeight().isPositive() - || style().marginBeforeCollapse() == MarginCollapse::Separate || style().marginAfterCollapse() == MarginCollapse::Separate) + || style().marginBeforeCollapse() == MarginCollapse::Separate || style().marginAfterCollapse() == MarginCollapse::Separate + || createsNewFormattingContext()) return false; @@ -4675,7 +4675,7 @@ bool RenderBox::shrinkToAvoidFloats() const bool RenderBox::createsNewFormattingContext() const { - return isInlineBlockOrInlineTable() || isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || isFlexItemIncludingDeprecated() + return isInlineBlockOrInlineTable() || isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || isFlexItemIncludingDeprecated() || !style().isOverflowVisible() || isTableCell() || isTableCaption() || isFieldset() || isWritingModeRoot() || isDocumentElementRenderer() || isRenderFragmentedFlow() || isRenderFragmentContainer() || isGridItem() || style().specifiesColumns() || style().columnSpan() == ColumnSpan::All || style().display() == DisplayType::FlowRoot; This fixes all the cases except the ones involving <body>. As I mentioned the problem is that style().isOverflowVisible() is not exactly what we want. We need something like https://webkit-search.igalia.com/webkit/source/Source/WebCore/layout/layouttree/LayoutBox.cpp#391 which you added in r240213. I wonder what's the plan from the LFC POV. Is it OK to share code with the good-old-layout system and LFC? Would it be better to reimplement it so it does not taint LFC?
alan
Comment 12 2021-04-06 09:03:13 PDT
(In reply to Sergio Villar Senin from comment #11) > (In reply to zalan from comment #10) > > (In reply to zalan from comment #9) > > > Created attachment 425229 [details] > > > Safari-Chrome-LFC > > > > > > So it looks LFC collapses these margins properly. > > and all the other cases are working as well. > > Nice! > > BTW this is the patch I was working on: > > @@ -497,7 +497,8 @@ bool RenderBlock::isSelfCollapsingBlock() const > if (logicalHeight() > 0 > || isTable() || borderAndPaddingLogicalHeight() > || style().logicalMinHeight().isPositive() > - || style().marginBeforeCollapse() == MarginCollapse::Separate || > style().marginAfterCollapse() == MarginCollapse::Separate) > + || style().marginBeforeCollapse() == MarginCollapse::Separate || > style().marginAfterCollapse() == MarginCollapse::Separate > + || createsNewFormattingContext()) > return false; > > @@ -4675,7 +4675,7 @@ bool RenderBox::shrinkToAvoidFloats() const > > bool RenderBox::createsNewFormattingContext() const > { > - return isInlineBlockOrInlineTable() || > isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || > isFlexItemIncludingDeprecated() > + return isInlineBlockOrInlineTable() || > isFloatingOrOutOfFlowPositioned() || hasOverflowClip() || > isFlexItemIncludingDeprecated() || !style().isOverflowVisible() > || isTableCell() || isTableCaption() || isFieldset() || > isWritingModeRoot() || isDocumentElementRenderer() || > isRenderFragmentedFlow() || isRenderFragmentContainer() > || isGridItem() || style().specifiesColumns() || > style().columnSpan() == ColumnSpan::All || style().display() == > DisplayType::FlowRoot; > > This fixes all the cases except the ones involving <body>. As I mentioned > the problem is that style().isOverflowVisible() is not exactly what we want. > We need something like > https://webkit-search.igalia.com/webkit/source/Source/WebCore/layout/ > layouttree/LayoutBox.cpp#391 which you added in r240213. > > I wonder what's the plan from the LFC POV. Is it OK to share code with the > good-old-layout system and LFC? Would it be better to reimplement it so it > does not taint LFC? I don't think it's worth templatizing it (or share this logic through some other means) I'd just reimplement it in RenderElement (or wherever it is applicable).
Radar WebKit Bug Importer
Comment 13 2021-04-12 08:19:14 PDT
<rdar://problem/76539100>
Sergio Villar Senin
Comment 14 2021-08-26 09:31:01 PDT
Created attachment 436524 [details] Patch
Sergio Villar Senin
Comment 15 2021-08-30 11:57:45 PDT
Created attachment 436800 [details] Patch
alan
Comment 16 2021-08-31 16:30:22 PDT
Comment on attachment 436800 [details] Patch Please do not combine 2 different fixes in one patch. It makes regression tracking more involved.
Sergio Villar Senin
Comment 17 2021-09-01 04:55:06 PDT
(In reply to zalan from comment #16) > Comment on attachment 436800 [details] > Patch > > Please do not combine 2 different fixes in one patch. It makes regression > tracking more involved. I've created https://bugs.webkit.org/show_bug.cgi?id=229759 for the extra isRenderFragmentContainer() removal.
Sergio Villar Senin
Comment 18 2021-09-07 07:58:49 PDT
(In reply to zalan from comment #16) > Comment on attachment 436800 [details] > Patch > > Please do not combine 2 different fixes in one patch. It makes regression > tracking more involved. Now that bug 229759 landed I'm landing this one.
Sergio Villar Senin
Comment 19 2021-09-07 08:00:32 PDT
Committed r282085 (241385@main): <https://commits.webkit.org/241385@main>
Note You need to log in before you can comment on or make changes to this bug.