Fix mozilla-mobile#9614 - Use the new InputResultDetail

Both NestedGeckoView and NestedWebView will now return an InputResultDetail wrapping many new details about how a touch event will be handled. NestedGeckoView's InputResultDetail will be used to more accurately decide when to animate the toolbar or start the pull to refresh feature. NestedWebView's InputResultDetail will only have details about if it will handle the touch or not. With all the other being unknown the dynamic toolbar or pull to refresh features will not work.
Mugurell · Mar 24, 2021 · f4eaadc · f4eaadc
1 parent ded0419
commit f4eaadc
Show file tree

Hide file tree

Showing 14 changed files with 215 additions and 182 deletions.
diff --git a/...ne-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/...ne-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt
@@ -90,13 +90,6 @@ class GeckoEngineView @JvmOverloads constructor(
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     internal var currentSelection: BasicSelectionActionDelegate? = null
 
-    /**
-     * Cache of the last valid input result we got from GeckoView.
-     */
-    @Suppress("Deprecation")
-    @VisibleForTesting
-    internal var lastInputResult: EngineView.InputResult = EngineView.InputResult.INPUT_RESULT_UNHANDLED
-
     override var selectionActionDelegate: SelectionActionDelegate? = null
 
     init {
@@ -175,25 +168,7 @@ class GeckoEngineView @JvmOverloads constructor(
     override fun canScrollVerticallyDown() =
         true // waiting for this issue https://bugzilla.mozilla.org/show_bug.cgi?id=1507569
 
-    @Suppress("MagicNumber", "Deprecation")
-    override fun getInputResult(): EngineView.InputResult {
-        // Direct mapping of GeckoView's returned values.
-        // If not fail fast to allow for a quick fix.
-        val input = geckoView.inputResult
-        lastInputResult = when (input) {
-            0 -> EngineView.InputResult.INPUT_RESULT_UNHANDLED
-            1 -> EngineView.InputResult.INPUT_RESULT_HANDLED
-            2 -> EngineView.InputResult.INPUT_RESULT_HANDLED_CONTENT
-            3 -> {
-                // Drop this result and return the previous.
-                // See https://bugzilla.mozilla.org/show_bug.cgi?id=1687430 for details
-                lastInputResult
-            }
-            else -> throw IllegalArgumentException("Unexpected geckoView.inputResult: \"$input\"")
-        }
-
-        return lastInputResult
-    }
+    override fun getInputResultDetail() = geckoView.inputResultDetail
 
     override fun setVerticalClipping(clippingHeight: Int) {
         geckoView.setVerticalClipping(clippingHeight)

diff --git a/...ne-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/NestedGeckoView.kt b/...ne-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/NestedGeckoView.kt
@@ -10,10 +10,8 @@ import androidx.annotation.VisibleForTesting
 import androidx.core.view.NestedScrollingChild
 import androidx.core.view.NestedScrollingChildHelper
 import androidx.core.view.ViewCompat
-import mozilla.components.concept.engine.EngineView
+import mozilla.components.concept.engine.InputResultDetail
 import org.mozilla.geckoview.GeckoView
-import org.mozilla.geckoview.PanZoomController.INPUT_RESULT_HANDLED
-import org.mozilla.geckoview.PanZoomController.INPUT_RESULT_UNHANDLED
 
 /**
  * geckoView that supports nested scrolls (for using in a CoordinatorLayout).
@@ -44,11 +42,11 @@ open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrolli
     internal var childHelper: NestedScrollingChildHelper = NestedScrollingChildHelper(this)
 
     /**
-     * Integer indicating how user's MotionEvent was handled.
+     * How user's MotionEvent will be handled.
      *
-     * There must be a 1-1 relation between this values and [EngineView.InputResult]'s.
+     * @see InputResultDetail
      */
-    internal var inputResult: Int = INPUT_RESULT_UNHANDLED
+    internal val inputResultDetail = InputResultDetail()
 
     init {
         isNestedScrollingEnabled = true
@@ -62,7 +60,8 @@ open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrolli
 
         when (action) {
             MotionEvent.ACTION_MOVE -> {
-                val allowScroll = !shouldPinOnScreen() && inputResult == INPUT_RESULT_HANDLED
+                val allowScroll = !shouldPinOnScreen() && inputResultDetail.isTouchHandledByBrowser()
+
                 var deltaY = lastY - eventY
 
                 if (allowScroll && dispatchNestedPreScroll(0, deltaY, scrollConsumed, scrollOffset)) {
@@ -99,7 +98,7 @@ open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrolli
                 stopNestedScroll()
                 // Reset handled status so that parents of this View would not get the old value
                 // when querying it for a newly started touch event.
-                inputResult = INPUT_RESULT_UNHANDLED
+                inputResultDetail.reset()
             }
         }
 
@@ -121,9 +120,9 @@ open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrolli
     internal fun updateInputResult(event: MotionEvent) {
         super.onTouchEventForDetailResult(event)
             .accept {
-                // This should never be null.
-                // Prefer to crash and investigate after rather than not knowing about problems with this.
-                inputResult = it?.handledResult()!!
+                inputResultDetail.update(
+                    it?.handledResult(), it?.scrollableDirections(), it?.overscrollDirections()
+                )
                 startNestedScroll(ViewCompat.SCROLL_AXIS_VERTICAL)
             }
     }

diff --git a/...ecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt b/...ecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt
@@ -12,7 +12,6 @@ import android.view.View
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import mozilla.components.browser.engine.gecko.GeckoEngineView.Companion.DARK_COVER
 import mozilla.components.browser.engine.gecko.selection.GeckoSelectionActionDelegate
-import mozilla.components.concept.engine.EngineView
 import mozilla.components.concept.engine.mediaquery.PreferredColorScheme
 import mozilla.components.concept.engine.selection.SelectionActionDelegate
 import mozilla.components.support.test.argumentCaptor
@@ -22,6 +21,7 @@ import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertNull
+import org.junit.Assert.assertSame
 import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -278,55 +278,11 @@ class GeckoEngineViewTest {
         assertFalse(engineView.canClearSelection())
     }
 
-    @Suppress("Deprecation")
     @Test
-    fun `currentInputResult should default to EngineView#InputResult#INPUT_RESULT_UNHANDLED`() {
+    fun `GIVEN a GeckoView WHEN EngineView returns the InputResultDetail THEN the value from the GeckoView is used`() {
         val engineView = GeckoEngineView(context)
+        val geckoview = engineView.geckoView
 
-        assertEquals(EngineView.InputResult.INPUT_RESULT_UNHANDLED, engineView.lastInputResult)
-    }
-
-    @Suppress("Deprecation")
-    @Test
-    fun `getInputResult should do a 1-1 mapping of the values received from GeckoView and cache the result`() {
-        val engineView = GeckoEngineView(context)
-        engineView.geckoView = mock()
-
-        whenever(engineView.geckoView.inputResult).thenReturn(0)
-        assertEquals(EngineView.InputResult.INPUT_RESULT_UNHANDLED, engineView.getInputResult())
-        assertEquals(EngineView.InputResult.INPUT_RESULT_UNHANDLED, engineView.lastInputResult)
-
-        whenever(engineView.geckoView.inputResult).thenReturn(1)
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED, engineView.getInputResult())
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED, engineView.lastInputResult)
-
-        whenever(engineView.geckoView.inputResult).thenReturn(2)
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED_CONTENT, engineView.getInputResult())
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED_CONTENT, engineView.lastInputResult)
-    }
-
-    @Suppress("Deprecation")
-    @Test
-    fun `INPUT_RESULD_IGNORED should be ignored`() {
-        val engineView = GeckoEngineView(context)
-        engineView.geckoView = mock()
-
-        whenever(engineView.geckoView.inputResult).thenReturn(1)
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED, engineView.getInputResult())
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED, engineView.lastInputResult)
-
-        whenever(engineView.geckoView.inputResult).thenReturn(3)
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED, engineView.getInputResult())
-        assertEquals(EngineView.InputResult.INPUT_RESULT_HANDLED, engineView.lastInputResult)
-    }
-
-    @Suppress("Deprecation")
-    @Test(expected = IllegalArgumentException::class)
-    fun `Values other than 0, 1, 2, 3 received as input results from GeckoView should throw`() {
-        val engineView = GeckoEngineView(context)
-        engineView.geckoView = mock()
-
-        whenever(engineView.geckoView.inputResult).thenReturn(4)
-        engineView.getInputResult()
+        assertSame(geckoview.inputResultDetail, engineView.getInputResultDetail())
     }
 }
diff --git a/...ecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/NestedGeckoViewTest.kt b/...ecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/NestedGeckoViewTest.kt
@@ -19,6 +19,7 @@ import mozilla.components.support.test.argumentCaptor
 import mozilla.components.support.test.mock
 import mozilla.components.support.test.mockMotionEvent
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -28,7 +29,6 @@ import org.mockito.Mockito.spy
 import org.mockito.Mockito.times
 import org.mockito.Mockito.verify
 import org.mozilla.geckoview.PanZoomController.INPUT_RESULT_HANDLED
-import org.mozilla.geckoview.PanZoomController.INPUT_RESULT_UNHANDLED
 import org.robolectric.Robolectric.buildActivity
 
 @RunWith(AndroidJUnit4::class)
@@ -97,7 +97,7 @@ class NestedGeckoViewTest {
         val nestedWebView = spy(NestedGeckoView(context))
         val mockChildHelper: NestedScrollingChildHelper = mock()
         nestedWebView.childHelper = mockChildHelper
-        nestedWebView.inputResult = INPUT_RESULT_HANDLED
+        nestedWebView.inputResultDetail.update(INPUT_RESULT_HANDLED)
         doReturn(true).`when`(nestedWebView).callSuperOnTouchEvent(any())
 
         doReturn(true).`when`(mockChildHelper).dispatchNestedPreScroll(
@@ -127,19 +127,23 @@ class NestedGeckoViewTest {
     @Test
     fun `verify onTouchEvent when ACTION_UP or ACTION_CANCEL`() {
         val nestedWebView = spy(NestedGeckoView(context))
-        nestedWebView.inputResult = INPUT_RESULT_HANDLED
+        nestedWebView.inputResultDetail.update(INPUT_RESULT_HANDLED)
         val mockChildHelper: NestedScrollingChildHelper = mock()
         nestedWebView.childHelper = mockChildHelper
         doReturn(true).`when`(nestedWebView).callSuperOnTouchEvent(any())
 
         nestedWebView.onTouchEvent(mockMotionEvent(ACTION_UP))
         verify(mockChildHelper).stopNestedScroll()
-        assertEquals(INPUT_RESULT_UNHANDLED, nestedWebView.inputResult)
+        // ACTION_UP should call "inputResultDetail.reset()". Test that call's effect.
+        assertTrue(nestedWebView.inputResultDetail.isTouchUnhandled())
+        assertFalse(nestedWebView.inputResultDetail.isTouchHandledByBrowser())
 
-        nestedWebView.inputResult = INPUT_RESULT_HANDLED
+        nestedWebView.inputResultDetail.update(INPUT_RESULT_HANDLED)
         nestedWebView.onTouchEvent(mockMotionEvent(ACTION_CANCEL))
         verify(mockChildHelper, times(2)).stopNestedScroll()
-        assertEquals(INPUT_RESULT_UNHANDLED, nestedWebView.inputResult)
+        // ACTION_CANCEL should call "inputResultDetail.reset()". Test that call's effect.
+        assertTrue(nestedWebView.inputResultDetail.isTouchUnhandled())
+        assertFalse(nestedWebView.inputResultDetail.isTouchHandledByBrowser())
 
         // onTouchEventForResult should be called only for ACTION_DOWN
         verify(nestedWebView, times(0)).updateInputResult(any())

diff --git a/...ser/engine-system/src/main/java/mozilla/components/browser/engine/system/NestedWebView.kt b/...ser/engine-system/src/main/java/mozilla/components/browser/engine/system/NestedWebView.kt
@@ -6,18 +6,20 @@ package mozilla.components.browser.engine.system
 
 import android.annotation.SuppressLint
 import android.content.Context
-import androidx.core.view.NestedScrollingChild
-import androidx.core.view.NestedScrollingChildHelper
 import android.view.MotionEvent
-import android.view.MotionEvent.ACTION_MOVE
+import android.view.MotionEvent.ACTION_CANCEL
 import android.view.MotionEvent.ACTION_DOWN
+import android.view.MotionEvent.ACTION_MOVE
 import android.view.MotionEvent.ACTION_UP
-import android.view.MotionEvent.ACTION_CANCEL
 import android.view.MotionEvent.obtain
 import android.webkit.WebView
 import androidx.annotation.VisibleForTesting
+import androidx.core.view.NestedScrollingChild
+import androidx.core.view.NestedScrollingChildHelper
 import androidx.core.view.ViewCompat
-import mozilla.components.concept.engine.EngineView
+import mozilla.components.concept.engine.INPUT_HANDLED
+import mozilla.components.concept.engine.INPUT_UNHANDLED
+import mozilla.components.concept.engine.InputResultDetail
 
 /**
  * WebView that supports nested scrolls (for using in a CoordinatorLayout).
@@ -46,18 +48,15 @@ class NestedWebView(context: Context) : WebView(context), NestedScrollingChild {
     internal var childHelper: NestedScrollingChildHelper = NestedScrollingChildHelper(this)
 
     /**
-     * Integer indicating how user's MotionEvent was handled.
+     * How user's MotionEvent will be handled.
      *
-     * There must be a 1-1 relation between this values and [EngineView.InputResult]'s.
-     */
-    @Suppress("Deprecation")
-    internal val inputResult: Int
-        get() = getInputResult(eventHandled)
-
-    /**
-     * Holder for if the previous [android.view.MotionEvent] was handled by us or not.
+     * @see InputResultDetail
      */
-    private var eventHandled: Boolean = false
+    @VisibleForTesting
+    internal val inputResultDetail = InputResultDetail().apply {
+        // The constructor enables vertical overscrolling by default. Don't want that.
+        update(overscrollDirections = 0)
+    }
 
     init {
         isNestedScrollingEnabled = true
@@ -104,14 +103,20 @@ class NestedWebView(context: Context) : WebView(context), NestedScrollingChild {
         }
 
         // Execute event handler from parent class in all cases
-        eventHandled = super.onTouchEvent(event)
+        val eventHandled = callSuperOnTouchEvent(event)
+        updateInputResult(eventHandled)
 
         // Recycle previously obtained event
         event.recycle()
 
         return eventHandled
     }
 
+    @VisibleForTesting
+    internal fun callSuperOnTouchEvent(event: MotionEvent): Boolean {
+        return super.onTouchEvent(event)
+    }
+
     // NestedScrollingChild
 
     override fun setNestedScrollingEnabled(enabled: Boolean) {
@@ -156,12 +161,12 @@ class NestedWebView(context: Context) : WebView(context), NestedScrollingChild {
         return childHelper.dispatchNestedPreFling(velocityX, velocityY)
     }
 
-    @Suppress("Deprecation")
-    private fun getInputResult(eventHandled: Boolean): Int {
-        return if (eventHandled) {
-            EngineView.InputResult.INPUT_RESULT_HANDLED.value
+    @VisibleForTesting
+    internal fun updateInputResult(eventHandled: Boolean) {
+        inputResultDetail.update(if (eventHandled) {
+            INPUT_HANDLED
         } else {
-            EngineView.InputResult.INPUT_RESULT_UNHANDLED.value
-        }
+            INPUT_UNHANDLED
+        })
     }
 }
diff --git a/.../engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineView.kt b/.../engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineView.kt
@@ -53,6 +53,7 @@ import mozilla.components.concept.engine.EngineSession
 import mozilla.components.concept.engine.EngineSession.TrackingProtectionPolicy
 import mozilla.components.concept.engine.EngineView
 import mozilla.components.concept.engine.HitResult
+import mozilla.components.concept.engine.InputResultDetail
 import mozilla.components.concept.engine.content.blocking.Tracker
 import mozilla.components.concept.engine.prompt.PromptRequest
 import mozilla.components.concept.engine.request.RequestInterceptor.InterceptionResponse
@@ -694,15 +695,12 @@ class SystemEngineView @JvmOverloads constructor(
 
     override fun canScrollVerticallyDown() = session?.webView?.canScrollVertically(1) ?: false
 
-    @Suppress("Deprecation")
-    override fun getInputResult(): EngineView.InputResult {
-        (session?.webView as? NestedWebView)?.let { nestedWebView ->
-            // Direct mapping of WebView's returned values.
-            // There should be a 1-1 relation. If not fail fast to allow for a quick fix.
-            return EngineView.InputResult.values().first { it.value == nestedWebView.inputResult }
-        }
-        // Let's be preventive about not knowing if user's touch was handled and say no.
-        return EngineView.InputResult.INPUT_RESULT_UNHANDLED
+    override fun getInputResultDetail(): InputResultDetail {
+        return (session?.webView as? NestedWebView)?.inputResultDetail
+            ?: InputResultDetail().apply {
+                // The constructor enables vertical overscrolling by default. Don't want that.
+                update(overscrollDirections = 0)
+            }
     }
 
     override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) {