From 353832c272cddcfaec3d1c065247a626afb6ed24 Mon Sep 17 00:00:00 2001 From: Marco Castelluccio Date: Sun, 29 Sep 2019 16:33:38 +0000 Subject: [PATCH] Bug 1110277 patch 3 - Look for the GenConPseudos() property on the first continuation/ib-split so that we can find it when looking for the ::after frame. r=bzbarsky The change to GetAfterFrameForContent prevents the reframe that is part of the chain of events leading to this bug, and thus fixes the bug on its own. The change to GetBeforeFrameForContent seems desirable for symmetry. Note that patch 6 also independently fixes the reported bug. This probably needs somewhat careful review. We should examine: (1) what the rules for calling nsLayoutUtils::GetBeforeFrame and nsLayoutUtils::GetAfterFrame are, and whether both (or neither) need to be patched. (2) What the rules are for which frame the GenConProperty() lives on, and whether we should adjust nsIFrame::GetGenConPseudos() to either do something more intelligent, or assert about callers. (We should probably clean up some of these things in a followup bug.) Since the symptom of this bug is (once patch 4 is in the tree) only causing extra reframes, it can only be tested using the new API (from bug 1115691) for observing reframes. I confirmed that the test for this bug fails without the patch and passes with the patch (as noted by the removal of its todo annotation). This patch fixes the assertion on layout/generic/crashtests/600100.xhtml, though I haven't investigated why. UltraBlame original commit: 1bbebe9fec17e88234845d8da22c8b44f394121b --- layout/base/nsCSSFrameConstructor.cpp | 3 +++ layout/base/nsLayoutUtils.cpp | 17 ++++++++++++++--- ...rame_reconstruction_for_pseudo_elements.html | 8 ++++---- layout/generic/crashtests/crashtests.list | 2 +- layout/generic/nsIFrame.h | 2 ++ 5 files changed, 24 insertions(+), 8 deletions(-) diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp index a9128a7fb0d7a..7197701522566 100644 --- a/layout/base/nsCSSFrameConstructor.cpp +++ b/layout/base/nsCSSFrameConstructor.cpp @@ -5789,6 +5789,9 @@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState static void AddGenConPseudoToFrame(nsIFrame* aOwnerFrame, nsIContent* aContent) { + NS_ASSERTION(nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(aOwnerFrame), + "property should only be set on first continuation/ib-sibling"); + typedef nsAutoTArray T; const FramePropertyDescriptor* prop = nsIFrame::GenConProperty(); FrameProperties props = aOwnerFrame->Properties(); diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp index 7443e5c8bc72d..cd7328ff13a39 100644 --- a/layout/base/nsLayoutUtils.cpp +++ b/layout/base/nsLayoutUtils.cpp @@ -1171,7 +1171,10 @@ nsLayoutUtils::GetChildListNameFor(nsIFrame* aChildFrame) nsLayoutUtils::GetBeforeFrameForContent(nsIFrame* aFrame, nsIContent* aContent) { - nsContainerFrame* genConParentFrame = aFrame->GetContentInsertionFrame(); + + + nsContainerFrame* genConParentFrame = + FirstContinuationOrIBSplitSibling(aFrame)->GetContentInsertionFrame(); if (!genConParentFrame) { return nullptr; } @@ -1207,7 +1210,10 @@ nsLayoutUtils::GetBeforeFrame(nsIFrame* aFrame) nsLayoutUtils::GetAfterFrameForContent(nsIFrame* aFrame, nsIContent* aContent) { - nsContainerFrame* genConParentFrame = aFrame->GetContentInsertionFrame(); + + + nsContainerFrame* genConParentFrame = + FirstContinuationOrIBSplitSibling(aFrame)->GetContentInsertionFrame(); if (!genConParentFrame) { return nullptr; } @@ -1224,8 +1230,13 @@ nsLayoutUtils::GetAfterFrameForContent(nsIFrame* aFrame, + genConParentFrame = aFrame->GetContentInsertionFrame(); + if (!genConParentFrame) { + return nullptr; + } nsIFrame* lastParentContinuation = - nsLayoutUtils::LastContinuationWithChild(genConParentFrame); + LastContinuationWithChild(static_cast( + LastContinuationOrIBSplitSibling(genConParentFrame))); nsIFrame* childFrame = lastParentContinuation->GetLastChild(nsIFrame::kPrincipalList); if (childFrame && diff --git a/layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html b/layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html index ca2d44bba66aa..5e6901478a082 100644 --- a/layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html +++ b/layout/base/tests/test_frame_reconstruction_for_pseudo_elements.html @@ -35,12 +35,12 @@ SimpleTest.waitForExplicitFinish(); function run() { - runtest("first line test", "#firstlinetest > .testspan", {}); - runtest("after test", "#aftertest > .testspan", { todo: true }); + runtest("first line test", "#firstlinetest > .testspan"); + runtest("after test", "#aftertest > .testspan"); SimpleTest.finish(); } - function runtest(description, selector, flags) { + function runtest(description, selector) { var utils = SpecialPowers.getDOMWindowUtils(window); var span = document.querySelector(selector); var cs = getComputedStyle(span, ""); @@ -54,7 +54,7 @@ var endcolor = cs.color; var endcount = utils.framesConstructed; is(endcolor, "rgb(0, 0, 255)", description + ": final color"); - (flags.todo ? todo_is : is)(endcount, startcount, + is(endcount, startcount, description + ": should not do frame construction") } diff --git a/layout/generic/crashtests/crashtests.list b/layout/generic/crashtests/crashtests.list index 7e5917c247238..ed722e9354e5f 100644 --- a/layout/generic/crashtests/crashtests.list +++ b/layout/generic/crashtests/crashtests.list @@ -425,7 +425,7 @@ asserts(0-1) load 592118.html load 594808-1.html load 595435-1.xhtml load 595740-1.html -pref(layout.float-fragments-inside-column.enabled,true) asserts(1) load 600100.xhtml # bug 866955 +pref(layout.float-fragments-inside-column.enabled,true) load 600100.xhtml pref(layout.float-fragments-inside-column.enabled,false) load 600100.xhtml load 603490-1.html load 603510-1.html diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h index 0fcc7454e784a..6bb5acd1dac00 100644 --- a/layout/generic/nsIFrame.h +++ b/layout/generic/nsIFrame.h @@ -871,6 +871,8 @@ class nsIFrame : public nsQueryFrame NS_DECLARE_FRAME_PROPERTY(GenConProperty, DestroyContentArray) nsTArray* GetGenConPseudos() { + NS_ASSERTION(nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(this), + "should only call on first continuation/ib-sibling"); const FramePropertyDescriptor* prop = GenConProperty(); return static_cast*>(Properties().Get(prop)); }