Bug 224185

Summary:

Interoperability issue in margin collapsing with overflow:hidden elements

Product:

WebKit

Reporter:

Sergio Villar Senin <svillar>

Component:

Layout and Rendering

Assignee:

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:

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-

Description Sergio Villar Senin 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

Comment 1 zalan 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)

Comment 2 Sergio Villar Senin 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?

Comment 3 zalan 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?

Comment 4 Sergio Villar Senin 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

Comment 5 Sergio Villar Senin 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".

Comment 6 Sergio Villar Senin 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.

Comment 7 Sergio Villar Senin 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).

Comment 8 zalan 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.

Comment 9 zalan 2021-04-05 18:43:59 PDT

Created attachment 425229 [details]
Safari-Chrome-LFC

So it looks LFC collapses these margins properly.

Comment 10 zalan 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.

Comment 11 Sergio Villar Senin 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?

Comment 12 zalan 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).

Comment 13 Radar WebKit Bug Importer 2021-04-12 08:19:14 PDT

<rdar://problem/76539100>

Comment 14 Sergio Villar Senin 2021-08-26 09:31:01 PDT

Created attachment 436524 [details]
Patch

Comment 15 Sergio Villar Senin 2021-08-30 11:57:45 PDT

Created attachment 436800 [details]
Patch

Comment 16 zalan 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.

Comment 17 Sergio Villar Senin 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.

Comment 18 Sergio Villar Senin 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.

Comment 19 Sergio Villar Senin 2021-09-07 08:00:32 PDT

Committed r282085 (241385@main): <https://commits.webkit.org/241385@main>