Bug 1732179: Account for cross-process offset properly in bounds calculation, r=morgan

This revision slightly reworks the way that we calculate accessible bounds in
RemoteAccessibleBase::BoundsWithOffset. Rather than looking "below" the current
accessible for a doc accessible to determine whether we should apply a cached
cross-process offset, we instead look "above" the current accessible, to its
parent, to check for a cross-process offset to apply. Then, in ApplyTransform,
we ensure that we maintain that cross-process offset.

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

accessible/ipc/DocAccessibleParent.h

@@ -61,6 +61,14 @@ class DocAccessibleParent : public RemoteAccessible,
   void SetTopLevelInContentProcess() { mTopLevelInContentProcess = true; }
   bool IsTopLevelInContentProcess() const { return mTopLevelInContentProcess; }
 
+  /**
+   * Determine whether this is an out-of-process iframe document, embedded by a
+   * remote embedder document.
+   */
+  bool IsOOPIframeDoc() const {
+    return !mTopLevel && mTopLevelInContentProcess;
+  }
+
   bool IsShutdown() const { return mShutdown; }
 
   /**

accessible/ipc/RemoteAccessibleBase.cpp

@@ -441,8 +441,20 @@ Maybe<nsRect> RemoteAccessibleBase<Derived>::RetrieveCachedBounds() const {
 template <class Derived>
 void RemoteAccessibleBase<Derived>::ApplyCrossProcOffset(
     nsRect& aBounds) const {
+  Accessible* parentAcc = Parent();
+  if (!parentAcc || !parentAcc->IsRemote() || !parentAcc->IsOuterDoc()) {
+    return;
+  }
+
+  if (!IsDoc() || !AsDoc()->IsOOPIframeDoc()) {
+    // We should only apply cross-proc offsets to OOP iframe documents. If we're
+    // anything else, return early here.
+    return;
+  }
+
   Maybe<const nsTArray<int32_t>&> maybeOffset =
-      mCachedFields->GetAttribute<nsTArray<int32_t>>(nsGkAtoms::crossorigin);
+      parentAcc->AsRemote()->mCachedFields->GetAttribute<nsTArray<int32_t>>(
+          nsGkAtoms::crossorigin);
   if (!maybeOffset) {
     return;
   }
@@ -463,12 +475,38 @@ bool RemoteAccessibleBase<Derived>::ApplyTransform(nsRect& aBounds) const {
   if (!maybeTransform) {
     return false;
   }
-  // The transform matrix we cache is meant to operate on rects
-  // within the coordinate space of the frame to which the
-  // transform is applied (self-relative rects). We cache bounds
-  // relative to some ancestor. Remove the relative offset before
-  // transforming. The transform matrix will add it back in.
-  aBounds.MoveTo(0, 0);
+
+  // Layout does not include translations in transform matricies for iframes.
+  // Because of that, we avoid making aBounds self-relative before transforming
+  // when working with a transform on an iframe. This is true for both in- and
+  // out-of-process iframes.
+  bool isIframe = false;
+  if (IsRemote() && IsOuterDoc()) {
+    Accessible* firstChild = FirstChild();
+    if (firstChild && firstChild->IsRemote() && firstChild->IsDoc()) {
+      const RemoteAccessible* firstChildRemote = firstChild->AsRemote();
+      if (!firstChildRemote->AsDoc()->IsTopLevel()) {
+        isIframe = true;
+      }
+    }
+  }
+
+  if (isIframe) {
+    // We want to maintain the effect of the cross-process offset on the
+    // bounds, but otherwise make the rect self-relative. To accomplish that,
+    // remove the influence of the cached bounds, if any.
+    if (Maybe<nsRect> maybeBounds = RetrieveCachedBounds()) {
+      aBounds.MoveBy(-maybeBounds.value().TopLeft());
+    }
+  } else {
+    // The transform matrix we cache is meant to operate on rects
+    // within the coordinate space of the frame to which the
+    // transform is applied (self-relative rects). We cache bounds
+    // relative to some ancestor. Remove the relative offset before
+    // transforming. The transform matrix will add it back in.
+    aBounds.MoveTo(0, 0);
+  }
+
   auto mtxInPixels = gfx::Matrix4x4Typed<CSSPixel, CSSPixel>::FromUnknownMatrix(
       *(*maybeTransform));
 
@@ -532,11 +570,12 @@ LayoutDeviceIntRect RemoteAccessibleBase<Derived>::BoundsWithOffset(
       bounds.SetRectY(bounds.y + internalRect.y, internalRect.height);
     }
 
+    ApplyCrossProcOffset(bounds);
+
     Unused << ApplyTransform(bounds);
 
     LayoutDeviceIntRect devPxBounds;
     const Accessible* acc = Parent();
-    const RemoteAccessibleBase<Derived>* recentAcc = this;
     while (acc && acc->IsRemote()) {
       RemoteAccessible* remoteAcc = const_cast<Accessible*>(acc)->AsRemote();
 
@@ -561,16 +600,12 @@ LayoutDeviceIntRect RemoteAccessibleBase<Derived>::BoundsWithOffset(
           topDoc = remoteAcc->AsDoc();
         }
 
-        if (remoteAcc->IsOuterDoc()) {
-          if (recentAcc && recentAcc->IsDoc() &&
-              !recentAcc->AsDoc()->IsTopLevel() &&
-              recentAcc->AsDoc()->IsTopLevelInContentProcess()) {
-            // We're unable to account for the document offset of remote,
-            // cross process iframes when computing parent-relative bounds.
-            // Instead we store this value separately and apply it here.
-            remoteAcc->ApplyCrossProcOffset(remoteBounds);
-          }
-        }
+        // We're unable to account for the document offset of remote,
+        // cross process iframes when computing parent-relative bounds.
+        // Instead we store this value separately and apply it here. This
+        // offset is cached on both in - and OOP iframes, but is only applied
+        // to OOP iframes.
+        remoteAcc->ApplyCrossProcOffset(remoteBounds);
 
         // Apply scroll offset, if applicable. Only the contents of an
         // element are affected by its scroll offset, which is why this call
@@ -584,7 +619,6 @@ LayoutDeviceIntRect RemoteAccessibleBase<Derived>::BoundsWithOffset(
         bounds.MoveBy(remoteBounds.X(), remoteBounds.Y());
         Unused << remoteAcc->ApplyTransform(bounds);
       }
-      recentAcc = remoteAcc;
       acc = acc->Parent();
     }
 

accessible/tests/browser/bounds/browser.ini

@@ -18,3 +18,4 @@ skip-if = os == 'win' # bug 1372296
 [browser_zero_area.js]
 [browser_test_display_contents.js]
 [browser_test_simple_transform.js]
+[browser_test_iframe_transform.js]

accessible/tests/browser/bounds/browser_test_iframe_transform.js

@@ -0,0 +1,142 @@
+/* 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";
+
+const TRANSLATION_OFFSET = 50;
+const ELEM_ID = "test-elem-id";
+
+// Modify the style of an iframe within the content process. This is different
+// from, e.g., invokeSetStyle, because this function doesn't rely on
+// invokeContentTask, which runs in the context of the iframe itself.
+async function invokeSetStyleIframe(browser, id, style, value) {
+  if (value) {
+    Logger.log(`Setting ${style} style to ${value} for iframe with id: ${id}`);
+  } else {
+    Logger.log(`Removing ${style} style from iframe with id: ${id}`);
+  }
+
+  // Translate the iframe itself (not content within it).
+  await SpecialPowers.spawn(
+    browser,
+    [id, style, value],
+    (iframeId, iframeStyle, iframeValue) => {
+      const elm = content.document.getElementById(iframeId);
+      if (iframeValue) {
+        elm.style[iframeStyle] = iframeValue;
+      } else {
+        delete elm.style[iframeStyle];
+      }
+    }
+  );
+}
+
+// Test the accessible's bounds, comparing them to the content bounds from DOM.
+// This function also accepts an offset, which is necessary in some cases where
+// DOM doesn't know about cross-process offsets.
+function testBoundsWithOffset(browser, iframeDocAcc, id, domElmBounds, offset) {
+  // Get the bounds as reported by the accessible.
+  const acc = findAccessibleChildByID(iframeDocAcc, id);
+  const accX = {};
+  const accY = {};
+  const accWidth = {};
+  const accHeight = {};
+  acc.getBounds(accX, accY, accWidth, accHeight);
+
+  // getContentBoundsForDOMElm's result doesn't include iframe translation
+  // for in-process iframes, but does for out-of-process iframes. To account
+  // for that here, manually add in the translation offset when examining an
+  // in-process iframe. This manual adjustment isn't necessary without the cache
+  // since, without it, accessible bounds don't include the translation offset either.
+  const addTranslationOffset = !gIsRemoteIframe && isCacheEnabled;
+  const expectedX = addTranslationOffset
+    ? domElmBounds[0] + offset
+    : domElmBounds[0];
+  const expectedY = addTranslationOffset
+    ? domElmBounds[1] + offset
+    : domElmBounds[1];
+  const expectedWidth = domElmBounds[2];
+  const expectedHeight = domElmBounds[3];
+
+  let boundsAreEquivalent = true;
+  boundsAreEquivalent &&= accX.value == expectedX;
+  boundsAreEquivalent &&= accY.value == expectedY;
+  boundsAreEquivalent &&= accWidth.value == expectedWidth;
+  boundsAreEquivalent &&= accHeight.value == expectedHeight;
+  return boundsAreEquivalent;
+}
+
+addAccessibleTask(
+  `<div id='${ELEM_ID}'>hello world</div>`,
+  async function(browser, iframeDocAcc, contentDocAcc) {
+    ok(iframeDocAcc, "IFRAME document accessible is present");
+
+    await testBoundsInContent(iframeDocAcc, ELEM_ID, browser);
+
+    // Translate the iframe, which should modify cross-process offset.
+    await invokeSetStyleIframe(
+      browser,
+      DEFAULT_IFRAME_ID,
+      "transform",
+      `translate(${TRANSLATION_OFFSET}px, ${TRANSLATION_OFFSET}px)`
+    );
+
+    // Allow content to advance to update DOM, then capture the DOM bounds.
+    await waitForContentPaint(browser);
+    const domElmBoundsAfterTranslate = await getContentBoundsForDOMElm(
+      browser,
+      ELEM_ID
+    );
+
+    // Ensure that there's enough time for the cache to update.
+    await untilCacheOk(() => {
+      return testBoundsWithOffset(
+        browser,
+        iframeDocAcc,
+        ELEM_ID,
+        domElmBoundsAfterTranslate,
+        TRANSLATION_OFFSET
+      );
+    }, "Accessible bounds have changed in the cache and match DOM bounds.");
+
+    // XXX Enable this test after resolution of bug 1792120. It will not
+    // succeed as-is due to stale relative-bounds in the cache.
+    /*
+    // Adjust padding of the iframe, then verify bounds adjust properly.
+    // iframes already have a border by default, so we check padding here.
+    const PADDING_OFFSET = 100;
+    await invokeSetStyleIframe(
+      browser,
+      DEFAULT_IFRAME_ID,
+      "padding",
+      `${PADDING_OFFSET}px`
+    );
+
+    // Allow content to advance to update DOM, then capture the DOM bounds.
+    await waitForContentPaint(browser);
+    const domElmBoundsAfterAddingPadding = await getContentBoundsForDOMElm(
+      browser,
+      ELEM_ID
+    );
+
+    await untilCacheOk(() => {
+      return testBoundsWithOffset(
+        browser,
+        iframeDocAcc,
+        ELEM_ID,
+        domElmBoundsAfterAddingPadding,
+        TRANSLATION_OFFSET
+      );
+    }, "Accessible bounds have changed in the cache and match DOM bounds.");
+    */
+  },
+  {
+    topLevel: false,
+    iframe: true,
+    remoteIframe: true,
+    iframeAttrs: {
+      style: `height: 100px; width: 100px;`,
+    },
+  }
+);