Bug 1438467: Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements. r=mats,bz draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 15 Feb 2018 01:41:16 +0100
changeset 759134 dded6b3d6e6f4b9f4e6cb3c2a5efb2af359af8c3
parent 759133 9c5161fa5624c5831dacc5787bd91ad0e56687d5
child 759135 7c1f7abbd65acc417d9384f7db1d716877eae731
push id100275
push userbmo:emilio@crisal.io
push dateFri, 23 Feb 2018 18:33:59 +0000
reviewersmats, bz
bugs1438467
milestone60.0a1
Bug 1438467: Don't reconstruct the parent when tearing down display: contents nodes with pseudo-elements. r=mats,bz We just need to use the existing StyleChildrenIterator which iterates over them. We need to be a bit careful though, since ::before and ::after are owned by their own frame, and thus could be unbound from the tree or even dead after removing the frame. Hopefully the only access to the node being removed is unnecessary (anon roots don't have siblings anyway). There's also the weird thing of the thing we're iterating changing under the hood. It works fine for this case, but maybe it would be better to handle them explicitly like: if (Element* before = nsLayoutUtils::GetBeforePseudo(aChild)) { bool didReconstruct = ContentRemoved(aChild, ...); if (didReconstruct) { return true; } MOZ_ASSERT(!nsLayoutUtils::GetBeforePseudo(aChild)); } // Same for ::after. StyleChildrenIterator iter(aChild); for (..) { // Do the rest of the kids, which can't get unbound. } That'd repeat a bunch of code, so not a fan neither... I pointed this out more explicitly in a comment instead. MozReview-Commit-ID: HBsjLH01Db3
layout/base/nsCSSFrameConstructor.cpp
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -1718,27 +1718,16 @@ nsCSSFrameConstructor::NotifyDestroyingF
     CountersDirty();
   }
 
   RestyleManager()->NotifyDestroyingFrame(aFrame);
 
   nsFrameManager::NotifyDestroyingFrame(aFrame);
 }
 
-static bool
-HasGeneratedContent(const nsIContent* aChild)
-{
-  if (!aChild->MayHaveAnonymousChildren()) {
-    return false;
-  }
-
-  return nsLayoutUtils::GetBeforeFrame(aChild) ||
-         nsLayoutUtils::GetAfterFrame(aChild);
-}
-
 struct nsGenConInitializer {
   nsAutoPtr<nsGenConNode> mNode;
   nsGenConList*           mList;
   void (nsCSSFrameConstructor::*mDirtyAll)();
 
   nsGenConInitializer(nsGenConNode* aNode, nsGenConList* aList,
                       void (nsCSSFrameConstructor::*aDirtyAll)())
     : mNode(aNode), mList(aList), mDirtyAll(aDirtyAll) {}
@@ -8489,16 +8478,18 @@ nsCSSFrameConstructor::ContentRangeInser
 
 bool
 nsCSSFrameConstructor::ContentRemoved(nsIContent* aContainer,
                                       nsIContent* aChild,
                                       nsIContent* aOldNextSibling,
                                       RemoveFlags aFlags)
 {
   MOZ_ASSERT(aChild);
+  MOZ_ASSERT(!aChild->IsRootOfAnonymousSubtree() || !aOldNextSibling,
+             "Anonymous roots don't have siblings");
   AUTO_LAYOUT_PHASE_ENTRY_POINT(mPresShell->GetPresContext(), FrameC);
   nsPresContext* presContext = mPresShell->GetPresContext();
   MOZ_ASSERT(presContext, "Our presShell should have a valid presContext");
 
   // We want to detect when the viewport override element stored in the
   // prescontext is in the subtree being removed.  Except in fullscreen cases
   // (which are handled in Element::UnbindFromTree and do not get stored on the
   // prescontext), the override element is always either the root element or a
@@ -8559,32 +8550,19 @@ nsCSSFrameConstructor::ContentRemoved(ns
     // XXXbz the GetContent() != aChild check is needed due to bug 135040.
     // Remove it once that's fixed.
     childFrame = nullptr;
     UnregisterDisplayNoneStyleFor(aChild, aContainer);
   }
   MOZ_ASSERT(!childFrame || !GetDisplayContentsStyleFor(aChild),
              "display:contents nodes shouldn't have a frame");
   if (!childFrame && GetDisplayContentsStyleFor(aChild)) {
-    if (HasGeneratedContent(aChild)) {
-      nsIContent* ancestor = aChild->GetFlattenedTreeParent();
-      MOZ_ASSERT(ancestor, "display: contents on the root?");
-      while (!ancestor->GetPrimaryFrame()) {
-        ancestor = ancestor->GetFlattenedTreeParent();
-        MOZ_ASSERT(ancestor, "we can't have a display: contents subtree root!");
-      }
-
-      // XXXmats Can we recreate frames only for the ::after/::before content?
-      // XXX Perhaps even only those that belong to the aChild sub-tree?
-      LAYOUT_PHASE_TEMP_EXIT();
-      RecreateFramesForContent(ancestor, InsertionKind::Async);
-      LAYOUT_PHASE_TEMP_REENTER();
-      return true;
-    }
-
+    // NOTE(emilio): We may iterate through ::before and ::after here and they
+    // may be gone after the respective ContentRemoved call. Right now
+    // StyleChildrenIterator handles that properly, so it's not an issue.
     StyleChildrenIterator iter(aChild);
     for (nsIContent* c = iter.GetNextChild(); c; c = iter.GetNextChild()) {
       if (c->GetPrimaryFrame() || GetDisplayContentsStyleFor(c)) {
         LAYOUT_PHASE_TEMP_EXIT();
         bool didReconstruct =
           ContentRemoved(aChild, c, nullptr, REMOVE_FOR_RECONSTRUCTION);
         LAYOUT_PHASE_TEMP_REENTER();
         if (didReconstruct) {
@@ -8737,25 +8715,29 @@ nsCSSFrameConstructor::ContentRemoved(ns
     if (gReallyNoisyContentUpdates) {
       printf("nsCSSFrameConstructor::ContentRemoved: childFrame=");
       nsFrame::ListTag(stdout, childFrame);
       putchar('\n');
       parentFrame->List(stdout, 0);
     }
 #endif
 
-
     // Notify the parent frame that it should delete the frame
     if (childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW) {
       childFrame = childFrame->GetPlaceholderFrame();
       NS_ASSERTION(childFrame, "Missing placeholder frame for out of flow.");
       parentFrame = childFrame->GetParent();
     }
+
     RemoveFrame(nsLayoutUtils::GetChildListNameFor(childFrame), childFrame);
 
+    // NOTE(emilio): aChild could be dead here already if it is a ::before or
+    // ::after pseudo-element (since in that case it was owned by childFrame,
+    // which we just destroyed).
+
     if (isRoot) {
       mRootElementFrame = nullptr;
       mRootElementStyleFrame = nullptr;
       mDocElementContainingBlock = nullptr;
       mPageSequenceFrame = nullptr;
       mHasRootAbsPosContainingBlock = false;
     }
 
@@ -8764,43 +8746,40 @@ nsCSSFrameConstructor::ContentRemoved(ns
     }
 
     // If we're just reconstructing frames for the element, then the
     // following ContentInserted notification on the element will
     // take care of fixing up any adjacent text nodes.  We don't need
     // to do this if the table parent type of our parent type is not
     // eTypeBlock, though, because in that case the whitespace isn't
     // being suppressed due to us anyway.
-    if (aContainer && !aChild->IsRootOfAnonymousSubtree() &&
-        aFlags == REMOVE_CONTENT &&
+    if (aContainer && aOldNextSibling && aFlags == REMOVE_CONTENT &&
         GetParentType(parentType) == eTypeBlock) {
       // Adjacent whitespace-only text nodes might have been suppressed if
       // this node does not have inline ends. Create frames for them now
       // if necessary.
       // Reframe any text node just before the node being removed, if there is
       // one, and if it's not the last child or the first child. If a whitespace
       // textframe was being suppressed and it's now the last child or first
       // child then it can stay suppressed since the parent must be a block
       // and hence it's adjacent to a block end.
       // If aOldNextSibling is null, then the text node before the node being
       // removed is the last node, and we don't need to worry about it.
       //
       // FIXME(emilio): This should probably use the lazy frame construction
       // bits if possible instead of reframing it in place.
-      if (aOldNextSibling) {
-        nsIContent* prevSibling = aOldNextSibling->GetPreviousSibling();
-        if (prevSibling && prevSibling->GetPreviousSibling()) {
-          LAYOUT_PHASE_TEMP_EXIT();
-          ReframeTextIfNeeded(aContainer, prevSibling);
-          LAYOUT_PHASE_TEMP_REENTER();
-        }
+      nsIContent* prevSibling = aOldNextSibling->GetPreviousSibling();
+      if (prevSibling && prevSibling->GetPreviousSibling()) {
+        LAYOUT_PHASE_TEMP_EXIT();
+        ReframeTextIfNeeded(aContainer, prevSibling);
+        LAYOUT_PHASE_TEMP_REENTER();
       }
       // Reframe any text node just after the node being removed, if there is
       // one, and if it's not the last child or the first child.
-      if (aOldNextSibling && aOldNextSibling->GetNextSibling() &&
+      if (aOldNextSibling->GetNextSibling() &&
           aOldNextSibling->GetPreviousSibling()) {
         LAYOUT_PHASE_TEMP_EXIT();
         ReframeTextIfNeeded(aContainer, aOldNextSibling);
         LAYOUT_PHASE_TEMP_REENTER();
       }
     }
 
 #ifdef DEBUG