Bug 1413619: Fix insertion point computation when display: contents pseudos are involved. r=mats draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 01 Nov 2017 18:56:13 +0100
changeset 693104 cf9670bd5b18b1787a5a455a5a66717e3e92218e
parent 693103 188c2c383aa7be4eb51f76a03f7d7d054629f4b2
child 738961 f27c9a69086757e75d0b09f7e6bda18ff36c8901
push id87718
push userbmo:emilio@crisal.io
push dateSat, 04 Nov 2017 10:56:41 +0000
reviewersmats
bugs1413619
milestone58.0a1
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved. r=mats This is a significant rework of how do we compute the insertion point of a node. We handle pseudos in the same function instead of out of band, and also recurse up when the parent has display: contents, which simplifies the code IMO. MozReview-Commit-ID: 1rSfv1Tq5gO
dom/base/ChildIterator.cpp
dom/base/ChildIterator.h
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
layout/base/nsLayoutUtils.cpp
layout/base/nsLayoutUtils.h
layout/reftests/css-grid/reftest.list
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/css-display-3/display-contents-dynamic-pseudo-insertion-001-ref.html
testing/web-platform/tests/css/css-display-3/display-contents-dynamic-pseudo-insertion-001.html
--- a/dom/base/ChildIterator.cpp
+++ b/dom/base/ChildIterator.cpp
@@ -157,27 +157,27 @@ FlattenedChildIterator::Init(bool aIgnor
         mXBLInvolved = true;
         break;
       }
     }
   }
 }
 
 bool
-ExplicitChildIterator::Seek(nsIContent* aChildToFind)
+ExplicitChildIterator::Seek(const nsIContent* aChildToFind)
 {
   if (aChildToFind->GetParent() == mParent &&
       !aChildToFind->IsRootOfAnonymousSubtree()) {
     // Fast path: just point ourselves to aChildToFind, which is a
     // normal DOM child of ours.
-    MOZ_ASSERT(!nsContentUtils::IsContentInsertionPoint(aChildToFind));
-    mChild = aChildToFind;
+    mChild = const_cast<nsIContent*>(aChildToFind);
     mIndexInInserted = 0;
     mDefaultChild = nullptr;
     mIsFirst = false;
+    MOZ_ASSERT(!nsContentUtils::IsContentInsertionPoint(mChild));
     return true;
   }
 
   // Can we add more fast paths here based on whether the parent of aChildToFind
   // is a shadow insertion point or content insertion point?
 
   // Slow path: just walk all our kids.
   return Seek(aChildToFind, nullptr);
@@ -281,17 +281,17 @@ AllChildrenIterator::Get() const
 
     default:
       return nullptr;
   }
 }
 
 
 bool
-AllChildrenIterator::Seek(nsIContent* aChildToFind)
+AllChildrenIterator::Seek(const nsIContent* aChildToFind)
 {
   if (mPhase == eAtBegin || mPhase == eAtBeforeKid) {
     mPhase = eAtExplicitKids;
     Element* beforePseudo = nsLayoutUtils::GetBeforePseudo(mOriginalContent);
     if (beforePseudo && beforePseudo == aChildToFind) {
       mPhase = eAtBeforeKid;
       return true;
     }
--- a/dom/base/ChildIterator.h
+++ b/dom/base/ChildIterator.h
@@ -58,23 +58,23 @@ public:
       mIndexInInserted(aOther.mIndexInInserted) {}
 
   nsIContent* GetNextChild();
 
   // Looks for aChildToFind respecting insertion points until aChildToFind is
   // found.  This version can take shortcuts that the two-argument version
   // can't, so can be faster (and in fact can be O(1) instead of O(N) in many
   // cases).
-  bool Seek(nsIContent* aChildToFind);
+  bool Seek(const nsIContent* aChildToFind);
 
   // Looks for aChildToFind respecting insertion points until aChildToFind is found.
   // or aBound is found. If aBound is nullptr then the seek is unbounded. Returns
   // whether aChildToFind was found as an explicit child prior to encountering
   // aBound.
-  bool Seek(nsIContent* aChildToFind, nsIContent* aBound)
+  bool Seek(const nsIContent* aChildToFind, nsIContent* aBound)
   {
     // It would be nice to assert that we find aChildToFind, but bz thinks that
     // we might not find aChildToFind when called from ContentInserted
     // if first-letter frames are about.
 
     // We can't easily take shortcuts here because we'd have to have a way to
     // compare aChildToFind to aBound.
     nsIContent* child;
@@ -127,47 +127,61 @@ protected:
 // children, those are iterated over.  The iterator can be initialized to start
 // at the end by providing false for aStartAtBeginning in order to start
 // iterating in reverse from the last child.
 class FlattenedChildIterator : public ExplicitChildIterator
 {
 public:
   explicit FlattenedChildIterator(const nsIContent* aParent,
                                   bool aStartAtBeginning = true)
-    : ExplicitChildIterator(aParent, aStartAtBeginning), mXBLInvolved(false)
+    : ExplicitChildIterator(aParent, aStartAtBeginning)
+    , mXBLInvolved(false)
+    , mOriginalContent(aParent)
   {
     Init(false);
   }
 
   FlattenedChildIterator(FlattenedChildIterator&& aOther)
-    : ExplicitChildIterator(Move(aOther)), mXBLInvolved(aOther.mXBLInvolved) {}
+    : ExplicitChildIterator(Move(aOther))
+    , mXBLInvolved(aOther.mXBLInvolved)
+    , mOriginalContent(aOther.mOriginalContent)
+  {}
 
   FlattenedChildIterator(const FlattenedChildIterator& aOther)
-    : ExplicitChildIterator(aOther), mXBLInvolved(aOther.mXBLInvolved) {}
+    : ExplicitChildIterator(aOther)
+    , mXBLInvolved(aOther.mXBLInvolved)
+    , mOriginalContent(aOther.mOriginalContent)
+  {}
 
   bool XBLInvolved() { return mXBLInvolved; }
 
+  const nsIContent* Parent() const { return mOriginalContent; }
+
 protected:
   /**
    * This constructor is a hack to help AllChildrenIterator which sometimes
    * doesn't want to consider XBL.
    */
   FlattenedChildIterator(const nsIContent* aParent, uint32_t aFlags,
                          bool aStartAtBeginning = true)
-    : ExplicitChildIterator(aParent, aStartAtBeginning), mXBLInvolved(false)
+    : ExplicitChildIterator(aParent, aStartAtBeginning)
+    , mXBLInvolved(false)
+    , mOriginalContent(aParent)
   {
     bool ignoreXBL = aFlags & nsIContent::eAllButXBL;
     Init(ignoreXBL);
   }
 
   void Init(bool aIgnoreXBL);
 
   // For certain optimizations, nsCSSFrameConstructor needs to know if the
   // child list of the element that we're iterating matches its .childNodes.
   bool mXBLInvolved;
+
+  const nsIContent* mOriginalContent;
 };
 
 /**
  * AllChildrenIterator traverses the children of an element including before /
  * after content and optionally XBL children.  The iterator can be initialized
  * to start at the end by providing false for aStartAtBeginning in order to
  * start iterating in reverse from the last child.
  *
@@ -175,22 +189,21 @@ protected:
  * iteration, and will break horribly if that is not true.
  */
 class AllChildrenIterator : private FlattenedChildIterator
 {
 public:
   AllChildrenIterator(const nsIContent* aNode, uint32_t aFlags,
                       bool aStartAtBeginning = true) :
     FlattenedChildIterator(aNode, aFlags, aStartAtBeginning),
-    mOriginalContent(aNode), mAnonKidsIdx(aStartAtBeginning ? UINT32_MAX : 0),
+    mAnonKidsIdx(aStartAtBeginning ? UINT32_MAX : 0),
     mFlags(aFlags), mPhase(aStartAtBeginning ? eAtBegin : eAtEnd) { }
 
   AllChildrenIterator(AllChildrenIterator&& aOther)
     : FlattenedChildIterator(Move(aOther)),
-      mOriginalContent(aOther.mOriginalContent),
       mAnonKids(Move(aOther.mAnonKids)), mAnonKidsIdx(aOther.mAnonKidsIdx),
       mFlags(aOther.mFlags), mPhase(aOther.mPhase)
 #ifdef DEBUG
       , mMutationGuard(aOther.mMutationGuard)
 #endif
       {}
 
 #ifdef DEBUG
@@ -199,37 +212,35 @@ public:
 
   // Returns the current target the iterator is at, or null if the iterator
   // doesn't point to any child node (either eAtBegin or eAtEnd phase).
   nsIContent* Get() const;
 
   // Seeks the given node in children of a parent element, starting from
   // the current iterator's position, and sets the iterator at the given child
   // node if it was found.
-  bool Seek(nsIContent* aChildToFind);
+  bool Seek(const nsIContent* aChildToFind);
 
   nsIContent* GetNextChild();
   nsIContent* GetPreviousChild();
-  const nsIContent* Parent() const { return mOriginalContent; }
 
   enum IteratorPhase
   {
     eAtBegin,
     eAtBeforeKid,
     eAtExplicitKids,
     eAtAnonKids,
     eAtAfterKid,
     eAtEnd
   };
   IteratorPhase Phase() const { return mPhase; }
 
 private:
   // Helpers.
   void AppendNativeAnonymousChildren();
-  const nsIContent* mOriginalContent;
 
   // mAnonKids is an array of native anonymous children, mAnonKidsIdx is index
   // in the array. If mAnonKidsIdx < mAnonKids.Length() and mPhase is
   // eAtAnonKids then the iterator points at a child at mAnonKidsIdx index. If
   // mAnonKidsIdx == mAnonKids.Length() then the iterator is somewhere after
   // the last native anon child. If mAnonKidsIdx == UINT32_MAX then the iterator
   // is somewhere before the first native anon child.
   nsTArray<nsIContent*> mAnonKids;
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -6864,125 +6864,157 @@ nsCSSFrameConstructor::IsValidSibling(ns
     if ((legendContent && (LayoutFrameType::Legend != sibType)) ||
         (!legendContent && (LayoutFrameType::Legend == sibType)))
       return false;
   }
 
   return true;
 }
 
+// FIXME(emilio): If we ever kill IsValidSibling() we can simplify this quite a
+// bit (no need to pass aTargetContent or aTargetContentDisplay, and the
+// adjust() calls can be responsibility of the caller).
+template<nsCSSFrameConstructor::SiblingDirection aDirection>
 nsIFrame*
-nsCSSFrameConstructor::FindFrameForContentSibling(nsIContent* aContent,
-                                                  nsIContent* aTargetContent,
-                                                  StyleDisplay& aTargetContentDisplay,
-                                                  nsContainerFrame* aParentFrame,
-                                                  bool aPrevSibling)
-{
-  nsIFrame* sibling = aContent->GetPrimaryFrame();
-  if (!sibling && GetDisplayContentsStyleFor(aContent)) {
-    // A display:contents node - check if it has a ::before / ::after frame...
-    sibling = aPrevSibling ? nsLayoutUtils::GetAfterFrame(aContent)
-                           : nsLayoutUtils::GetBeforeFrame(aContent);
-    if (!sibling) {
-      // ... then recurse into children ...
-      const bool forward = !aPrevSibling;
-      FlattenedChildIterator iter(aContent, forward);
-      sibling = aPrevSibling ?
-        FindPreviousSibling(iter, aTargetContent, aTargetContentDisplay, aParentFrame) :
-        FindNextSibling(iter, aTargetContent, aTargetContentDisplay, aParentFrame);
-
-      // The recursion above has already done all the placeholder and
-      // continuation fixups.
+nsCSSFrameConstructor::FindSiblingInternal(
+  FlattenedChildIterator aIter,
+  nsIContent* aTargetContent,
+  StyleDisplay& aTargetContentDisplay)
+{
+  auto adjust = [&](nsIFrame* aPotentialSiblingFrame) -> nsIFrame* {
+    return AdjustSiblingFrame(
+      aPotentialSiblingFrame, aTargetContent, aTargetContentDisplay,
+      aDirection);
+  };
+
+  auto nextDomSibling = [](FlattenedChildIterator& aIter) -> nsIContent* {
+    return aDirection == SiblingDirection::Forward
+      ? aIter.GetNextChild() : aIter.GetPreviousChild();
+  };
+
+  auto getNearPseudo = [](const nsIContent* aContent) -> nsIFrame* {
+    return aDirection == SiblingDirection::Forward
+      ? nsLayoutUtils::GetBeforeFrame(aContent)
+      : nsLayoutUtils::GetAfterFrame(aContent);
+  };
+
+  auto getFarPseudo = [](const nsIContent* aContent) -> nsIFrame* {
+    return aDirection == SiblingDirection::Forward
+      ? nsLayoutUtils::GetAfterFrame(aContent)
+      : nsLayoutUtils::GetBeforeFrame(aContent);
+  };
+
+  while (nsIContent* sibling = nextDomSibling(aIter)) {
+    if (nsIFrame* primaryFrame = sibling->GetPrimaryFrame()) {
+      // XXX the GetContent() == sibling check is needed due to bug 135040.
+      // Remove it once that's fixed.
+      if (primaryFrame->GetContent() == sibling) {
+        if (nsIFrame* frame = adjust(primaryFrame)) {
+          return frame;
+        }
+      }
+    }
+
+    if (GetDisplayContentsStyleFor(sibling)) {
+      if (nsIFrame* frame = adjust(getNearPseudo(sibling))) {
+        return frame;
+      }
+
+      const bool startFromBeginning = aDirection == SiblingDirection::Forward;
+      FlattenedChildIterator iter(sibling, startFromBeginning);
+      nsIFrame* sibling = FindSiblingInternal<aDirection>(
+        iter, aTargetContent, aTargetContentDisplay);
       if (sibling) {
         return sibling;
       }
     }
-    if (!sibling) {
-      // ... then ::after / ::before on the opposite end.
-      sibling = aPrevSibling ? nsLayoutUtils::GetAfterFrame(aContent)
-                             : nsLayoutUtils::GetBeforeFrame(aContent);
-    }
-    if (!sibling) {
-      return nullptr;
-    }
-  } else if (!sibling || sibling->GetContent() != aContent) {
-    // XXX the GetContent() != aContent check is needed due to bug 135040.
-    // Remove it once that's fixed.
+  }
+
+  return adjust(getFarPseudo(aIter.Parent()));
+}
+
+nsIFrame*
+nsCSSFrameConstructor::AdjustSiblingFrame(
+  nsIFrame* aSibling,
+  nsIContent* aTargetContent,
+  mozilla::StyleDisplay& aTargetContentDisplay,
+  SiblingDirection aDirection)
+{
+  if (!aSibling) {
     return nullptr;
   }
 
-  // If the frame is out-of-flow, GetPrimaryFrame() will have returned the
-  // out-of-flow frame; we want the placeholder.
-  if (sibling->GetStateBits() & NS_FRAME_OUT_OF_FLOW) {
-    nsIFrame* placeholderFrame = sibling->GetPlaceholderFrame();
-    NS_ASSERTION(placeholderFrame, "no placeholder for out-of-flow frame");
-    sibling = placeholderFrame;
-  }
-
-  // The frame we have now should never be a continuation.
-  NS_ASSERTION(!sibling->GetPrevContinuation(), "How did that happen?");
-
-  if (aPrevSibling) {
-    // The frame may be a ib-split frame (a split inline frame that
-    // contains a block).  Get the last part of that split.
-    if (IsFramePartOfIBSplit(sibling)) {
-      sibling = GetLastIBSplitSibling(sibling, true);
+  if (aSibling->GetStateBits() & NS_FRAME_OUT_OF_FLOW) {
+    aSibling = aSibling->GetPlaceholderFrame();
+    MOZ_ASSERT(aSibling);
+  }
+
+  MOZ_ASSERT(!aSibling->GetPrevContinuation(), "How?");
+  if (aDirection == SiblingDirection::Backward) {
+    // The frame may be a ib-split frame (a split inline frame that contains a
+    // block).  Get the last part of that split.
+    if (IsFramePartOfIBSplit(aSibling)) {
+      aSibling = GetLastIBSplitSibling(aSibling, true);
     }
 
     // The frame may have a continuation. If so, we want the last
     // non-overflow-container continuation as our previous sibling.
-    sibling = sibling->GetTailContinuation();
-  }
-
-  if (aTargetContent &&
-      !IsValidSibling(sibling, aTargetContent, aTargetContentDisplay)) {
-    sibling = nullptr;
-  }
-
-  return sibling;
+    aSibling = aSibling->GetTailContinuation();
+  }
+
+  if (!IsValidSibling(aSibling, aTargetContent, aTargetContentDisplay)) {
+    return nullptr;
+  }
+
+  return aSibling;
+}
+
+nsIFrame*
+nsCSSFrameConstructor::FindPreviousSibling(const FlattenedChildIterator& aIter,
+                                           StyleDisplay& aTargetContentDisplay)
+{
+  return FindSibling<SiblingDirection::Backward>(aIter, aTargetContentDisplay);
 }
 
 nsIFrame*
-nsCSSFrameConstructor::FindPreviousSibling(FlattenedChildIterator aIter,
-                                           nsIContent* aTargetContent,
-                                           StyleDisplay& aTargetContentDisplay,
-                                           nsContainerFrame* aParentFrame)
-{
-  // Note: not all content objects are associated with a frame (e.g., if it's
-  // `display: none') so keep looking until we find a previous frame.
-  while (nsIContent* sibling = aIter.GetPreviousChild()) {
-    MOZ_ASSERT(sibling != aTargetContent);
-    nsIFrame* prevSibling =
-      FindFrameForContentSibling(sibling, aTargetContent, aTargetContentDisplay,
-                                 aParentFrame, true);
-    if (prevSibling) {
-      // Found a previous sibling, we're done!
-      return prevSibling;
-    }
-  }
-
-  return nullptr;
-}
-
+nsCSSFrameConstructor::FindNextSibling(const FlattenedChildIterator& aIter,
+                                       StyleDisplay& aTargetContentDisplay)
+{
+  return FindSibling<SiblingDirection::Forward>(aIter, aTargetContentDisplay);
+}
+
+template<nsCSSFrameConstructor::SiblingDirection aDirection>
 nsIFrame*
-nsCSSFrameConstructor::FindNextSibling(FlattenedChildIterator aIter,
-                                       nsIContent* aTargetContent,
-                                       StyleDisplay& aTargetContentDisplay,
-                                       nsContainerFrame* aParentFrame)
-{
-  while (nsIContent* sibling = aIter.GetNextChild()) {
-    MOZ_ASSERT(sibling != aTargetContent);
-    nsIFrame* nextSibling =
-      FindFrameForContentSibling(sibling, aTargetContent, aTargetContentDisplay,
-                                 aParentFrame, false);
-
-    if (nextSibling) {
-      // We found a next sibling, we're done!
-      return nextSibling;
-    }
+nsCSSFrameConstructor::FindSibling(const FlattenedChildIterator& aIter,
+                                   StyleDisplay& aTargetContentDisplay)
+{
+  nsIContent* targetContent = aIter.Get();
+  nsIFrame* sibling =
+    FindSiblingInternal<aDirection>(aIter, targetContent, aTargetContentDisplay);
+  if (sibling) {
+    return sibling;
+  }
+
+  // Our siblings (if any) do not have a frame to guide us. The frame for the
+  // target content should be inserted whereever a frame for the container would
+  // be inserted. This is needed when inserting into display: contents nodes.
+  const nsIContent* current = aIter.Parent();
+  while (GetDisplayContentsStyleFor(current)) {
+    const nsIContent* parent = current->GetFlattenedTreeParent();
+    MOZ_ASSERT(parent, "No display: contents on the root");
+
+    FlattenedChildIterator iter(parent);
+    iter.Seek(current);
+    sibling = FindSiblingInternal<aDirection>(
+        iter, targetContent, aTargetContentDisplay);
+    if (sibling) {
+      return sibling;
+    }
+
+    current = parent;
   }
 
   return nullptr;
 }
 
 // For fieldsets, returns the area frame, if the child is not a legend.
 static nsContainerFrame*
 GetAdjustedParentFrame(nsContainerFrame* aParentFrame,
@@ -7037,56 +7069,32 @@ nsCSSFrameConstructor::GetInsertionPrevS
     MOZ_ASSERT(aChild->GetProperty(nsGkAtoms::restylableAnonymousNode),
                "Someone passed native anonymous content directly into frame "
                "construction.  Stop doing that!");
   }
 
   // Note that FindPreviousSibling is passed the iterator by value, so that
   // the later usage of the iterator starts from the same place.
   StyleDisplay childDisplay = UNSET_DISPLAY;
-  nsIFrame* prevSibling =
-    FindPreviousSibling(iter, iter.Get(), childDisplay, aInsertion->mParentFrame);
+  nsIFrame* prevSibling = FindPreviousSibling(iter, childDisplay);
 
   // Now, find the geometric parent so that we can handle
   // continuations properly. Use the prev sibling if we have it;
   // otherwise use the next sibling.
   if (prevSibling) {
     aInsertion->mParentFrame = prevSibling->GetParent()->GetContentInsertionFrame();
   } else {
     // If there is no previous sibling, then find the frame that follows
+    //
+    // FIXME(emilio): This is really complex and probably shouldn't be.
     if (aEndSkipChild) {
       iter.Seek(aEndSkipChild);
       iter.GetPreviousChild();
     }
-    nsIFrame* nextSibling =
-      FindNextSibling(iter, iter.Get(), childDisplay, aInsertion->mParentFrame);
-    if (GetDisplayContentsStyleFor(aInsertion->mContainer)) {
-      if (!nextSibling) {
-        // Our siblings (if any) does not have a frame to guide us.
-        // The frame for aChild should be inserted whereever a frame for
-        // the container would be inserted.  This is needed when inserting
-        // into nested display:contents nodes.
-        nsIContent* child = aInsertion->mContainer;
-        nsIContent* parent = child->GetParent();
-        aInsertion->mParentFrame =
-          ::GetAdjustedParentFrame(aInsertion->mParentFrame, parent);
-        InsertionPoint fakeInsertion(aInsertion->mParentFrame, parent);
-        nsIFrame* result = GetInsertionPrevSibling(&fakeInsertion, child, aIsAppend,
-                                                   aIsRangeInsertSafe, nullptr, nullptr);
-        MOZ_ASSERT(aInsertion->mParentFrame->GetContent() ==
-                   fakeInsertion.mParentFrame->GetContent());
-        // fakeInsertion.mParentFrame may now be a continuation of the frame
-        // we started with in the ctor above.
-        aInsertion->mParentFrame = fakeInsertion.mParentFrame;
-        return result;
-      }
-
-      prevSibling = nextSibling->GetPrevSibling();
-    }
-
+    nsIFrame* nextSibling = FindNextSibling(iter, childDisplay);
     if (nextSibling) {
       aInsertion->mParentFrame = nextSibling->GetParent()->GetContentInsertionFrame();
     } else {
       // No previous or next sibling, so treat this like an appended frame.
       *aIsAppend = true;
       if (IsFramePartOfIBSplit(aInsertion->mParentFrame)) {
         // Since we're appending, we'll walk to the last anonymous frame
         // that was created for the broken inline frame.  But don't walk
@@ -8334,32 +8342,16 @@ nsCSSFrameConstructor::ContentRangeInser
         return;
       }
 
       container = insertion.mParentFrame->GetContent();
       frameType = insertion.mParentFrame->Type();
     }
   }
 
-  if (!prevSibling) {
-    // We're inserting the new frames as the first child. See if the
-    // parent has a :before pseudo-element
-    nsIFrame* firstChild = insertion.mParentFrame->PrincipalChildList().FirstChild();
-
-    if (firstChild &&
-        nsLayoutUtils::IsGeneratedContentFor(container, firstChild,
-                                             nsCSSPseudoElements::before)) {
-      // Insert the new frames after the last continuation of the :before
-      prevSibling = firstChild->GetTailContinuation();
-      insertion.mParentFrame = prevSibling->GetParent()->GetContentInsertionFrame();
-      // Don't change isAppend here; we'll can call AppendFrames as needed, and
-      // the change to our prevSibling doesn't affect that.
-    }
-  }
-
   AutoFrameConstructionItemList items(this);
   ParentType parentType = GetParentType(frameType);
   FlattenedChildIterator iter(aContainer);
   bool haveNoXBLChildren = (!iter.XBLInvolved() || !iter.GetNextChild());
   if (aStartChild->GetPreviousSibling() &&
       parentType == eTypeBlock && haveNoXBLChildren) {
     // If there's a text node in the normal content list just before the
     // new nodes, and it has no frame, make a frame construction item for
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -2115,74 +2115,63 @@ private:
    * resolved for aFrameItems are wrong (they don't take ::first-line into
    * account), and we should fix them up, which is what this method does.
    *
    * This method does not mutate aFrameItems.
    */
   void CheckForFirstLineInsertion(nsIFrame* aParentFrame,
                                   nsFrameItems& aFrameItems);
 
-  /**
-   * Find the right frame to use for aContent when looking for sibling
-   * frames for aTargetContent.  If aPrevSibling is true, this
-   * will look for last continuations, etc, as necessary.  This calls
-   * IsValidSibling as needed; if that returns false it returns null.
-   *
-   * @param aContent the content to search for frames
-   * @param aTargetContent the content we're finding a sibling frame for
-   * @param aTargetContentDisplay the CSS display enum for aTargetContent if
-   *          already known, UNSET_DISPLAY otherwise. It will be filled in
-   *          if needed.
-   * @param aParentFrame the nearest ancestor frame, used internally for
-   *          finding ::after / ::before frames
-   * @param aPrevSibling true if we're searching in reverse DOM order
-   */
-  nsIFrame* FindFrameForContentSibling(nsIContent* aContent,
-                                       nsIContent* aTargetContent,
-                                       mozilla::StyleDisplay& aTargetContentDisplay,
-                                       nsContainerFrame* aParentFrame,
-                                       bool aPrevSibling);
+  // The direction in which we should look for siblings.
+  enum class SiblingDirection
+  {
+    Forward,
+    Backward,
+  };
 
   /**
-   * Find the frame for the content immediately preceding the one aIter
-   * points to, following continuations if necessary.  aIter is passed by
-   * value on purpose, so as not to modify the caller's iterator.
+   * Find the frame for the content immediately next to the one aIter points to,
+   * in the direction SiblingDirection indicates, following continuations if
+   * necessary.
+   *
+   * aIter is passed by const reference on purpose, so as not to modify the
+   * caller's iterator.
    *
    * @param aIter should be positioned such that aIter.GetPreviousChild()
    *          is the first content to search for frames
-   * @param aTargetContent the content we're finding a sibling frame for
-   * @param aTargetContentDisplay the CSS display enum for aTargetContent if
-   *          already known, UNSET_DISPLAY otherwise. It will be filled in
-   *          if needed.
-   * @param aParentFrame the nearest ancestor frame, used inernally for
-   *          finding ::after / ::before frames
+   * @param aTargetContentDisplay the CSS display enum for the content aIter
+   *          points to if already known, UNSET_DISPLAY otherwise. It will be
+   *          filled in if needed.
    */
-  nsIFrame* FindPreviousSibling(mozilla::dom::FlattenedChildIterator aIter,
-                                nsIContent* aTargetContent,
-                                mozilla::StyleDisplay& aTargetContentDisplay,
-                                nsContainerFrame* aParentFrame);
+  template<SiblingDirection>
+  nsIFrame* FindSibling(const mozilla::dom::FlattenedChildIterator& aIter,
+                        mozilla::StyleDisplay& aTargetContentDisplay);
+
+  // Helper for the implementation of FindSibling.
+  template<SiblingDirection>
+  nsIFrame* FindSiblingInternal(
+    mozilla::dom::FlattenedChildIterator,
+    nsIContent* aTargetContent,
+    mozilla::StyleDisplay& aTargetContentDisplay);
 
-  /**
-   * Find the frame for the content node immediately following the one aIter
-   * points to, following continuations if necessary.  aIter is passed by value
-   * on purpose, so as not to modify the caller's iterator.
-   *
-   * @param aIter should be positioned such that aIter.GetNextChild()
-   *          is the first content to search for frames
-   * @param aTargetContent the content we're finding a sibling frame for
-   * @param aTargetContentDisplay the CSS display enum for aTargetContent if
-   *          already known, UNSET_DISPLAY otherwise. It will be filled in
-   *          if needed.
-   * @param aParentFrame the nearest ancestor frame, used inernally for
-   *          finding ::after / ::before frames
-   */
-  nsIFrame* FindNextSibling(mozilla::dom::FlattenedChildIterator aIter,
-                            nsIContent* aTargetContent,
-                            mozilla::StyleDisplay& aTargetContentDisplay,
-                            nsContainerFrame* aParentFrame);
+  // An alias of FindSibling<SiblingDirection::Forward>.
+  nsIFrame* FindNextSibling(const mozilla::dom::FlattenedChildIterator& aIter,
+                            mozilla::StyleDisplay& aTargetContentDisplay);
+  // An alias of FindSibling<SiblingDirection::Backwards>.
+  nsIFrame* FindPreviousSibling(const mozilla::dom::FlattenedChildIterator& aIter,
+                                mozilla::StyleDisplay& aTargetContentDisplay);
+
+  // Given a potential first-continuation sibling frame for aTargetContent,
+  // verify that it is an actual valid sibling for it, and return the
+  // appropriate continuation the new frame for aTargetContent should be
+  // inserted next to.
+  nsIFrame* AdjustSiblingFrame(nsIFrame* aSibling,
+                               nsIContent* aTargetContent,
+                               mozilla::StyleDisplay& aTargetContentDisplay,
+                               SiblingDirection aDirection);
 
   // Find the right previous sibling for an insertion.  This also updates the
   // parent frame to point to the correct continuation of the parent frame to
   // use, and returns whether this insertion is to be treated as an append.
   // aChild is the child being inserted.
   // aIsRangeInsertSafe returns whether it is safe to do a range insert with
   // aChild being the first child in the range. It is the callers'
   // responsibility to check whether a range insert is safe with regards to
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -1731,43 +1731,16 @@ nsLayoutUtils::GetFloatFromPlaceholder(n
                  "How did that happen?");
     return outOfFlowFrame;
   }
 
   return nullptr;
 }
 
 // static
-bool
-nsLayoutUtils::IsGeneratedContentFor(nsIContent* aContent,
-                                     nsIFrame* aFrame,
-                                     nsAtom* aPseudoElement)
-{
-  NS_PRECONDITION(aFrame, "Must have a frame");
-  NS_PRECONDITION(aPseudoElement, "Must have a pseudo name");
-
-  if (!aFrame->IsGeneratedContentFrame()) {
-    return false;
-  }
-  nsIFrame* parent = aFrame->GetParent();
-  NS_ASSERTION(parent, "Generated content can't be root frame");
-  if (parent->IsGeneratedContentFrame()) {
-    // Not the root of the generated content
-    return false;
-  }
-
-  if (aContent && parent->GetContent() != aContent) {
-    return false;
-  }
-
-  return (aFrame->GetContent()->NodeInfo()->NameAtom() == nsGkAtoms::mozgeneratedcontentbefore) ==
-    (aPseudoElement == nsCSSPseudoElements::before);
-}
-
-// static
 nsIFrame*
 nsLayoutUtils::GetCrossDocParentFrame(const nsIFrame* aFrame,
                                       nsPoint* aExtraOffset)
 {
   nsIFrame* p = aFrame->GetParent();
   if (p)
     return p;
 
--- a/layout/base/nsLayoutUtils.h
+++ b/layout/base/nsLayoutUtils.h
@@ -389,33 +389,16 @@ public:
    * Gets the real primary frame associated with the content object.
    *
    * In the case of absolutely positioned elements and floated elements,
    * the real primary frame is the frame that is out of the flow and not the
    * placeholder frame.
    */
   static nsIFrame* GetRealPrimaryFrameFor(const nsIContent* aContent);
 
-  /**
-   * IsGeneratedContentFor returns true if aFrame is the outermost
-   * frame for generated content of type aPseudoElement for aContent.
-   * aFrame *might not* have the aPseudoElement pseudo-style! For example
-   * it might be a table wrapper frame and the inner table frame might
-   * have the pseudo-style.
-   *
-   * @param aContent the content node we're looking at.  If this is
-   *        null, then we just assume that aFrame has the right content
-   *        pointer.
-   * @param aFrame the frame we're looking at
-   * @param aPseudoElement the pseudo type we're interested in
-   * @return whether aFrame is the generated aPseudoElement frame for aContent
-   */
-  static bool IsGeneratedContentFor(nsIContent* aContent, nsIFrame* aFrame,
-                                      nsAtom* aPseudoElement);
-
 #ifdef DEBUG
   // TODO: remove, see bug 598468.
   static bool gPreventAssertInCompareTreePosition;
 #endif // DEBUG
 
   /**
    * CompareTreePosition determines whether aContent1 comes before or
    * after aContent2 in a preorder traversal of the content tree.
--- a/layout/reftests/css-grid/reftest.list
+++ b/layout/reftests/css-grid/reftest.list
@@ -241,17 +241,17 @@ asserts(0-10) == grid-fragmentation-015.
 == grid-fragmentation-dyn2-018.html grid-fragmentation-018-ref.html
 == grid-fragmentation-dyn1-019.html grid-fragmentation-019-ref.html
 == grid-fragmentation-dyn2-019.html grid-fragmentation-019-ref.html
 == grid-fragmentation-dyn3-019.html grid-fragmentation-019-ref.html
 == grid-fragmentation-dyn4-019.html grid-fragmentation-019-ref.html
 == grid-fragmentation-dyn5-019.html grid-fragmentation-019-ref.html
 == grid-fragmentation-dyn1-020.html grid-fragmentation-020-ref.html
 == grid-fragmentation-dyn2-020.html grid-fragmentation-020-ref.html
-!= grid-fragmentation-dyn1-021.html grid-fragmentation-021-ref.html # bug 1251799
+== grid-fragmentation-dyn1-021.html grid-fragmentation-021-ref.html
 == grid-fragmentation-dyn2-021.html grid-fragmentation-021-ref.html
 == grid-fragmentation-dyn3-021.html grid-fragmentation-021-ref.html
 == grid-fragmentation-dyn4-021.html grid-fragmentation-021-ref.html
 == grid-fragmentation-dyn5-021.html grid-fragmentation-021-ref.html
 == grid-fragmentation-dyn2-022.html grid-fragmentation-007-ref.html
 == grid-fragmentation-dyn1-023.html grid-fragmentation-023-ref.html
 == grid-fragmentation-dyn2-023.html grid-fragmentation-023-ref.html
 == grid-fragmentation-dyn3-023.html grid-fragmentation-023-ref.html
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -104164,16 +104164,28 @@
       [
        "/css/css-display-3/display-contents-multicol-001-ref.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/css-display-3/display-contents-dynamic-pseudo-insertion-001.html": [
+    [
+     "/css/css-display-3/display-contents-dynamic-pseudo-insertion-001.html",
+     [
+      [
+       "/css/css-display-3/display-contents-dynamic-pseudo-insertion-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/css-display-3/display-contents-dynamic-table-001-inline.html": [
     [
      "/css/css-display-3/display-contents-dynamic-table-001-inline.html",
      [
       [
        "/css/css-display-3/display-contents-table-001-ref.html",
        "=="
       ]
@@ -229639,16 +229651,21 @@
      {}
     ]
    ],
    "css/css-display-3/display-contents-dynamic-generated-content-fieldset-001-ref.html": [
     [
      {}
     ]
    ],
+   "css/css-display-3/display-contents-dynamic-pseudo-insertion-001-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/css-display-3/display-contents-flex-001-ref.html": [
     [
      {}
     ]
    ],
    "css/css-display-3/display-contents-flex-002-ref.html": [
     [
      {}
@@ -480794,16 +480811,24 @@
   "css/css-display-3/display-contents-dynamic-multicol-001-inline.html": [
    "e5f365024f14c246ed8e9410e1f08e5d48b1c3ff",
    "reftest"
   ],
   "css/css-display-3/display-contents-dynamic-multicol-001-none.html": [
    "4fe143c25be9ecc08e58428f0c588f72c5cb8a3f",
    "reftest"
   ],
+  "css/css-display-3/display-contents-dynamic-pseudo-insertion-001-ref.html": [
+   "af3b16fc8d05be3ab16d65a2f668233668a23583",
+   "support"
+  ],
+  "css/css-display-3/display-contents-dynamic-pseudo-insertion-001.html": [
+   "221fb940d34860f62541ad53bb4b3834f3552577",
+   "reftest"
+  ],
   "css/css-display-3/display-contents-dynamic-table-001-inline.html": [
    "5c105a711c083e61eefc4121c707d3ca2ffda3ec",
    "reftest"
   ],
   "css/css-display-3/display-contents-dynamic-table-001-none.html": [
    "77723986b19e8c6ed8f66cdd92670eec724bf48d",
    "reftest"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-display-3/display-contents-dynamic-pseudo-insertion-001-ref.html
@@ -0,0 +1,5 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+PASS
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-display-3/display-contents-dynamic-pseudo-insertion-001.html
@@ -0,0 +1,26 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test: Dynamic insertion on empty display: contents element with pseudo-elements</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://drafts.csswg.org/css-display-3/#valdef-display-contents">
+<link rel=match href="display-contents-dynamic-pseudo-insertion-001-ref.html">
+<style>
+.contents {
+  display: contents;
+  border: 10px solid red;
+}
+.contents::before {
+  content: "A";
+}
+.contents::after {
+  content: "SS";
+}
+</style>
+<div class="contents"></div>
+<script>
+document.body.offsetTop;
+let span = document.createElement('span');
+span.innerHTML = "P";
+let contents = document.querySelector('.contents');
+contents.parentNode.insertBefore(span, contents);
+</script>