Bug 1534070 - Factor scroll-padding into the position calculation where nsIPresShell::ScrollContentIntoView() is going to scroll if necessary. r=botond

In the case where scroll-snap-type is specified for the scroll container, the
scroll-padding is also factored into in ScrollFrameHelper::ComputeScrollSnapInfo
which is called via ScrollFrameHelper::ScrollToWithOrigin.  It doesn't double
the scroll-padding value, but it's actually redundant, we should avoid it.
We could separate the functionality of ScrollToWithOrigin, one is to scroll
to a given element, the other is to scroll to a given position.  The former will
be used for Element.scrollIntoElement and relevant stuff, the latter will be
used for Element.scrollTo and relevant stuff.  That's being said, as of now, we
have still the old scroll snap implementation, so the separation will introduce
complexity, the separation should be done once after the old implementation
removed.

There are 9 call sites of nsIPresShell::ScrollContentIntoView:
  nsIPresShell::GoToAnchor
  nsIPresShell::ScrollToAnchor
  Element::ScrollIntoView
   We definitely needs scroll-padding and scroll-margin for these functions.

  nsCoreUtils::ScrollTo
   This is used for Accesible::ScrollTo which scrolls to a given accesible node,
   probably we should behave as what Element::ScrollIntoView does.

  Accessible::DispatchClickEvent
   Similar to the above, similated various mouse events on a given target node.

  PresShell::EventHandler::PrepareToUseCaretPosition
  PresShell::EventHandler::GetCurrentItemAndPositionForElement
   Both are for context menu, we shouldn't consider scroll-padding and
   scroll-margin.

  nsFormFillController::SetPopupOpen
   This is used for autocompletion popup, we shouldn't consider scroll-padding
   and scroll-margin.

  nsFocusManager::ScrollIntoView
   This is bit unfortunate, we should use scroll-padding and scroll-margin
   depending on call site of this function. Bug 1535232 is for this case.

cssom-view/scrollIntoView-scrollPadding.html which has some tests that is
actually testing scroll-padding with scrollIntoView passes with this change.

The reftest in this change is a test case that the browser navigates to an
element with specifying the anchor to the element.

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

--HG--
extra : moz-landing-system : lando

dom/base/Selection.cpp

@@ -1879,7 +1879,8 @@ nsresult Selection::DoAutoScroll(nsIFrame* aFrame, nsPoint aPoint) {
   while (true) {
     didScroll = presShell->ScrollFrameRectIntoView(
         aFrame, nsRect(aPoint, nsSize(0, 0)), nsIPresShell::ScrollAxis(),
-        nsIPresShell::ScrollAxis(), 0);
+        nsIPresShell::ScrollAxis(),
+        nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING);
     if (!weakFrame || !weakRootFrame) {
       return NS_OK;
     }
@@ -3089,7 +3090,7 @@ nsresult Selection::ScrollIntoView(SelectionRegion aRegion,
   // vertical scrollbar or the scroll range is at least one device pixel)
   aVertical.mOnlyIfPerceivedScrollableDirection = true;
 
-  uint32_t flags = 0;
+  uint32_t flags = nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING;
   if (aFlags & Selection::SCROLL_FIRST_ANCESTOR_ONLY) {
     flags |= nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY;
   }

dom/base/nsFocusManager.cpp

@@ -2125,7 +2125,11 @@ void nsFocusManager::ScrollIntoView(nsIPresShell* aPresShell,
                                  nsIPresShell::SCROLL_IF_NOT_VISIBLE),
         nsIPresShell::ScrollAxis(nsIPresShell::SCROLL_MINIMUM,
                                  nsIPresShell::SCROLL_IF_NOT_VISIBLE),
-        nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
+        nsIPresShell::SCROLL_OVERFLOW_HIDDEN |
+            // FIXME: Bug 1535232: Change the option depending on call sites,
+            // i.e. don't set the option if this function gets called from
+            // Element.focus().
+            nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING);
 }
 
 void nsFocusManager::RaiseWindow(nsPIDOMWindowOuter* aWindow) {

layout/base/PresShell.cpp

@@ -3525,8 +3525,16 @@ bool nsIPresShell::ScrollFrameRectIntoView(nsIFrame* aFrame,
         }
         targetRect.Inflate(padding);
       }
-      ScrollToShowRect(sf, targetRect - sf->GetScrolledFrame()->GetPosition(),
+
-                       aVertical, aHorizontal, aFlags);
+      targetRect -= sf->GetScrolledFrame()->GetPosition();
+      if (!(aFlags & nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING)) {
+        nsMargin scrollPadding = sf->GetScrollPadding();
+        targetRect.Inflate(scrollPadding);
+        targetRect = targetRect.Intersect(sf->GetScrolledRect());
+      }
+
+      ScrollToShowRect(sf, targetRect, aVertical, aHorizontal, aFlags);
+
       nsPoint newPosition = sf->LastScrollDestination();
       // If the scroll position increased, that means our content moved up,
       // so our rect's offset should decrease
@@ -8458,7 +8466,8 @@ bool PresShell::EventHandler::PrepareToUseCaretPosition(
                                  nsIPresShell::SCROLL_IF_NOT_VISIBLE),
         nsIPresShell::ScrollAxis(nsIPresShell::SCROLL_MINIMUM,
                                  nsIPresShell::SCROLL_IF_NOT_VISIBLE),
-        nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
+        nsIPresShell::SCROLL_OVERFLOW_HIDDEN |
+            nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING);
     NS_ENSURE_SUCCESS(rv, false);
     frame = content->GetPrimaryFrame();
     NS_WARNING_ASSERTION(frame, "No frame for focused content?");
@@ -8516,8 +8525,10 @@ void PresShell::EventHandler::GetCurrentItemAndPositionForElement(
     Element* aFocusedElement, nsIContent** aTargetToUse,
     LayoutDeviceIntPoint& aTargetPt, nsIWidget* aRootWidget) {
   nsCOMPtr<nsIContent> focusedContent = aFocusedElement;
-  mPresShell->ScrollContentIntoView(focusedContent, ScrollAxis(), ScrollAxis(),
+  mPresShell->ScrollContentIntoView(
-                                    nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
+      focusedContent, ScrollAxis(), ScrollAxis(),
+      nsIPresShell::SCROLL_OVERFLOW_HIDDEN |
+          nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING);
 
   nsPresContext* presContext = GetPresContext();
 

layout/base/nsIPresShell.h

@@ -779,7 +779,8 @@ class nsIPresShell : public nsStubDocumentObserver {
     SCROLL_NO_PARENT_FRAMES = 0x04,
     SCROLL_SMOOTH = 0x08,
     SCROLL_SMOOTH_AUTO = 0x10,
-    SCROLL_SNAP = 0x20
+    SCROLL_SNAP = 0x20,
+    SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING = 0x40
   };
   /**
    * Scrolls the view of the document so that the given area of a frame
@@ -798,6 +799,10 @@ class nsIPresShell : public nsStubDocumentObserver {
    * If SCROLL_NO_PARENT_FRAMES is set then we only scroll
    * nodes in this document, not in any parent documents which
    * contain this document in a iframe or the like.
+   * If SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING is set we ignore scroll-margin
+   * value specified for |aFrame| and scroll-padding value for the scroll
+   * container. This option is typically used to locate poped-up frames into
+   * view.
    * @return true if any scrolling happened, false if no scrolling happened
    */
   bool ScrollFrameRectIntoView(nsIFrame* aFrame, const nsRect& aRect,

layout/forms/nsListControlFrame.cpp

@@ -1816,7 +1816,8 @@ void nsListControlFrame::ScrollToFrame(dom::HTMLOptionElement& aOptElement) {
         childFrame, nsRect(nsPoint(0, 0), childFrame->GetSize()),
         nsIPresShell::ScrollAxis(), nsIPresShell::ScrollAxis(),
         nsIPresShell::SCROLL_OVERFLOW_HIDDEN |
-            nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY);
+            nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY |
+            nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING);
   }
 }
 

layout/generic/nsGfxScrollFrame.cpp

@@ -6779,6 +6779,23 @@ static nsMargin ResolveScrollPaddingStyle(
                                                  aScrollPortSize));
 }
 
+nsMargin ScrollFrameHelper::GetScrollPadding() const {
+  nsIFrame* styleFrame;
+  if (mIsRoot) {
+    const Element* scrollElement =
+        mOuter->PresContext()->GetViewportScrollStylesOverrideElement();
+    styleFrame = scrollElement ? scrollElement->GetPrimaryFrame() : mOuter;
+  } else {
+    styleFrame = mOuter;
+  }
+  MOZ_ASSERT(styleFrame);
+
+  // The spec says percentage values are relative to the scroll port size.
+  // https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding
+  return ResolveScrollPaddingStyle(styleFrame->StylePadding()->mScrollPadding,
+                                   GetScrollPortRect().Size());
+}
+
 layers::ScrollSnapInfo ScrollFrameHelper::ComputeScrollSnapInfo(
     const Maybe<nsPoint>& aDestination) const {
   ScrollSnapInfo result;
@@ -6812,11 +6829,8 @@ layers::ScrollSnapInfo ScrollFrameHelper::ComputeScrollSnapInfo(
   }
 
   if (StaticPrefs::layout_css_scroll_snap_v1_enabled()) {
-    // The spec says percentage values are relative to the scroll port size.
-    // https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding
     nsRect snapport = GetScrollPortRect();
-    nsMargin scrollPadding = ResolveScrollPaddingStyle(
+    nsMargin scrollPadding = GetScrollPadding();
-        mOuter->StylePadding()->mScrollPadding, snapport.Size());
 
     Maybe<nsRect> snapportOnDestination;
     if (aDestination) {

layout/generic/nsGfxScrollFrame.h

@@ -338,6 +338,8 @@ class ScrollFrameHelper : public nsIReflowCallback {
   bool GetSnapPointForDestination(nsIScrollableFrame::ScrollUnit aUnit,
                                   nsPoint aStartPos, nsPoint& aDestination);
 
+  nsMargin GetScrollPadding() const;
+
   nsSize GetLineScrollAmount() const;
   nsSize GetPageScrollAmount() const;
 
@@ -959,6 +961,9 @@ class nsHTMLScrollFrame : public nsContainerFrame,
   virtual nsSize GetPageScrollAmount() const override {
     return mHelper.GetPageScrollAmount();
   }
+  virtual nsMargin GetScrollPadding() const override {
+    return mHelper.GetScrollPadding();
+  }
   /**
    * @note This method might destroy the frame, pres shell and other objects.
    */
@@ -1442,6 +1447,9 @@ class nsXULScrollFrame final : public nsBoxFrame,
   virtual nsSize GetPageScrollAmount() const override {
     return mHelper.GetPageScrollAmount();
   }
+  virtual nsMargin GetScrollPadding() const override {
+    return mHelper.GetScrollPadding();
+  }
   /**
    * @note This method might destroy the frame, pres shell and other objects.
    */

layout/generic/nsIScrollableFrame.h

@@ -192,6 +192,10 @@ class nsIScrollableFrame : public nsIScrollbarMediator {
    */
   virtual nsSize GetPageScrollAmount() const = 0;
 
+  /**
+   * Return scroll-padding value of this frame.
+   */
+  virtual nsMargin GetScrollPadding() const = 0;
   /**
    * Some platforms (OSX) may generate additional scrolling events even
    * after the user has stopped scrolling, simulating a momentum scrolling

layout/reftests/css-scroll-snap/reftest.list

@@ -0,0 +1,3 @@
+default-preferences pref(layout.css.scroll-snap-v1.enabled,true)
+
+== scroll-padding-on-anchor.html#target scroll-padding-on-anchor-ref.html

layout/reftests/css-scroll-snap/scroll-padding-on-anchor-ref.html

@@ -0,0 +1,16 @@
+<style>
+html,body {
+  overflow: hidden;
+  margin: 0px;
+  padding: 0px;
+}
+body {
+  padding-top: 100px;
+}
+#target {
+  background-color: blue;
+  width: 100%;
+  height: 100px;
+}
+</style>
+<div id="target"></div>

layout/reftests/css-scroll-snap/scroll-padding-on-anchor.html

@@ -0,0 +1,29 @@
+<!--
+ This reftest is a test case that scroll-padding value is propery handled when
+ navigating an anchor node, id="target" in this case, so the content scrolls
+ to the target element on loading.
+-->
+<style>
+html,body {
+  overflow: hidden;
+  scroll-padding: 100px;
+  margin: 0px;
+  padding: 0px;
+}
+.spacer {
+  height: 2000px;
+  width: 100%;
+  padding: 0px;
+  margin: 0px;
+}
+#target {
+  background-color: blue;
+  width: 100%;
+  height: 100px;
+  padding: 0px;
+  margin: 0px;
+}
+</style>
+<div class="spacer"></div>
+<div id="target"></div>
+<div class="spacer"></div>

layout/reftests/reftest.list

@@ -106,6 +106,9 @@ include css-ruby/reftest.list
 # css required
 include css-required/reftest.list
 
+# css scroll snap
+include css-scroll-snap/reftest.list
+
 # css shapes
 include css-shapes/reftest.list
 

layout/xul/nsMenuPopupFrame.cpp

@@ -1808,7 +1808,8 @@ void nsMenuPopupFrame::EnsureMenuItemIsVisible(nsMenuFrame* aMenuItem) {
         aMenuItem, nsRect(nsPoint(0, 0), aMenuItem->GetRect().Size()),
         nsIPresShell::ScrollAxis(), nsIPresShell::ScrollAxis(),
         nsIPresShell::SCROLL_OVERFLOW_HIDDEN |
-            nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY);
+            nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY |
+            nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING);
   }
 }
 

testing/web-platform/meta/css/cssom-view/__dir__.ini

@@ -0,0 +1 @@
+prefs: [layout.css.scroll-snap-v1.enabled:true]

testing/web-platform/meta/css/cssom-view/scrollIntoView-scrollPadding.html.ini

@@ -1,10 +0,0 @@
-[scrollIntoView-scrollPadding.html]
-  [scrollIntoView({block: "center", inline: "center"})]
-    expected: FAIL
-
-  [scrollIntoView({block: "start", inline: "start"})]
-    expected: FAIL
-
-  [scrollIntoView({block: "end", inline: "end"})]
-    expected: FAIL
-

toolkit/components/satchel/nsFormFillController.cpp

@@ -386,7 +386,8 @@ nsFormFillController::SetPopupOpen(bool aPopupOpen) {
                                    nsIPresShell::SCROLL_IF_NOT_VISIBLE),
           nsIPresShell::ScrollAxis(nsIPresShell::SCROLL_MINIMUM,
                                    nsIPresShell::SCROLL_IF_NOT_VISIBLE),
-          nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
+          nsIPresShell::SCROLL_OVERFLOW_HIDDEN |
+              nsIPresShell::SCROLL_IGNORE_SCROLL_MARGIN_AND_PADDING);
       // mFocusedPopup can be destroyed after ScrollContentIntoView, see bug
       // 420089
       if (mFocusedPopup) {