224185 – Interoperability issue in margin collapsing with overflow:hidden elements

RESOLVED FIXED 224185

Interoperability issue in margin collapsing with overflow:hidden elements

https://bugs.webkit.org/show_bug.cgi?id=224185

Summary Interoperability issue in margin collapsing with overflow:hidden elements

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, zalan	no flags	Details
Patch (6.09 KB, patch) 2021-08-26 09:31 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (7.15 KB, patch) 2021-08-30 11:57 PDT, Sergio Villar Senin	zalan: review+ zalan: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

zalan

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?

zalan

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

zalan

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.

zalan

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

Created attachment 425229 [details] Safari-Chrome-LFC So it looks LFC collapses these margins properly.

zalan

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?

zalan

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

zalan

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

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Sergio Villar Senin

Reported

2021-04-05 08:18 PDT

Modified

2021-09-07 08:00 PDT History

CC List

13 users Show

URL

Keywords InRadar

Depends on

229759

Blocks

Dependencies

tree graph