commit 2809fa98ae70d6e606535fc187a99c7e7e5e1941
parent de22534abec8ebcad2eb72dce464c4450d08f4bf
Author: Morgan Rae Reschenberg <mreschenberg@berkeley.edu>
Date: Thu, 23 Oct 2025 20:40:28 +0000
Bug 1991817: Avoid scaling scroll offset in content due to potential for async updates from APZ r=eeejay
Differential Revision: https://phabricator.services.mozilla.com/D269674
Diffstat:
5 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
@@ -775,7 +775,7 @@ void DocAccessible::HandleScroll(nsINode* aTarget) {
}
std::pair<nsPoint, nsRect> DocAccessible::ComputeScrollData(
- const LocalAccessible* aAcc) {
+ const LocalAccessible* aAcc, bool aShouldScaleByResolution) {
nsPoint scrollPoint;
nsRect scrollRange;
@@ -789,9 +789,13 @@ std::pair<nsPoint, nsRect> DocAccessible::ComputeScrollData(
// is currently only used on Android, and popups are rendered natively
// there.
if (sf) {
- scrollPoint = sf->GetScrollPosition() * mPresShell->GetResolution();
+ scrollPoint = sf->GetScrollPosition();
scrollRange = sf->GetScrollRange();
- scrollRange.ScaleRoundOut(mPresShell->GetResolution());
+
+ if (aShouldScaleByResolution) {
+ scrollPoint = scrollPoint * mPresShell->GetResolution();
+ scrollRange.ScaleRoundOut(mPresShell->GetResolution());
+ }
}
}
diff --git a/accessible/generic/DocAccessible.h b/accessible/generic/DocAccessible.h
@@ -401,9 +401,11 @@ class DocAccessible : public HyperTextAccessible,
* 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.
+ * PresShell's resolution when aShouldScaleByResolution is explicitly
+ * true or unspecified.
*/
- std::pair<nsPoint, nsRect> ComputeScrollData(const LocalAccessible* aAcc);
+ std::pair<nsPoint, nsRect> ComputeScrollData(
+ const LocalAccessible* aAcc, bool aShouldScaleByResolution = true);
/**
* Only works in content process documents.
diff --git a/accessible/generic/LocalAccessible.cpp b/accessible/generic/LocalAccessible.cpp
@@ -3831,7 +3831,17 @@ already_AddRefed<AccAttributes> LocalAccessible::BundleFieldsForCache(
}
if (aCacheDomain & CacheDomain::ScrollPosition && frame) {
- const auto [scrollPosition, scrollRange] = mDoc->ComputeScrollData(this);
+ // We request these values unscaled when caching because the scaled values
+ // have resolution multiplied in. We can encounter a race condition when
+ // using APZ where the resolution is not propogated back to content in time
+ // for it to be multipled into the scroll position calculation. Even if we
+ // end up with the correct resolution cached in parent, our final bounds
+ // will be incorrect. Instead of scaling here, we scale in parent with our
+ // cached resolution so any incorrectness will be consistent and dependent
+ // on a single cache update (Resolution) instead of two (Resolution and
+ // ScrollPosition).
+ const auto [scrollPosition, scrollRange] =
+ mDoc->ComputeScrollData(this, /* aShouldScaleByResolution */ false);
if (scrollRange.width || scrollRange.height) {
// If the scroll range is 0 by 0, this acc is not scrollable. We
// can't simply check scrollPosition != 0, since it's valid for scrollable
diff --git a/accessible/ipc/RemoteAccessible.cpp b/accessible/ipc/RemoteAccessible.cpp
@@ -776,7 +776,8 @@ bool RemoteAccessible::ApplyTransform(nsRect& aCumulativeBounds) const {
return true;
}
-bool RemoteAccessible::ApplyScrollOffset(nsRect& aBounds) const {
+bool RemoteAccessible::ApplyScrollOffset(nsRect& aBounds,
+ float aResolution) const {
ASSERT_DOMAINS_ACTIVE(CacheDomain::ScrollPosition);
Maybe<const nsTArray<int32_t>&> maybeScrollPosition =
mCachedFields->GetAttribute<nsTArray<int32_t>>(CacheKey::ScrollPosition);
@@ -793,7 +794,8 @@ bool RemoteAccessible::ApplyScrollOffset(nsRect& aBounds) const {
// moves up/closer to the origin).
nsPoint scrollOffset(-scrollPosition[0], -scrollPosition[1]);
- aBounds.MoveBy(scrollOffset.x, scrollOffset.y);
+ aBounds.MoveBy(scrollOffset.x * aResolution * aResolution,
+ scrollOffset.y * aResolution * aResolution);
// Return true here even if the scroll offset was 0,0 because the RV is used
// as a scroll container indicator. Non-scroll containers won't have cached
@@ -899,6 +901,11 @@ LayoutDeviceIntRect RemoteAccessible::BoundsWithOffset(
ApplyCrossDocOffset(bounds);
+ Maybe<float> res =
+ mDoc->mCachedFields->GetAttribute<float>(CacheKey::Resolution);
+ MOZ_ASSERT(res, "No cached document resolution found.");
+ const float resolution = res.valueOr(1.0f);
+
LayoutDeviceIntRect devPxBounds;
const Accessible* acc = Parent();
bool encounteredFixedContainer = IsFixedPos();
@@ -948,7 +955,8 @@ LayoutDeviceIntRect RemoteAccessible::BoundsWithOffset(
// happens in this loop instead of both inside and outside of
// the loop (like ApplyTransform).
// Never apply scroll offsets past a fixed container.
- const bool hasScrollArea = remoteAcc->ApplyScrollOffset(bounds);
+ const bool hasScrollArea =
+ remoteAcc->ApplyScrollOffset(bounds, resolution);
// If we are hit testing and the Accessible has a scroll area, ensure
// that the bounds we've calculated so far are constrained to the
diff --git a/accessible/ipc/RemoteAccessible.h b/accessible/ipc/RemoteAccessible.h
@@ -459,7 +459,7 @@ class RemoteAccessible : public Accessible, public HyperTextAccessibleBase {
void SetParent(RemoteAccessible* aParent);
Maybe<nsRect> RetrieveCachedBounds() const;
bool ApplyTransform(nsRect& aCumulativeBounds) const;
- bool ApplyScrollOffset(nsRect& aBounds) const;
+ bool ApplyScrollOffset(nsRect& aBounds, float aResolution) const;
void ApplyCrossDocOffset(nsRect& aBounds) const;
void ApplyVisualViewportOffset(nsRect& aBounds) const;
LayoutDeviceIntRect BoundsWithOffset(
