Bug 1908242 - Make sure that sticky items are processed in the right order. r=TYLin

We rely on positioning ancestors before descendants to get the right
position. This was very implicitly enforced before the regressing bug,
by reframing on position changes (due to the order
StickyScrollContainer::AddFrame was called, ancestors before children).

In order to support dynamic sticky changes without reframing, we need to
handle the case of an existing ancestor switching position to sticky
dynamically.

Luckily we already have the right data-structure for that, so the change
is rather trivial.

Write a testharness test for this because APZ does get the position
right, so it's more future-proof against regressions to just test the
raw layout values.

Differential Revision: https://phabricator.services.mozilla.com/D216858

layout/base/DepthOrderedFrameList.h

@@ -35,6 +35,11 @@ class DepthOrderedFrameList {
 
   auto IterFromShallowest() const { return Reversed(mList); }
 
+  // The number of elements that there are.
+  size_t Length() const { return mList.Length(); }
+  nsIFrame* ElementAt(size_t i) const { return mList.ElementAt(i); }
+  void RemoveElementAt(size_t i) { mList.RemoveElementAt(i); }
+
  private:
   struct FrameAndDepth {
     nsIFrame* mFrame;

layout/generic/ScrollContainerFrame.cpp

@@ -5129,8 +5129,7 @@ static nsSize GetScrollPortSizeExcludingHeadersAndFooters(
       StickyScrollContainer::GetStickyScrollContainerForScrollFrame(
           aScrollFrame);
   if (ssc) {
-    const nsTArray<nsIFrame*>& stickyFrames = ssc->GetFrames();
-    for (nsIFrame* f : stickyFrames) {
+    for (nsIFrame* f : ssc->GetFrames().IterFromShallowest()) {
       // If it's acting like fixed position.
       if (ssc->IsStuckInYDirection(f)) {
         AddToListIfHeaderFooter(f, aScrollFrame, aScrollPort, list);

layout/generic/StickyScrollContainer.cpp

@@ -351,22 +351,23 @@ void StickyScrollContainer::UpdatePositions(nsPoint aScrollPosition,
 
   OverflowChangedTracker oct;
   oct.SetSubtreeRoot(aSubtreeRoot);
-  for (nsTArray<nsIFrame*>::size_type i = 0; i < mFrames.Length(); i++) {
-    nsIFrame* f = mFrames[i];
+  // We need to position ancestors before children, so iter from shallowest. We
+  // don't use mFrames.IterFromShallowest() because we want to handle removal of
+  // non-first continuations while we iterate.
+  for (size_t i = mFrames.Length(); i--;) {
+    nsIFrame* f = mFrames.ElementAt(i);
     if (!nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(f)) {
-      // This frame was added in nsIFrame::Init before we knew it wasn't
-      // the first ib-split-sibling.
+      // This frame was added in nsIFrame::DidSetComputedStyle before we knew it
+      // wasn't the first ib-split-sibling.
       mFrames.RemoveElementAt(i);
-      --i;
       continue;
     }
-
     if (aSubtreeRoot) {
       // Reflowing the scroll frame, so recompute offsets.
       ComputeStickyOffsets(f);
     }
     // mFrames will only contain first continuations, because we filter in
-    // nsIFrame::Init.
+    // nsIFrame::DidSetComputedStyle.
     PositionContinuations(f);
 
     f = f->GetParent();

layout/generic/StickyScrollContainer.h

@@ -12,10 +12,11 @@
 #ifndef StickyScrollContainer_h
 #define StickyScrollContainer_h
 
+#include "mozilla/DepthOrderedFrameList.h"
+#include "nsIScrollPositionListener.h"
 #include "nsPoint.h"
 #include "nsRectAbsolute.h"
 #include "nsTArray.h"
-#include "nsIScrollPositionListener.h"
 
 struct nsRect;
 class nsIFrame;
@@ -40,8 +41,8 @@ class StickyScrollContainer final : public nsIScrollPositionListener {
   static StickyScrollContainer* GetStickyScrollContainerForScrollFrame(
       nsIFrame* aScrollFrame);
 
-  void AddFrame(nsIFrame* aFrame) { mFrames.AppendElement(aFrame); }
-  void RemoveFrame(nsIFrame* aFrame) { mFrames.RemoveElement(aFrame); }
+  void AddFrame(nsIFrame* aFrame) { mFrames.Add(aFrame); }
+  void RemoveFrame(nsIFrame* aFrame) { mFrames.Remove(aFrame); }
 
   ScrollContainerFrame* ScrollContainer() const {
     return mScrollContainerFrame;
@@ -82,7 +83,7 @@ class StickyScrollContainer final : public nsIScrollPositionListener {
 
   ~StickyScrollContainer();
 
-  const nsTArray<nsIFrame*>& GetFrames() const { return mFrames; }
+  const DepthOrderedFrameList& GetFrames() const { return mFrames; }
 
   /**
    * Returns true if the frame is "stuck" in the y direction, ie it's acting
@@ -103,7 +104,7 @@ class StickyScrollContainer final : public nsIScrollPositionListener {
                            nsRect* aContain) const;
 
   ScrollContainerFrame* const mScrollContainerFrame;
-  nsTArray<nsIFrame*> mFrames;
+  DepthOrderedFrameList mFrames;
   nsPoint mScrollPosition;
 };
 

testing/web-platform/tests/css/css-position/position-sticky-dynamic-ancestor-001.html

@@ -0,0 +1,63 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<meta name="viewport" content="width=device-width">
+<title>Dynamic sticky position change doesn't break inner sticky positioned items</title>
+<link rel="help" href="https://drafts.csswg.org/css-position-3/#sticky-position">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1908242">
+<link rel="author" title="Emilio Cobos" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  body {
+    margin: 0;
+    display: flex;
+  }
+
+  #content {
+    border: 10px dashed gray;
+    height: 600vh;
+  }
+
+  #sidebar {
+    align-self: start;
+    background-color: white;
+    border: 10px dashed gray;
+    top: 0;
+  }
+
+  #sidebar-header {
+    position: sticky;
+    top: 0;
+    background-color: rgba(255, 0, 0, .5);
+  }
+
+  #sidebar-content {
+    padding: 5px;
+    height: 200vh;
+  }
+</style>
+<div id=content>
+  CONTENT
+</div>
+<div id=sidebar>
+  <div id=sidebar-header>
+    SIDEBAR TOP STICKY
+  </div>
+  <div id=sidebar-content>
+    SIDEBAR CONTENT
+  </div>
+</div>
+<script>
+let sidebar = document.getElementById("sidebar");
+let sidebarHeader = document.getElementById("sidebar-header");
+test(function() {
+  // Make the header and sidebar stick.
+  window.scrollTo(0, 100);
+  assert_less_than(sidebar.getBoundingClientRect().top, 0, "Sidebar should not be stuck (yet)");
+  assert_equals(sidebarHeader.getBoundingClientRect().top, 0, "Sidebar header should be stuck");
+  sidebar.style.position = "sticky";
+  assert_equals(sidebar.getBoundingClientRect().top, 0, "Sidebar should be stuck now");
+  assert_equals(sidebarHeader.getBoundingClientRect().top, 10, "Sidebar header should be stuck under sidebar border");
+});
+</script>