Bug 1520344 - Store last scroll anchor position in writing-mode relative coordinates. r=dholbert
authorRyan Hunt <rhunt@eqrion.net>
Wed, 16 Jan 2019 15:13:12 -0600
changeset 511662 57aacc322be1c01f82b987a41f6e577260778d0e
parent 511661 05d0f0893f36b19412c516e5ee97c628f624a307
child 511663 ef836f7dfc086551403866e9dd5ca81feb4cdabc
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1520344
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1520344 - Store last scroll anchor position in writing-mode relative coordinates. r=dholbert This commit changes ScrollAnchorContainer to store the offset between the scroll anchoring bounding rect start edge and the scroll-port start edge in the block axis of the scroll frame. The logic to clamp the negative portions of scroll anchor bounding rect is also amended to only clamp the portion that is beyond the border start edge in the block axis of the scroll frame. Differential Revision: https://phabricator.services.mozilla.com/D16757
layout/generic/ScrollAnchorContainer.cpp
layout/generic/ScrollAnchorContainer.h
testing/web-platform/meta/css/css-scroll-anchoring/start-edge-in-block-layout-direction.html.ini
--- a/layout/generic/ScrollAnchorContainer.cpp
+++ b/layout/generic/ScrollAnchorContainer.cpp
@@ -16,17 +16,17 @@
 // #define ANCHOR_LOG(...) printf_stderr("ANCHOR: " __VA_ARGS__)
 
 namespace mozilla {
 namespace layout {
 
 ScrollAnchorContainer::ScrollAnchorContainer(ScrollFrameHelper* aScrollFrame)
     : mScrollFrame(aScrollFrame),
       mAnchorNode(nullptr),
-      mLastAnchorPos(0, 0),
+      mLastAnchorOffset(0),
       mAnchorNodeIsDirty(true),
       mApplyingAnchorAdjustment(false),
       mSuppressAnchorAdjustment(false) {}
 
 ScrollAnchorContainer::~ScrollAnchorContainer() {}
 
 ScrollAnchorContainer* ScrollAnchorContainer::FindFor(nsIFrame* aFrame) {
   aFrame = aFrame->GetParent();
@@ -72,55 +72,93 @@ static void SetAnchorFlags(const nsIFram
          frame = frame->GetNextContinuation()) {
       frame->InvalidateFrame();
     }
   }
 }
 
 /**
  * Compute the scrollable overflow rect [1] of aCandidate relative to
- * aScrollFrame with all transforms applied. The specification is also
- * ambiguous about what can be selected as a scroll anchor, which makes
- * the scroll anchoring bounding rect partially undefined [2]. This code
- * attempts to match the implementation in Blink.
+ * aScrollFrame with all transforms applied.
+ *
+ * The specification is ambiguous about what can be selected as a scroll anchor,
+ * which makes the scroll anchoring bounding rect partially undefined [2]. This
+ * code attempts to match the implementation in Blink.
+ *
+ * An additional unspecified behavior is that any scrollable overflow before the
+ * border start edge in the block axis of aScrollFrame should be clamped. This
+ * is to prevent absolutely positioned descendant elements from being able to
+ * trigger scroll adjustments [3].
  *
  * [1]
  * https://drafts.csswg.org/css-scroll-anchoring-1/#scroll-anchoring-bounding-rect
  * [2] https://github.com/w3c/csswg-drafts/issues/3478
+ * [3] https://bugzilla.mozilla.org/show_bug.cgi?id=1519541
  */
 static nsRect FindScrollAnchoringBoundingRect(const nsIFrame* aScrollFrame,
                                               nsIFrame* aCandidate) {
   MOZ_ASSERT(nsLayoutUtils::IsProperAncestorFrame(aScrollFrame, aCandidate));
   if (!!Text::FromNodeOrNull(aCandidate->GetContent())) {
     nsRect bounding;
     for (nsIFrame* continuation = aCandidate->FirstContinuation(); continuation;
          continuation = continuation->GetNextContinuation()) {
-      nsRect localRect =
+      nsRect overflowRect =
           continuation->GetScrollableOverflowRectRelativeToSelf();
       nsRect transformed = nsLayoutUtils::TransformFrameRectToAncestor(
-          continuation, localRect, aScrollFrame);
+          continuation, overflowRect, aScrollFrame);
       bounding = bounding.Union(transformed);
     }
     return bounding;
   }
 
-  nsRect localRect = aCandidate->GetScrollableOverflowRectRelativeToSelf();
+  nsRect borderRect = aCandidate->GetRectRelativeToSelf();
+  nsRect overflowRect = aCandidate->GetScrollableOverflowRectRelativeToSelf();
+
+  NS_ASSERTION(overflowRect.Contains(borderRect),
+               "overflow rect must include border rect, and the clamping logic "
+               "here depends on that");
 
-  // XXX this isn't correct with non-vertical-tb writing-mode, see bug 1520344
-  if (localRect.X() < 0) {
-    localRect.SetBoxX(0, localRect.XMost());
+  // Clamp the scrollable overflow rect to the border start edge on the block
+  // axis of the scroll frame
+  WritingMode writingMode = aScrollFrame->GetWritingMode();
+  switch (writingMode.GetBlockDir()) {
+    case WritingMode::eBlockTB: {
+      overflowRect.SetBoxY(borderRect.Y(), overflowRect.YMost());
+      break;
+    }
+    case WritingMode::eBlockLR: {
+      overflowRect.SetBoxX(borderRect.X(), overflowRect.XMost());
+      break;
+    }
+    case WritingMode::eBlockRL: {
+      overflowRect.SetBoxX(overflowRect.X(), borderRect.XMost());
+      break;
+    }
   }
-  if (localRect.Y() < 0) {
-    localRect.SetBoxY(0, localRect.YMost());
-  }
+
   nsRect transformed = nsLayoutUtils::TransformFrameRectToAncestor(
-      aCandidate, localRect, aScrollFrame);
+      aCandidate, overflowRect, aScrollFrame);
   return transformed;
 }
 
+/**
+ * Compute the offset between the scrollable overflow rect start edge of
+ * aCandidate and the scroll-port start edge of aScrollFrame, in the block axis
+ * of aScrollFrame.
+ */
+static nscoord FindScrollAnchoringBoundingOffset(
+    const ScrollFrameHelper* aScrollFrame, nsIFrame* aCandidate) {
+  WritingMode writingMode = aScrollFrame->mOuter->GetWritingMode();
+  nsRect physicalBounding =
+      FindScrollAnchoringBoundingRect(aScrollFrame->mOuter, aCandidate);
+  LogicalRect logicalBounding(writingMode, physicalBounding,
+                              aScrollFrame->mScrolledFrame->GetSize());
+  return logicalBounding.BStart(writingMode);
+}
+
 void ScrollAnchorContainer::SelectAnchor() {
   MOZ_ASSERT(mScrollFrame->mScrolledFrame);
   MOZ_ASSERT(mAnchorNodeIsDirty);
 
   if (!StaticPrefs::layout_css_scroll_anchoring_enabled()) {
     return;
   }
 
@@ -186,22 +224,21 @@ void ScrollAnchorContainer::SelectAnchor
       SetAnchorFlags(mScrollFrame->mScrolledFrame, mAnchorNode, true);
     }
   } else {
     ANCHOR_LOG("Anchor node has remained (%p).\n", mAnchorNode);
   }
 
   // Calculate the position to use for scroll adjustments
   if (mAnchorNode) {
-    mLastAnchorPos =
-        FindScrollAnchoringBoundingRect(Frame(), mAnchorNode).TopLeft();
-    ANCHOR_LOG("Using last anchor position = [%d, %d].\n", mLastAnchorPos.x,
-               mLastAnchorPos.y);
+    mLastAnchorOffset =
+        FindScrollAnchoringBoundingOffset(mScrollFrame, mAnchorNode);
+    ANCHOR_LOG("Using last anchor offset = %d.\n", mLastAnchorOffset);
   } else {
-    mLastAnchorPos = nsPoint();
+    mLastAnchorOffset = 0;
   }
 
   mAnchorNodeIsDirty = false;
 }
 
 void ScrollAnchorContainer::UserScrolled() {
   if (mApplyingAnchorAdjustment) {
     return;
@@ -221,92 +258,94 @@ void ScrollAnchorContainer::InvalidateAn
 
   ANCHOR_LOG("Invalidating scroll anchor %p for %p.\n", mAnchorNode, this);
 
   if (mAnchorNode) {
     SetAnchorFlags(mScrollFrame->mScrolledFrame, mAnchorNode, false);
   }
   mAnchorNode = nullptr;
   mAnchorNodeIsDirty = true;
-  mLastAnchorPos = nsPoint();
+  mLastAnchorOffset = 0;
   Frame()->PresShell()->PostPendingScrollAnchorSelection(this);
 }
 
 void ScrollAnchorContainer::Destroy() {
   if (mAnchorNode) {
     SetAnchorFlags(mScrollFrame->mScrolledFrame, mAnchorNode, false);
   }
   mAnchorNode = nullptr;
   mAnchorNodeIsDirty = false;
-  mLastAnchorPos = nsPoint();
+  mLastAnchorOffset = 0;
 }
 
 void ScrollAnchorContainer::ApplyAdjustments() {
   if (!mAnchorNode || mAnchorNodeIsDirty) {
     mSuppressAnchorAdjustment = false;
     ANCHOR_LOG("Ignoring post-reflow (anchor=%p, dirty=%d, container=%p).\n",
                mAnchorNode, mAnchorNodeIsDirty, this);
     return;
   }
 
-  nsPoint current =
-      FindScrollAnchoringBoundingRect(Frame(), mAnchorNode).TopLeft();
-  nsPoint adjustment = current - mLastAnchorPos;
-  nsIntPoint adjustmentDevicePixels =
-      adjustment.ToNearestPixels(Frame()->PresContext()->AppUnitsPerDevPixel());
-
-  ANCHOR_LOG("Anchor has moved from [%d, %d] to [%d, %d].\n", mLastAnchorPos.x,
-             mLastAnchorPos.y, current.x, current.y);
-
+  nscoord current =
+      FindScrollAnchoringBoundingOffset(mScrollFrame, mAnchorNode);
+  nscoord logicalAdjustment = current - mLastAnchorOffset;
   WritingMode writingMode = Frame()->GetWritingMode();
 
-  // The specification only allows for anchor adjustments in the block
-  // dimension, so remove the other component.
-  if (writingMode.IsVertical()) {
-    adjustmentDevicePixels.y = 0;
-  } else {
-    adjustmentDevicePixels.x = 0;
-  }
+  ANCHOR_LOG("Anchor has moved from %d to %d.\n", mLastAnchorOffset, current);
 
-  if (adjustmentDevicePixels == nsIntPoint()) {
+  if (logicalAdjustment == 0) {
     ANCHOR_LOG("Ignoring zero delta anchor adjustment for %p.\n", this);
     mSuppressAnchorAdjustment = false;
     return;
   }
 
   if (mSuppressAnchorAdjustment) {
     ANCHOR_LOG("Applying anchor adjustment suppression for %p.\n", this);
     mSuppressAnchorAdjustment = false;
     InvalidateAnchor();
     return;
   }
 
-  ANCHOR_LOG("Applying anchor adjustment of (%d %d) for %p and anchor %p.\n",
-             adjustment.x, adjustment.y, this, mAnchorNode);
+  ANCHOR_LOG("Applying anchor adjustment of %d in %s for %p and anchor %p.\n",
+             logicalAdjustment, writingMode.DebugString(), this, mAnchorNode);
+
+  nsPoint physicalAdjustment;
+  switch (writingMode.GetBlockDir()) {
+    case WritingMode::eBlockTB: {
+      physicalAdjustment.y = logicalAdjustment;
+      break;
+    }
+    case WritingMode::eBlockLR: {
+      physicalAdjustment.x = logicalAdjustment;
+      break;
+    }
+    case WritingMode::eBlockRL: {
+      physicalAdjustment.x = -logicalAdjustment;
+      break;
+    }
+  }
+  nsIntPoint physicalDevicePixels = physicalAdjustment.ToNearestPixels(
+      Frame()->PresContext()->AppUnitsPerDevPixel());
 
   MOZ_ASSERT(!mApplyingAnchorAdjustment);
   // We should use AutoRestore here, but that doesn't work with bitfields
   mApplyingAnchorAdjustment = true;
   mScrollFrame->ScrollBy(
-      adjustmentDevicePixels, nsIScrollableFrame::DEVICE_PIXELS,
+      physicalDevicePixels, nsIScrollableFrame::DEVICE_PIXELS,
       nsIScrollableFrame::INSTANT, nullptr, nsGkAtoms::relative);
   mApplyingAnchorAdjustment = false;
 
   nsPresContext* pc = Frame()->PresContext();
   Document* doc = pc->Document();
-  if (writingMode.IsVertical()) {
-    doc->UpdateForScrollAnchorAdjustment(adjustment.x);
-  } else {
-    doc->UpdateForScrollAnchorAdjustment(adjustment.y);
-  }
+  doc->UpdateForScrollAnchorAdjustment(logicalAdjustment);
 
   // The anchor position may not be in the same relative position after
   // adjustment. Update ourselves so we have consistent state.
-  mLastAnchorPos =
-      FindScrollAnchoringBoundingRect(Frame(), mAnchorNode).TopLeft();
+  mLastAnchorOffset =
+      FindScrollAnchoringBoundingOffset(mScrollFrame, mAnchorNode);
 }
 
 ScrollAnchorContainer::ExamineResult
 ScrollAnchorContainer::ExamineAnchorCandidate(nsIFrame* aFrame) const {
 #ifdef DEBUG_FRAME_DUMP
   nsCString tag = aFrame->ListTag();
   ANCHOR_LOG("\tVisiting frame=%s (%p).\n", tag.get(), aFrame);
 #else
--- a/layout/generic/ScrollAnchorContainer.h
+++ b/layout/generic/ScrollAnchorContainer.h
@@ -127,20 +127,21 @@ class ScrollAnchorContainer final {
   // The owner of this scroll anchor container
   ScrollFrameHelper* mScrollFrame;
 
   // The anchor node that we will scroll to keep in the same relative position
   // after reflows. This may be null if we were not able to select a valid
   // scroll anchor
   nsIFrame* mAnchorNode;
 
-  // The last position of the scroll anchor node relative to the scrollable
-  // frame. This is used for calculating the distance to scroll to keep the
-  // anchor node in the same relative position
-  nsPoint mLastAnchorPos;
+  // The last offset of the scroll anchor node's scrollable overflow rect start
+  // edge relative to the scroll-port start edge, in the block axis of the
+  // scroll frame. This is used for calculating the distance to scroll to keep
+  // the anchor node in the same relative position
+  nscoord mLastAnchorOffset;
 
   // True if we should recalculate our anchor node at the next chance
   bool mAnchorNodeIsDirty : 1;
   // True if we are applying a scroll anchor adjustment
   bool mApplyingAnchorAdjustment : 1;
   // True if we should suppress anchor adjustments
   bool mSuppressAnchorAdjustment : 1;
 };
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-scroll-anchoring/start-edge-in-block-layout-direction.html.ini
+++ /dev/null
@@ -1,11 +0,0 @@
-[start-edge-in-block-layout-direction.html]
-  [Vertical-RL LTR.]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1447743
-    issue: https://github.com/w3c/csswg-drafts/issues/2704
-
-  [Vertical-RL RTL.]
-    expected: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1447743
-    issue: https://github.com/w3c/csswg-drafts/issues/2704
-