From 6f02b18148a388aa1deb2f8b7458029384361f0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 10 Nov 2021 15:24:30 +0000 Subject: [PATCH] Bug 1739894 - Stop including overflowing descendants in outlines. r=jwatt Keep old behavior behind a pref. I've kept the tests that failed enabling the pref because they're somewhat hard to rewrite and I'd rather avoid that until we know this will stick, specially since they test the guts of the recursive function that I'm basically ~disabling. I've checked that the rendering on those is sensible / matches other browsers. Differential Revision: https://phabricator.services.mozilla.com/D130717 --- layout/generic/nsIFrame.cpp | 48 +++++++++++-------- layout/reftests/bugs/reftest.list | 24 +++++----- layout/reftests/columns/reftest.list | 2 +- layout/reftests/outline/reftest.list | 4 +- modules/libpref/init/StaticPrefList.yaml | 9 ++++ .../meta/css/css-ui/outline-027.html.ini | 2 - .../meta/css/css-ui/outline-028.html.ini | 2 - 7 files changed, 52 insertions(+), 39 deletions(-) delete mode 100644 testing/web-platform/meta/css/css-ui/outline-027.html.ini delete mode 100644 testing/web-platform/meta/css/css-ui/outline-028.html.ini diff --git a/layout/generic/nsIFrame.cpp b/layout/generic/nsIFrame.cpp index 0b1130a536e8..6f8da60543c0 100644 --- a/layout/generic/nsIFrame.cpp +++ b/layout/generic/nsIFrame.cpp @@ -9338,13 +9338,15 @@ bool nsIFrame::SetOverflowAreas(const OverflowAreas& aOverflowAreas) { } } +enum class ApplyTransform : bool { No, Yes }; + /** - * Compute the union of the border boxes of aFrame and its descendants, - * in aFrame's coordinate space (if aApplyTransform is false) or its - * post-transform coordinate space (if aApplyTransform is true). + * Compute the outline inner rect (so without outline-width and outline-offset) + * of aFrame, maybe iterating over its descendants, in aFrame's coordinate space + * or its post-transform coordinate space (depending on aApplyTransform). */ -static nsRect UnionBorderBoxes( - nsIFrame* aFrame, bool aApplyTransform, bool& aOutValid, +static nsRect ComputeOutlineInnerRect( + nsIFrame* aFrame, ApplyTransform aApplyTransform, bool& aOutValid, const nsSize* aSizeOverride = nullptr, const OverflowAreas* aOverflowOverride = nullptr) { const nsRect bounds(nsPoint(0, 0), @@ -9367,7 +9369,8 @@ static nsRect UnionBorderBoxes( // Start from our border-box, transformed. See comment below about // transform of children. - bool doTransform = aApplyTransform && aFrame->IsTransformed(); + bool doTransform = + aApplyTransform == ApplyTransform::Yes && aFrame->IsTransformed(); TransformReferenceBox boundsRefBox(nullptr, bounds); if (doTransform) { u = nsDisplayTransform::TransformRect(bounds, aFrame, boundsRefBox); @@ -9375,6 +9378,10 @@ static nsRect UnionBorderBoxes( u = bounds; } + if (aOutValid && !StaticPrefs::layout_outline_include_overflow()) { + return u; + } + // Only iterate through the children if the overflow areas suggest // that we might need to, and if the frame doesn't clip its overflow // anyway. @@ -9419,18 +9426,18 @@ static nsRect UnionBorderBoxes( continue; } - // Note that passing |true| for aApplyTransform when - // child->Combines3DTransformWithAncestors() is incorrect if our - // aApplyTransform is false... but the opposite would be as - // well. This is because elements within a preserve-3d scene - // are always transformed up to the top of the scene. This - // means we don't have a mechanism for getting a transform up to - // an intermediate point within the scene. We choose to - // over-transform rather than under-transform because this is - // consistent with other overflow areas. + // Note that passing ApplyTransform::Yes when + // child->Combines3DTransformWithAncestors() returns true is incorrect if + // our aApplyTransform is No... but the opposite would be as well. + // This is because elements within a preserve-3d scene are always + // transformed up to the top of the scene. This means we don't have a + // mechanism for getting a transform up to an intermediate point within + // the scene. We choose to over-transform rather than under-transform + // because this is consistent with other overflow areas. bool validRect = true; nsRect childRect = - UnionBorderBoxes(child, true, validRect) + child->GetPosition(); + ComputeOutlineInnerRect(child, ApplyTransform::Yes, validRect) + + child->GetPosition(); if (!validRect) { continue; @@ -9505,13 +9512,14 @@ static void ComputeAndIncludeOutlineArea(nsIFrame* aFrame, // calling FinishAndStoreOverflow again, which in turn calls this // function again. We still need to deal with preserve-3d a bit. nsRect innerRect; - bool validRect; + bool validRect = false; if (frameForArea == aFrame) { - innerRect = - UnionBorderBoxes(aFrame, false, validRect, &aNewSize, &aOverflowAreas); + innerRect = ComputeOutlineInnerRect(aFrame, ApplyTransform::No, validRect, + &aNewSize, &aOverflowAreas); } else { for (; frameForArea; frameForArea = frameForArea->GetNextSibling()) { - nsRect r(UnionBorderBoxes(frameForArea, true, validRect)); + nsRect r = + ComputeOutlineInnerRect(aFrame, ApplyTransform::Yes, validRect); // Adjust for offsets transforms up to aFrame's pre-transform // (i.e., normal) coordinate space; see comments in diff --git a/layout/reftests/bugs/reftest.list b/layout/reftests/bugs/reftest.list index 821e198dc5de..7b11d0b5c43c 100644 --- a/layout/reftests/bugs/reftest.list +++ b/layout/reftests/bugs/reftest.list @@ -837,7 +837,7 @@ fails-if(cocoaWidget||winWidget) random-if(!cocoaWidget&&!winWidget) != 399636-q == 400813-1.html 400813-1-ref.html == 400826-1.html 400826-1-ref.html == chrome://reftest/content/bugs/401946-1.xhtml about:blank -== 402338-1.html 402338-1-ref.html +pref(layout.outline.include-overflow,true) == 402338-1.html 402338-1-ref.html == 402567-1.html 402567-1-ref.html fuzzy-if(winWidget&&!nativeThemePref,0-8,0-1) == 402567-2.html 402567-2-ref.html == 402567-3.html 402567-3-ref.html @@ -1006,17 +1006,17 @@ fails == 423823-1.html 423823-1-ref.html # scrolling rowgroups were removed in b == chrome://reftest/content/bugs/424074-1.xhtml chrome://reftest/content/bugs/424074-1-ref.xhtml fails-if(Android) != chrome://reftest/content/bugs/424074-1.xhtml chrome://reftest/content/bugs/424074-1-ref2.xhtml random-if(gtkWidget) == chrome://reftest/content/bugs/424074-1-ref2.xhtml chrome://reftest/content/bugs/424074-1-ref3.xhtml -== 424236-1.html 424236-1-ref.html -== 424236-2.html 424236-2-ref.html -== 424236-3.html 424236-3-ref.html -== 424236-4.html 424236-4-ref.html -== 424236-5.html 424236-5-ref.html -== 424236-6.html 424236-6-ref.html -== 424236-7.html 424236-7-ref.html -== 424236-8.html 424236-8-ref.html -== 424236-9.html 424236-9-ref.html -== 424236-10.html 424236-10-ref.html -== 424236-11.html 424236-3-ref.html +pref(layout.outline.include-overflow,true) == 424236-1.html 424236-1-ref.html +pref(layout.outline.include-overflow,true) == 424236-2.html 424236-2-ref.html +pref(layout.outline.include-overflow,true) == 424236-3.html 424236-3-ref.html +pref(layout.outline.include-overflow,true) == 424236-4.html 424236-4-ref.html +pref(layout.outline.include-overflow,true) == 424236-5.html 424236-5-ref.html +pref(layout.outline.include-overflow,true) == 424236-6.html 424236-6-ref.html +pref(layout.outline.include-overflow,true) == 424236-7.html 424236-7-ref.html +pref(layout.outline.include-overflow,true) == 424236-8.html 424236-8-ref.html +pref(layout.outline.include-overflow,true) == 424236-9.html 424236-9-ref.html +pref(layout.outline.include-overflow,true) == 424236-10.html 424236-10-ref.html +pref(layout.outline.include-overflow,true) == 424236-11.html 424236-3-ref.html == 424434-1.html 424434-1-ref.html == 424465-1.html 424465-1-ref.html == 424631-1.html 424631-1-ref.html diff --git a/layout/reftests/columns/reftest.list b/layout/reftests/columns/reftest.list index 160124167c43..916817f49ff7 100644 --- a/layout/reftests/columns/reftest.list +++ b/layout/reftests/columns/reftest.list @@ -11,7 +11,7 @@ == column-balancing-overflow-002.html column-balancing-overflow-002.ref.html == column-balancing-overflow-003.html column-balancing-overflow-003.ref.html == column-balancing-overflow-004.html column-balancing-overflow-004.ref.html -== column-balancing-overflow-005.html column-balancing-overflow-005.ref.html +pref(layout.outline.include-overflow,true) == column-balancing-overflow-005.html column-balancing-overflow-005.ref.html == column-balancing-000.html column-balancing-000.ref.html == column-balancing-001.html column-balancing-000.ref.html == column-balancing-002.html column-balancing-002.ref.html diff --git a/layout/reftests/outline/reftest.list b/layout/reftests/outline/reftest.list index 28e14f310c99..db3083ea6c62 100644 --- a/layout/reftests/outline/reftest.list +++ b/layout/reftests/outline/reftest.list @@ -1,6 +1,6 @@ fuzzy(0-52,0-1452) == outline-and-box-shadow.html outline-and-box-shadow-ref.html -== outline-and-3d-transform-1a.html outline-and-3d-transform-1-ref.html -== outline-and-3d-transform-1b.html outline-and-3d-transform-1-ref.html +pref(layout.outline.include-overflow,true) == outline-and-3d-transform-1a.html outline-and-3d-transform-1-ref.html +pref(layout.outline.include-overflow,true) == outline-and-3d-transform-1b.html outline-and-3d-transform-1-ref.html fuzzy-if(gtkWidget,0-136,0-120) fuzzy-if(Android,0-255,0-356) fuzzy-if(d2d,0-16,0-96) fuzzy-if(cocoaWidget,0-255,0-120) fuzzy-if(winWidget,0-255,0-216) == outline-and-3d-transform-2.html outline-and-3d-transform-2-ref.html == outline-dynamic-change-1a.html outline-dynamic-change-1-ref.html == outline-dynamic-change-1b.html outline-dynamic-change-1-ref.html diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index e7ac542e94d9..78435b71a1d0 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -7772,6 +7772,15 @@ value: 0 mirror: always +# Whether outlines should include all overflowing descendants, or just the +# border-box of a given element. +# +# Historically we have included descendants but other browsers have not. +- name: layout.outline.include-overflow + type: bool + value: false + mirror: always + # Controls double click and Alt+Arrow word selection behavior. - name: layout.word_select.eat_space_to_next_word type: bool diff --git a/testing/web-platform/meta/css/css-ui/outline-027.html.ini b/testing/web-platform/meta/css/css-ui/outline-027.html.ini deleted file mode 100644 index 2b8d01942e1f..000000000000 --- a/testing/web-platform/meta/css/css-ui/outline-027.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[outline-027.html] - expected: FAIL diff --git a/testing/web-platform/meta/css/css-ui/outline-028.html.ini b/testing/web-platform/meta/css/css-ui/outline-028.html.ini deleted file mode 100644 index f203a9838264..000000000000 --- a/testing/web-platform/meta/css/css-ui/outline-028.html.ini +++ /dev/null @@ -1,2 +0,0 @@ -[outline-028.html] - expected: FAIL