Bug 1756229: Cache scroll position r=Jamie,emilio

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

accessible/base/CacheConstants.h

@@ -23,6 +23,7 @@ class CacheDomain {
   static constexpr uint64_t Actions = ((uint64_t)0x1) << 8;
   static constexpr uint64_t Style = ((uint64_t)0x1) << 9;
   static constexpr uint64_t TransformMatrix = ((uint64_t)0x1) << 10;
+  static constexpr uint64_t ScrollPosition = ((uint64_t)0x1) << 11;
   static constexpr uint64_t All = ~((uint64_t)0x0);
 };
 

accessible/generic/DocAccessible.cpp

@@ -449,6 +449,10 @@ void DocAccessible::Shutdown() {
   }
 
   mChildDocuments.Clear();
+  // mQueuedCacheUpdates can contain a reference to this document (ex. if the
+  // doc is scrollable and we're sending a scroll position update). Clear the
+  // map here to avoid creating ref cycles.
+  mQueuedCacheUpdates.Clear();
 
   // XXX thinking about ordering?
   if (mIPCDoc) {
@@ -616,6 +620,13 @@ void DocAccessible::ScrollTimerCallback(nsITimer* aTimer, void* aClosure) {
 }
 
 void DocAccessible::HandleScroll(nsINode* aTarget) {
+  // Regardless of our scroll timer, we need to send a cache update
+  // to ensure the next Bounds() query accurately reflects our position
+  // after scrolling.
+  if (LocalAccessible* scrollTarget = GetAccessible(aTarget)) {
+    QueueCacheUpdate(scrollTarget, CacheDomain::ScrollPosition);
+  }
+
   const uint32_t kScrollEventInterval = 100;
   // If we haven't dispatched a scrolling event for a target in at least
   // kScrollEventInterval milliseconds, dispatch one now.
@@ -648,6 +659,28 @@ void DocAccessible::HandleScroll(nsINode* aTarget) {
   }
 }
 
+std::pair<nsPoint, nsRect> DocAccessible::ComputeScrollData(
+    LocalAccessible* aAcc) {
+  nsPoint scrollPoint;
+  nsRect scrollRange;
+  nsIFrame* frame = aAcc->GetFrame();
+  nsIScrollableFrame* sf = aAcc == this
+                               ? mPresShell->GetRootScrollFrameAsScrollable()
+                               : frame->GetScrollTargetFrame();
+
+  // If there is no scrollable frame, it's likely a scroll in a popup, like
+  // <select>. Return a scroll offset and range of 0. The scroll info
+  // is currently only used on Android, and popups are rendered natively
+  // there.
+  if (sf) {
+    scrollPoint = sf->GetScrollPosition() * mPresShell->GetResolution();
+    scrollRange = sf->GetScrollRange();
+    scrollRange.ScaleRoundOut(mPresShell->GetResolution());
+  }
+
+  return {scrollPoint, scrollRange};
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // nsIObserver
 
@@ -2541,31 +2574,19 @@ void DocAccessible::DispatchScrollingEvent(nsINode* aTarget,
     return;
   }
 
-  LayoutDevicePoint scrollPoint;
-  LayoutDeviceRect scrollRange;
-  nsIScrollableFrame* sf = acc == this
-                               ? mPresShell->GetRootScrollFrameAsScrollable()
-                               : frame->GetScrollTargetFrame();
+  auto [scrollPoint, scrollRange] = ComputeScrollData(acc);
 
-  // If there is no scrollable frame, it's likely a scroll in a popup, like
-  // <select>. Just send an event with no scroll info. The scroll info
-  // is currently only used on Android, and popups are rendered natively
-  // there.
-  if (sf) {
-    int32_t appUnitsPerDevPixel =
-        mPresShell->GetPresContext()->AppUnitsPerDevPixel();
-    scrollPoint = LayoutDevicePoint::FromAppUnits(sf->GetScrollPosition(),
-                                                  appUnitsPerDevPixel) *
-                  mPresShell->GetResolution();
+  int32_t appUnitsPerDevPixel =
+      mPresShell->GetPresContext()->AppUnitsPerDevPixel();
 
-    scrollRange = LayoutDeviceRect::FromAppUnits(sf->GetScrollRange(),
-                                                 appUnitsPerDevPixel);
-    scrollRange.ScaleRoundOut(mPresShell->GetResolution());
-  }
+  LayoutDeviceIntPoint scrollPointDP = LayoutDevicePoint::FromAppUnitsToNearest(
+      scrollPoint, appUnitsPerDevPixel);
+  LayoutDeviceIntRect scrollRangeDP =
+      LayoutDeviceRect::FromAppUnitsToNearest(scrollRange, appUnitsPerDevPixel);
 
   RefPtr<AccEvent> event =
-      new AccScrollingEvent(aEventType, acc, scrollPoint.x, scrollPoint.y,
-                            scrollRange.width, scrollRange.height);
+      new AccScrollingEvent(aEventType, acc, scrollPointDP.x, scrollPointDP.y,
+                            scrollRangeDP.width, scrollRangeDP.height);
   nsEventShell::FireEvent(event);
 }
 

accessible/generic/DocAccessible.h

@@ -387,10 +387,19 @@ class DocAccessible : public HyperTextAccessibleWrap,
   /**
    * Notify the document that a DOM node has been scrolled. document will
    * dispatch throttled accessibility events for scrolling, and a scroll-end
-   * event.
+   * event. This function also queues a cache update for ScrollPosition.
    */
   void HandleScroll(nsINode* aTarget);
 
+  /**
+   * Retrieves the scroll frame (if it exists) for the given accessible
+   * and returns its scroll position and scroll range. If the given
+   * accessible is `this`, return the scroll position and range of
+   * the root scroll frame. Return values have been scaled by the
+   * PresShell's resolution.
+   */
+  std::pair<nsPoint, nsRect> ComputeScrollData(LocalAccessible* aAcc);
+
  protected:
   virtual ~DocAccessible();
 
@@ -761,11 +770,11 @@ class DocAccessible : public HyperTextAccessibleWrap,
   // Exclusively owned by IPDL so don't manually delete it!
   DocAccessibleChild* mIPCDoc;
 
-  nsTHashSet<RefPtr<LocalAccessible>> mMaybeBoundsChanged;
-
   // A hash map between LocalAccessibles and CacheDomains, tracking
   // cache updates that have been queued during the current tick
-  // but not yet sent.
+  // but not yet sent. It is possible for this map to contain a reference
+  // to the document it lives on. We clear the list in Shutdown() to
+  // avoid cyclical references.
   nsTHashMap<RefPtr<LocalAccessible>, uint64_t> mQueuedCacheUpdates;
 
   // A set of Accessibles moved during this tick. Only used in content

accessible/generic/LocalAccessible.cpp

@@ -3263,6 +3263,18 @@ already_AddRefed<AccAttributes> LocalAccessible::BundleFieldsForCache(
     }
   }
 
+  if (aCacheDomain & CacheDomain::ScrollPosition) {
+    if (nsIFrame* frame = GetFrame()) {
+      nsPoint scrollPosition;
+      std::tie(scrollPosition, std::ignore) = mDoc->ComputeScrollData(this);
+
+      nsTArray<int32_t> positionArr(2);
+      positionArr.AppendElement(scrollPosition.x);
+      positionArr.AppendElement(scrollPosition.y);
+      fields->SetAttribute(nsGkAtoms::scrollPosition, std::move(positionArr));
+    }
+  }
+
   if (aCacheDomain & CacheDomain::DOMNodeID && mContent) {
     nsAtom* id = mContent->GetID();
     if (id) {

accessible/ipc/RemoteAccessibleBase.cpp

@@ -341,6 +341,26 @@ bool RemoteAccessibleBase<Derived>::ApplyTransform(nsRect& aBounds) const {
   return true;
 }
 
+template <class Derived>
+void RemoteAccessibleBase<Derived>::ApplyScrollOffset(nsRect& aBounds) const {
+  Maybe<const nsTArray<int32_t>&> maybeScrollPosition =
+      mCachedFields->GetAttribute<nsTArray<int32_t>>(nsGkAtoms::scrollPosition);
+
+  if (!maybeScrollPosition || maybeScrollPosition->Length() != 2) {
+    return;
+  }
+  // Our retrieved value is in app units, so we don't need to do any
+  // unit conversion here.
+  const nsTArray<int32_t>& scrollPosition = *maybeScrollPosition;
+
+  // Scroll position is an inverse representation of scroll offset (since the
+  // further the scroll bar moves down the page, the further the page content
+  // moves up/closer to the origin).
+  nsPoint scrollOffset(-scrollPosition[0], -scrollPosition[1]);
+
+  aBounds.MoveBy(scrollOffset.x, scrollOffset.y);
+}
+
 template <class Derived>
 LayoutDeviceIntRect RemoteAccessibleBase<Derived>::Bounds() const {
   if (mCachedFields) {
@@ -358,7 +378,7 @@ LayoutDeviceIntRect RemoteAccessibleBase<Derived>::Bounds() const {
       Unused << ApplyTransform(bounds);
 
       LayoutDeviceIntRect devPxBounds;
-      const Accessible* acc = this;
+      const Accessible* acc = Parent();
 
       while (acc) {
         if (LocalAccessible* localAcc =
@@ -384,12 +404,9 @@ LayoutDeviceIntRect RemoteAccessibleBase<Derived>::Bounds() const {
         }
 
         RemoteAccessible* remoteAcc = const_cast<Accessible*>(acc)->AsRemote();
-        // Verify that remoteAcc is not `this`, since `bounds` was
-        // initialised to include this->RetrieveCachedBounds()
-        Maybe<nsRect> maybeRemoteBounds =
-            (remoteAcc == this) ? Nothing() : remoteAcc->RetrieveCachedBounds();
 
-        if (maybeRemoteBounds) {
+        if (Maybe<nsRect> maybeRemoteBounds =
+                remoteAcc->RetrieveCachedBounds()) {
           nsRect remoteBounds = *maybeRemoteBounds;
           // We need to take into account a non-1 resolution set on the
           // presshell. This happens with async pinch zooming, among other
@@ -409,8 +426,15 @@ LayoutDeviceIntRect RemoteAccessibleBase<Derived>::Bounds() const {
             bounds.ScaleRoundOut(res.valueOr(1.0f));
           }
 
-          // We should offset `bounds` by the bounds retrieved above.
-          // This is how we build screen coordinates from relative coordinates.
+          // Apply scroll offset, if applicable. Only the contents of an
+          // element are affected by its scroll offset, which is why this call
+          // happens in this loop instead of both inside and outside of
+          // the loop (like ApplyTransform).
+          remoteAcc->ApplyScrollOffset(remoteBounds);
+
+          // Regardless of whether this is a doc, we should offset `bounds`
+          // by the bounds retrieved here. This is how we build screen
+          // coordinates from relative coordinates.
           bounds.MoveBy(remoteBounds.X(), remoteBounds.Y());
           Unused << remoteAcc->ApplyTransform(bounds);
         }

accessible/ipc/RemoteAccessibleBase.h

@@ -323,6 +323,7 @@ class RemoteAccessibleBase : public Accessible, public HyperTextAccessibleBase {
   void SetParent(Derived* aParent);
   Maybe<nsRect> RetrieveCachedBounds() const;
   bool ApplyTransform(nsRect& aBounds) const;
+  void ApplyScrollOffset(nsRect& aBounds) const;
 
   virtual void ARIAGroupPosition(int32_t* aLevel, int32_t* aSetSize,
                                  int32_t* aPosInSet) const override;

accessible/tests/browser/bounds/browser_test_simple_transform.js

@@ -4,71 +4,20 @@
 
 "use strict";
 
-async function finishContentPaint(browser) {
-  await SpecialPowers.spawn(browser, [], () => {
-    return new Promise(function(r) {
-      content.requestAnimationFrame(() => content.setTimeout(r));
-    });
-  });
-}
-
-async function testContentBoundsInIframe(iframeDocAcc, id, browser) {
-  const acc = findAccessibleChildByID(iframeDocAcc, id);
-  const x = {};
-  const y = {};
-  const width = {};
-  const height = {};
-  acc.getBounds(x, y, width, height);
-
-  await invokeContentTask(
-    browser,
-    [id, x.value, y.value, width.value, height.value],
-    (_id, _x, _y, _width, _height) => {
-      const { Layout: LayoutUtils } = ChromeUtils.import(
-        "chrome://mochitests/content/browser/accessible/tests/browser/Layout.jsm"
-      );
-
-      let [
-        expectedX,
-        expectedY,
-        expectedWidth,
-        expectedHeight,
-      ] = LayoutUtils.getBoundsForDOMElm(_id, content.document);
-
-      ok(
-        _x >= expectedX - 5 || _x <= expectedX + 5,
-        "Got " + _x + ", accurate x for " + _id
-      );
-      ok(
-        _y >= expectedY - 5 || _y <= expectedY + 5,
-        "Got " + _y + ", accurate y for " + _id
-      );
-      ok(
-        _width >= expectedWidth - 5 || _width <= expectedWidth + 5,
-        "Got " + _width + ", accurate width for " + _id
-      );
-      ok(
-        _height >= expectedHeight - 5 || _height <= expectedHeight + 5,
-        "Got " + _height + ", accurate height for " + _id
-      );
-    }
-  );
-}
-
 // test basic translation
 addAccessibleTask(
   `<p id="translate">hello world</p>`,
   async function(browser, iframeDocAcc, contentDocAcc) {
     ok(iframeDocAcc, "IFRAME document accessible is present");
-    await testContentBoundsInIframe(iframeDocAcc, "translate", browser);
+    await testBoundsInContent(iframeDocAcc, "translate", browser);
 
     await invokeContentTask(browser, [], () => {
       let p = content.document.getElementById("translate");
       p.style = "transform: translate(100px, 100px);";
     });
 
-    await finishContentPaint(browser);
-    await testContentBoundsInIframe(iframeDocAcc, "translate", browser);
+    await waitForContentPaint(browser);
+    await testBoundsInContent(iframeDocAcc, "translate", browser);
   },
   { topLevel: true, iframe: true, remoteIframe: true }
 );
@@ -78,15 +27,15 @@ addAccessibleTask(
   `<p id="rotate">hello world</p>`,
   async function(browser, iframeDocAcc, contentDocAcc) {
     ok(iframeDocAcc, "IFRAME document accessible is present");
-    await testContentBoundsInIframe(iframeDocAcc, "rotate", browser);
+    await testBoundsInContent(iframeDocAcc, "rotate", browser);
 
     await invokeContentTask(browser, [], () => {
       let p = content.document.getElementById("rotate");
       p.style = "transform: rotate(-40deg);";
     });
 
-    await finishContentPaint(browser);
-    await testContentBoundsInIframe(iframeDocAcc, "rotate", browser);
+    await waitForContentPaint(browser);
+    await testBoundsInContent(iframeDocAcc, "rotate", browser);
   },
   { topLevel: true, iframe: true, remoteIframe: true }
 );
@@ -96,15 +45,15 @@ addAccessibleTask(
   `<p id="scale">hello world</p>`,
   async function(browser, iframeDocAcc, contentDocAcc) {
     ok(iframeDocAcc, "IFRAME document accessible is present");
-    await testContentBoundsInIframe(iframeDocAcc, "scale", browser);
+    await testBoundsInContent(iframeDocAcc, "scale", browser);
 
     await invokeContentTask(browser, [], () => {
       let p = content.document.getElementById("scale");
       p.style = "transform: scale(2);";
     });
 
-    await finishContentPaint(browser);
-    await testContentBoundsInIframe(iframeDocAcc, "scale", browser);
+    await waitForContentPaint(browser);
+    await testBoundsInContent(iframeDocAcc, "scale", browser);
   },
   { topLevel: true, iframe: true, remoteIframe: true }
 );

accessible/tests/browser/scroll/browser.ini

@@ -8,3 +8,4 @@ support-files =
 
 [browser_test_zoom_text.js]
 skip-if = e10s && os == 'win' # bug 1372296
+[browser_test_scroll_bounds.js]

accessible/tests/browser/scroll/browser_test_scroll_bounds.js

@@ -0,0 +1,47 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+/* import-globals-from ../../mochitest/layout.js */
+loadScripts({ name: "layout.js", dir: MOCHITESTS_DIR });
+
+async function runTests(browser, docAcc) {
+  ok(docAcc, "iframe document acc is present");
+  await testBoundsInContent(docAcc, "square", browser);
+  await testBoundsInContent(docAcc, "rect", browser);
+
+  await invokeContentTask(browser, [], () => {
+    content.document.getElementById("square").scrollIntoView();
+  });
+
+  await waitForContentPaint(browser);
+
+  await testBoundsInContent(docAcc, "square", browser);
+  await testBoundsInContent(docAcc, "rect", browser);
+
+  await invokeContentTask(browser, [], () => {
+    content.document.getElementById("rect").scrollIntoView();
+  });
+
+  await waitForContentPaint(browser);
+
+  await testBoundsInContent(docAcc, "square", browser);
+  await testBoundsInContent(docAcc, "rect", browser);
+}
+
+/**
+ * Test bounds of accessibles after scrolling
+ */
+addAccessibleTask(
+  `
+  <div id='square' style='height:100px; width:100px; background:green; margin-top:3000px; margin-bottom:4000px;'>
+  </div>
+
+  <div id='rect' style='height:40px; width:200px; background:blue; margin-bottom:3400px'>
+  </div>
+  `,
+  runTests,
+  { iframe: true, remoteIframe: true, chrome: true }
+);

accessible/tests/browser/scroll/browser_test_zoom_text.js

@@ -7,14 +7,6 @@
 /* import-globals-from ../../mochitest/layout.js */
 loadScripts({ name: "layout.js", dir: MOCHITESTS_DIR });
 
-async function waitForContentPaint(browser) {
-  await SpecialPowers.spawn(browser, [], () => {
-    return new Promise(function(r) {
-      content.requestAnimationFrame(() => content.setTimeout(r));
-    });
-  });
-}
-
 async function runTests(browser, accDoc) {
   await loadContentScripts(browser, "Layout.jsm");
 

accessible/tests/browser/shared-head.js

@@ -5,6 +5,7 @@
 "use strict";
 
 /* import-globals-from ../mochitest/common.js */
+/* import-globals-from ../mochitest/layout.js */
 /* import-globals-from ../mochitest/promisified-events.js */
 
 /* exported Logger, MOCHITESTS_DIR, invokeSetAttribute, invokeFocus,
@@ -14,7 +15,7 @@
             Cc, Cu, arrayFromChildren, forceGC, contentSpawnMutation,
             DEFAULT_IFRAME_ID, DEFAULT_IFRAME_DOC_BODY_ID, invokeContentTask,
             matchContentDoc, currentContentDoc, getContentDPR,
-            waitForImageMap, getContentBoundsForDOMElm, untilCacheIs, untilCacheOk */
+            waitForImageMap, getContentBoundsForDOMElm, untilCacheIs, untilCacheOk, testBoundsInContent, waitForContentPaint */
 
 const CURRENT_FILE_DIR = "/browser/accessible/tests/browser/";
 
@@ -860,3 +861,54 @@ function untilCacheIs(retrievalFunc, expected, message) {
     () => [retrievalFunc(), expected, message]
   ).then(([got, exp, msg]) => is(got, exp, msg));
 }
+
+async function waitForContentPaint(browser) {
+  await SpecialPowers.spawn(browser, [], () => {
+    return new Promise(function(r) {
+      content.requestAnimationFrame(() => content.setTimeout(r));
+    });
+  });
+}
+
+async function testBoundsInContent(iframeDocAcc, id, browser) {
+  const acc = findAccessibleChildByID(iframeDocAcc, id);
+  const x = {};
+  const y = {};
+  const width = {};
+  const height = {};
+  acc.getBounds(x, y, width, height);
+
+  await invokeContentTask(
+    browser,
+    [id, x.value, y.value, width.value, height.value],
+    (_id, _x, _y, _width, _height) => {
+      const { Layout: LayoutUtils } = ChromeUtils.import(
+        "chrome://mochitests/content/browser/accessible/tests/browser/Layout.jsm"
+      );
+
+      let [
+        expectedX,
+        expectedY,
+        expectedWidth,
+        expectedHeight,
+      ] = LayoutUtils.getBoundsForDOMElm(_id, content.document);
+
+      ok(
+        _x >= expectedX - 5 || _x <= expectedX + 5,
+        "Got " + _x + ", accurate x for " + _id
+      );
+      ok(
+        _y >= expectedY - 5 || _y <= expectedY + 5,
+        "Got " + _y + ", accurate y for " + _id
+      );
+      ok(
+        _width >= expectedWidth - 5 || _width <= expectedWidth + 5,
+        "Got " + _width + ", accurate width for " + _id
+      );
+      ok(
+        _height >= expectedHeight - 5 || _height <= expectedHeight + 5,
+        "Got " + _height + ", accurate height for " + _id
+      );
+    }
+  );
+}