commit 2182ab7d4e98ba48416df8b9137f006fb01d80c0
parent c7865f7f36aada8c1ebbfdab2b15bfbd382b4f17
Author: Hiroyuki Ikezoe <hikezoe.birchill@mozilla.com>
Date: Thu, 4 Dec 2025 05:29:28 +0000
Bug 1993407 - Use the visual viewport size not including the dynamic toolbar for a specific case. r=botond,geckoview-reviewers
We use the visual viewport size including the dynamic
toolbar area in APZ generally, but it has a side-effect happening in
FrameMetrics::KeepLayoutViewportEnclosingVisualViewport. In
KeepLayoutViewportEnclosingVisualViewport we try to adjust the layout
scroll offset so that the updated visual viewport by the new visual
offset fits within the layout viewport range. The side-effect is
harmless in most cases but we recently realized that it cases a user
visible issue such as bug 1997575.
To avoid bug 1997575 specifically, this change uses the visual viewport
not including the dynamic toolbar in KeepLayoutViewportEnclosingVisualViewport
for the specific case where all following conditions are met;
i) the dynamic toolbar is fully visible
ii) the software keyboard is opened
iii) the interactive-widget mode is resizes-visual
Differential Revision: https://phabricator.services.mozilla.com/D272985
Diffstat:
6 files changed, 164 insertions(+), 7 deletions(-)
diff --git a/gfx/layers/FrameMetrics.cpp b/gfx/layers/FrameMetrics.cpp
@@ -47,17 +47,50 @@ std::ostream& operator<<(std::ostream& aStream, const FrameMetrics& aMetrics) {
return aStream;
}
-void FrameMetrics::RecalculateLayoutViewportOffset() {
+CSSRect FrameMetrics::GetVisualViewportForLayoutViewportContainment(
+ ScreenCoord aFixedLayerBottomMargin) const {
+ const bool hasDynamicToolbar = GetCompositionSizeWithoutDynamicToolbar() !=
+ GetCompositionBounds().Size();
+ // In the case where the toolbar is dynamic if `aFixedLayerBottomMargin`
+ // is zero, it means the dynamic toolbar is fully visible.
+ const bool isDynamicToolbarFullyVisible =
+ hasDynamicToolbar && aFixedLayerBottomMargin == 0;
+
+ return CSSRect(
+ GetVisualScrollOffset(),
+ // Use `mCompositionSizeWithoutDynamicToolbar` in the case where the
+ // dynamic toolbar is fully visible.
+ // Theoretically we don't need to check `IsSoftwareKeyboardVisible()` or
+ // `GetInteractiveWidget()` either, but for now we'd like to restrict this
+ // behavior change in the scope of the visual scroll offset change
+ // initiated by zoom-to-focused-input on resizes-visual with the software
+ // keyboard.
+ // TODO Bug 2003420: This restriction will be dropped in one of the bugs
+ // blocking bug 2003420. As of now it's unclear what kind of test
+ // cases need to drop this restction as user visible issues.
+ isDynamicToolbarFullyVisible && IsSoftwareKeyboardVisible() &&
+ GetInteractiveWidget() == dom::InteractiveWidget::ResizesVisual
+ ? CalculateCompositedSizeInCssPixels(
+ ParentLayerRect(ParentLayerPoint(),
+ mCompositionSizeWithoutDynamicToolbar),
+ mZoom)
+ : CalculateCompositedSizeInCssPixels());
+}
+
+void FrameMetrics::RecalculateLayoutViewportOffset(
+ ScreenCoord aFixedLayerBottomMargin) {
// For subframes, the visual and layout viewports coincide, so just
// keep the layout viewport offset in sync with the visual one.
if (!mIsRootContent) {
mLayoutViewport.MoveTo(GetVisualScrollOffset());
return;
}
+
// For the root, the two viewports can diverge, but the layout
// viewport needs to keep enclosing the visual viewport.
- KeepLayoutViewportEnclosingVisualViewport(GetVisualViewport(),
- mScrollableRect, mLayoutViewport);
+ KeepLayoutViewportEnclosingVisualViewport(
+ GetVisualViewportForLayoutViewportContainment(aFixedLayerBottomMargin),
+ mScrollableRect, mLayoutViewport);
}
/* static */
diff --git a/gfx/layers/FrameMetrics.h b/gfx/layers/FrameMetrics.h
@@ -397,6 +397,15 @@ struct FrameMetrics {
CalculateCompositedSizeInCssPixels());
}
+ // TODO Bug 2003420: This function should eventually be able to supercede
+ // GetVisualViewport and drop the default argument for
+ // |aFixedLayerBottomMargin|. The difference from GetVisualViewport is this
+ // function handles the current dynamic toolbar state. In other words
+ // GetVisualViewport always handles the toolbar state as if the dynamic
+ // toolbar is completely hidden.
+ CSSRect GetVisualViewportForLayoutViewportContainment(
+ ScreenCoord aFixedLayerBottomMargin = 0) const;
+
void SetTransformToAncestorScale(
const ParentLayerToScreenScale2D& aTransformToAncestorScale) {
mTransformToAncestorScale = aTransformToAncestorScale;
@@ -472,7 +481,7 @@ struct FrameMetrics {
// allow APZ to async-scroll the layout viewport.
//
// This is a no-op if mIsRootContent is false.
- void RecalculateLayoutViewportOffset();
+ void RecalculateLayoutViewportOffset(ScreenCoord aFixedLayerBottomMargin = 0);
void SetFixedLayerMargins(const ScreenMargin& aFixedLayerMargins) {
mFixedLayerMargins = aFixedLayerMargins;
diff --git a/gfx/layers/apz/src/AsyncPanZoomController.cpp b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -5526,11 +5526,20 @@ void AsyncPanZoomController::NotifyLayersUpdated(
LayersUpdateFlags aLayersUpdateFlags) {
AssertOnUpdaterThread();
+ const FrameMetrics& aLayerMetrics = aScrollMetadata.GetMetrics();
+ ScreenMargin fixedLayerMargins;
+ // Get the fixed layers margins for if the new data is the root content one.
+ // NOTE: This needs to be done before obtaining |mRecursiveMutex| below to
+ // respect the APZ lock ordering principle.
+ if (aLayerMetrics.IsRootContent()) {
+ if (APZCTreeManager* treeManagerLocal = GetApzcTreeManager()) {
+ fixedLayerMargins = treeManagerLocal->GetCompositorFixedLayerMargins();
+ }
+ }
+
RecursiveMutexAutoLock lock(mRecursiveMutex);
bool isDefault = mScrollMetadata.IsDefault();
- const FrameMetrics& aLayerMetrics = aScrollMetadata.GetMetrics();
-
if ((aScrollMetadata == mLastContentPaintMetadata) && !isDefault) {
// No new information here, skip it.
APZC_LOGV("%p NotifyLayersUpdated short-circuit\n", this);
@@ -6113,7 +6122,19 @@ void AsyncPanZoomController::NotifyLayersUpdated(
// The rest of this branch largely follows the code in the
// |if (scrollOffsetUpdated)| branch above. Eventually it should get
// merged into that branch.
- Metrics().RecalculateLayoutViewportOffset();
+ //
+ // RecalculateLayoutViewportOffset tries to adjust the layout scroll offset
+ // if the updated visual scroll offset overflows the visual viewport from
+ // the layout viewport. Unfortunately the visual viewport calculated in APZ
+ // is basically including the dynamic toolbar area (because position:fixed
+ // (or sticky) elements are directly composited on the compositor in
+ // response to the dynamic toolbar movement), thus with the slightly larger
+ // visual viewport RecalculateLayoutViewportOffset unintentionally moves the
+ // layout scroll offset even if the dynamic toolbar is not collapsed at all.
+ // So we pass the compositor fixed layers margins which is representing the
+ // dynamic toolbar state to RecalculateLayoutViewportOffset to avoid such
+ // unintentional layout offset changes.
+ Metrics().RecalculateLayoutViewportOffset(fixedLayerMargins.bottom);
mExpectedGeckoMetrics.UpdateFrom(aLayerMetrics);
if (ShouldCancelAnimationForScrollUpdate(Nothing())) {
CancelAnimation();
diff --git a/mobile/android/geckoview/src/androidTest/assets/www/bug1993407.html b/mobile/android/geckoview/src/androidTest/assets/www/bug1993407.html
@@ -0,0 +1,36 @@
+<!doctype html>
+<meta http-equiv="content-type" content="text/html; charset=UTF-8" />
+<meta
+ name="viewport"
+ content="width=device-width,initial-scale=1,interactive-widget=resizes-visual"
+/>
+<style>
+ body {
+ margin: 0px;
+ }
+ input {
+ width: 100%;
+ height: calc(100lvh - 100svh);
+ position: fixed;
+ bottom: 0px;
+ padding: 0px;
+ border: none;
+ }
+ .ruler {
+ width: 100vw;
+ height: 100px;
+ font-size: 70px;
+ box-sizing: border-box;
+ border: solid black 1px;
+ background-color: gray;
+ }
+</style>
+<input type="text" value="text" size="10" />
+<script>
+ for (let i = 0; i < 20; i++) {
+ const div = document.createElement("div");
+ div.classList.add("ruler");
+ div.innerText = `${i}`;
+ document.body.appendChild(div);
+ }
+</script>
diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt
@@ -156,6 +156,7 @@ open class BaseSessionTest(
const val BUG1909181_HTML_PATH = "/assets/www/bug1909181.html"
const val BUG1912358_HTML_PATH = "/assets/www/bug1912358.html"
const val BUG1985669_HTML_PATH = "/assets/www/bug1985669.html"
+ const val BUG1993407_HTML_PATH = "/assets/www/bug1993407.html"
const val BUG1994311_HTML_PATH = "/assets/www/bug1994311.html"
const val POSITION_STICKY_HTML_PATH = "/assets/www/position-sticky.html"
const val POSITION_STICKY_ON_MAIN_THREAD_HTML_PATH = "/assets/www/position-sticky-on-main-thread.html"
diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/InteractiveWidgetTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/InteractiveWidgetTest.kt
@@ -17,6 +17,8 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.MediumTest
import org.hamcrest.Matchers.equalTo
import org.hamcrest.Matchers.not
+import org.hamcrest.Matchers.notNullValue
+import org.json.JSONObject
import org.junit.After
import org.junit.Before
import org.junit.Rule
@@ -330,4 +332,59 @@ class InteractiveWidgetTest : BaseSessionTest() {
equalTo(viewportHeight),
)
}
+
+ @GeckoSessionTestRule.NullDelegate(Autofill.Delegate::class)
+ @Test
+ fun bug1993407() {
+ mainSession.setActive(true)
+
+ mainSession.loadTestPath(BaseSessionTest.BUG1993407_HTML_PATH)
+ mainSession.waitForPageStop()
+ mainSession.promiseAllPaintsDone()
+ mainSession.flushApzRepaints()
+
+ val caretRect = mainSession.evaluateJS(
+ """
+ const inputRect = document.querySelector('input').getBoundingClientRect();
+ document.caretPositionFromPoint(0, inputRect.y)?.getClientRect();
+ """.trimIndent(),
+ )
+ assertThat("The caretRect should not be null", caretRect, notNullValue())
+
+ val caretRectObject = caretRect as JSONObject
+ val caretY = caretRectObject.getDouble("y")
+ val caretHeight = caretRectObject.getDouble("height")
+ val caretBottom = caretRectObject.getDouble("bottom")
+
+ // Open the software keyboard.
+ ensureKeyboardOpen()
+
+ mainSession.evaluateJS("document.querySelector('input').focus();")
+ mainSession.zoomToFocusedInput()
+
+ mainSession.flushApzRepaints()
+ mainSession.promiseAllPaintsDone()
+
+ val scrollY = mainSession.evaluateJS("window.scrollY") as Double
+ val offsetTop = mainSession.evaluateJS("window.visualViewport.offsetTop") as Double
+ val pageTop = mainSession.evaluateJS("window.visualViewport.pageTop") as Double
+ val visualViewportHeight = mainSession.evaluateJS("window.visualViewport.height") as Double
+
+ assertThat(
+ "The offsetTop and pageTop of visual viewport is not diverged",
+ offsetTop,
+ equalTo(pageTop),
+ )
+ assertThat("The offsetTop is not 0", offsetTop, not(equalTo(0.0)))
+ assertThat("The offsetTop is ", offsetTop, equalTo(caretBottom - visualViewportHeight))
+
+ assertThat(
+ "The layout scroll offset stays at 0",
+ scrollY,
+ equalTo(0.0),
+ )
+
+ // Close the software keyboard.
+ imm.hideSoftInputFromWindow(view.getWindowToken(), 0)
+ }
}
