Bug 1762255 - Convert MobileViewport intrinric size to app units and …

…use it in the comparison. r=botond,geckoview-reviewers,m_kato Differential Revision: https://phabricator.services.mozilla.com/D169288
dothq · Feb 28, 2023 · 9d1e021 · 9d1e021
1 parent d884f32
commit 9d1e021
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 4 deletions.
diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
@@ -8147,7 +8147,11 @@ bool nsLayoutUtils::UpdateCompositionBoundsForRCDRSF(
   if (shouldSubtractDynamicToolbar == SubtractDynamicToolbar::Yes) {
     if (RefPtr<MobileViewportManager> MVM =
             aPresContext->PresShell()->GetMobileViewportManager()) {
-      CSSSize intrinsicCompositionSize = MVM->GetIntrinsicCompositionSize();
+      // Convert the intrinsic composition size to app units here since
+      // the returned size of below CalculateScrollableRectForFrame call has
+      // been already converted/rounded to app units.
+      nsSize intrinsicCompositionSize =
+          CSSSize::ToAppUnits(MVM->GetIntrinsicCompositionSize());
 
       if (nsIScrollableFrame* rootScrollableFrame =
               aPresContext->PresShell()->GetRootScrollFrameAsScrollable()) {
@@ -8156,9 +8160,8 @@ bool nsLayoutUtils::UpdateCompositionBoundsForRCDRSF(
         // composition size (i.e. the dynamic toolbar should be able to move
         // only if the content is vertically scrollable).
         if (intrinsicCompositionSize.height <
-            CSSPixel::FromAppUnits(
-                CalculateScrollableRectForFrame(rootScrollableFrame, nullptr)
-                    .Height())) {
+            CalculateScrollableRectForFrame(rootScrollableFrame, nullptr)
+                .Height()) {
           shouldSubtractDynamicToolbar = SubtractDynamicToolbar::No;
         }
       }

diff --git a/mobile/android/geckoview/src/androidTest/assets/www/no-meta-viewport.html b/mobile/android/geckoview/src/androidTest/assets/www/no-meta-viewport.html
@@ -0,0 +1,5 @@
+<!DOCTYPE html>
+<html>
+<meta charset="utf-8">
+<h3>Nothing here</h3>
+</html>
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
@@ -123,6 +123,7 @@ open class BaseSessionTest(noErrorCollector: Boolean = false) {
         const val COLOR_ORANGE_BACKGROUND_HTML_PATH = "/assets/www/color_orange_background.html"
         const val TRACEMONKEY_PDF_PATH = "/assets/www/tracemonkey.pdf"
         const val HELLO_PDF_WORLD_PDF_PATH = "/assets/www/helloPDFWorld.pdf"
+        const val NO_META_VIEWPORT_HTML_PATH = "/assets/www/no-meta-viewport.html"
 
         const val TEST_ENDPOINT = GeckoSessionTestRule.TEST_ENDPOINT
         const val TEST_HOST = GeckoSessionTestRule.TEST_HOST

diff --git a/...ndroid/geckoview/src/androidTest/java/org/mozilla/geckoview/test/InputResultDetailTest.kt b/...ndroid/geckoview/src/androidTest/java/org/mozilla/geckoview/test/InputResultDetailTest.kt
@@ -377,4 +377,41 @@ class InputResultDetailTest : BaseSessionTest() {
             )
         }
     }
+
+    // Tests a situation where converting a scrollport size between CSS units and app units will
+    // result different values, and the difference causes an issue that unscrollable documents
+    // behave as if it's scrollable.
+    //
+    // Note about metrics that this test uses.
+    // A basic here is that documents having no meta viewport tags are laid out on 980px width
+    // canvas, the 980px is defined as "browser.viewport.desktopWidth".
+    //
+    // So, if the device screen size is (1080px, 2160px) then the document is scaled to
+    // (1080 / 980) = 1.10204. Then if the dynamic toolbar height is 59px, the scaled document
+    // height is (2160 - 59) / 1.10204 = 1906.46 (in CSS units).  It's converted and actually rounded
+    // to 114388 (= 1906.46 * 60) in app units. And it's converted back to 1906.47 (114388 / 60) in
+    // CSS units unfortunately.
+    @WithDisplay(width = 1080, height = 2160)
+    @Test
+    fun testFractionalScrollPortSize() {
+        sessionRule.setPrefsUntilTestEnd(
+            mapOf(
+                "browser.viewport.desktopWidth" to 980
+            )
+        )
+        sessionRule.display?.run { setDynamicToolbarMaxHeight(59) }
+
+        setupDocument(NO_META_VIEWPORT_HTML_PATH)
+
+        // Try to scroll down to see if the document is scrollable or not.
+        var value = sessionRule.waitForResult(sendDownEvent(50f, 50f))
+
+        assertResultDetail(
+            "The document isn't not scrollable at all",
+            value,
+            PanZoomController.INPUT_RESULT_UNHANDLED,
+            PanZoomController.SCROLLABLE_FLAG_NONE,
+            (PanZoomController.OVERSCROLL_FLAG_HORIZONTAL or PanZoomController.OVERSCROLL_FLAG_VERTICAL)
+        )
+    }
 }