From 553c4ed9a7b77f8e625edf581bd67878608cdbae Mon Sep 17 00:00:00 2001 From: Mugurell Date: Tue, 6 Jul 2021 15:58:13 +0300 Subject: [PATCH] For #10555 - Use INPUT_HANDLING_UNKNOWN as the default of InputResultDetail We need to wait until having a response from GeckoView on how it handled the touch only after which we'll know whether to animate the toolbar or not. The edgecase scenario of having pull to refresh enabled even before having a response from GeckoView will still work because "canOverscrollTop()" only checks for the touch to not be handled by the browser to pan the page. --- .../engine/gecko/NestedGeckoViewTest.kt | 4 +- .../engine/system/NestedWebViewTest.kt | 4 +- .../engine/system/SystemEngineViewTest.kt | 2 +- .../behavior/BrowserToolbarBehavior.kt | 2 +- .../behavior/BrowserToolbarBehaviorTest.kt | 34 ++++++++++- .../concept/engine/InputResultDetail.kt | 17 +++++- .../concept/engine/InputResultDetailTest.kt | 59 ++++++++++++++----- docs/changelog.md | 3 + 8 files changed, 100 insertions(+), 25 deletions(-) diff --git a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/NestedGeckoViewTest.kt b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/NestedGeckoViewTest.kt index b5d95681bc1..abc1d9a4cd6 100644 --- a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/NestedGeckoViewTest.kt +++ b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/NestedGeckoViewTest.kt @@ -135,14 +135,14 @@ class NestedGeckoViewTest { nestedWebView.onTouchEvent(mockMotionEvent(ACTION_UP)) verify(mockChildHelper).stopNestedScroll() // ACTION_UP should call "inputResultDetail.reset()". Test that call's effect. - assertTrue(nestedWebView.inputResultDetail.isTouchUnhandled()) + assertTrue(nestedWebView.inputResultDetail.isTouchHandlingUnknown()) assertFalse(nestedWebView.inputResultDetail.isTouchHandledByBrowser()) nestedWebView.inputResultDetail = nestedWebView.inputResultDetail.copy(INPUT_RESULT_HANDLED) nestedWebView.onTouchEvent(mockMotionEvent(ACTION_CANCEL)) verify(mockChildHelper, times(2)).stopNestedScroll() // ACTION_CANCEL should call "inputResultDetail.reset()". Test that call's effect. - assertTrue(nestedWebView.inputResultDetail.isTouchUnhandled()) + assertTrue(nestedWebView.inputResultDetail.isTouchHandlingUnknown()) assertFalse(nestedWebView.inputResultDetail.isTouchHandledByBrowser()) // onTouchEventForResult should be called only for ACTION_DOWN diff --git a/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/NestedWebViewTest.kt b/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/NestedWebViewTest.kt index 61c021fb57e..05b5f6d287f 100644 --- a/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/NestedWebViewTest.kt +++ b/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/NestedWebViewTest.kt @@ -120,7 +120,7 @@ class NestedWebViewTest { fun `GIVEN NestedWebView WHEN a new instance is created THEN a properly configured InputResultDetail is created`() { val nestedWebView = NestedWebView(testContext) - assertTrue(nestedWebView.inputResultDetail.isTouchUnhandled()) + assertTrue(nestedWebView.inputResultDetail.isTouchHandlingUnknown()) assertFalse(nestedWebView.inputResultDetail.canScrollToLeft()) assertFalse(nestedWebView.inputResultDetail.canScrollToTop()) assertFalse(nestedWebView.inputResultDetail.canScrollToRight()) @@ -148,7 +148,7 @@ class NestedWebViewTest { fun `GIVEN an instance of InputResultDetail WHEN updateInputResult called THEN it sets whether the touch was handled`() { val nestedWebView = NestedWebView(testContext) - assertTrue(nestedWebView.inputResultDetail.isTouchUnhandled()) + assertTrue(nestedWebView.inputResultDetail.isTouchHandlingUnknown()) nestedWebView.updateInputResult(true) assertTrue(nestedWebView.inputResultDetail.isTouchHandledByBrowser()) diff --git a/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineViewTest.kt b/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineViewTest.kt index e733c3af433..c96d2fb763e 100644 --- a/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineViewTest.kt +++ b/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineViewTest.kt @@ -1588,7 +1588,7 @@ class SystemEngineViewTest { val result = engineView.getInputResultDetail() assertNotNull(result) - assertTrue(result.isTouchUnhandled()) + assertTrue(result.isTouchHandlingUnknown()) assertFalse(result.canScrollToLeft()) assertFalse(result.canScrollToTop()) assertFalse(result.canScrollToRight()) diff --git a/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehavior.kt b/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehavior.kt index b00ce2a653c..167cd030773 100644 --- a/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehavior.kt +++ b/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehavior.kt @@ -201,7 +201,7 @@ class BrowserToolbarBehavior( browserToolbar?.let { toolbar -> if (shouldScroll && startedScroll) { yTranslator.translate(toolbar, distance) - } else { + } else if (engineView?.getInputResultDetail()?.isTouchHandlingUnknown() == false) { // Force expand the toolbar if the user scrolled up, it is not already expanded and // an animation to expand it is not already in progress, // otherwise the user could get stuck in a state where they cannot show the toolbar diff --git a/components/browser/toolbar/src/test/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehaviorTest.kt b/components/browser/toolbar/src/test/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehaviorTest.kt index 060ec739e49..36c72a6d47f 100644 --- a/components/browser/toolbar/src/test/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehaviorTest.kt +++ b/components/browser/toolbar/src/test/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehaviorTest.kt @@ -17,6 +17,7 @@ import com.google.android.material.snackbar.Snackbar import mozilla.components.browser.toolbar.BrowserToolbar import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.EngineView +import mozilla.components.concept.engine.INPUT_UNHANDLED import mozilla.components.concept.engine.InputResultDetail import mozilla.components.concept.engine.selection.SelectionActionDelegate import mozilla.components.support.test.any @@ -504,14 +505,17 @@ class BrowserToolbarBehaviorTest { } @Test - fun `Behavior will forceExpand when scrolling up and !shouldScroll`() { + fun `Behavior will forceExpand when scrolling up and !shouldScroll if the touch was handled in the browser`() { val behavior = spy(BrowserToolbarBehavior(testContext, null, ToolbarPosition.BOTTOM)) val yTranslator: BrowserToolbarYTranslator = mock() behavior.yTranslator = yTranslator behavior.initGesturesDetector(behavior.createGestureDetector()) - doReturn(false).`when`(behavior).shouldScroll val toolbar: BrowserToolbar = spy(BrowserToolbar(testContext, null, 0)) behavior.browserToolbar = toolbar + val engineView: EngineView = mock() + behavior.engineView = engineView + val handledTouchInput = InputResultDetail.newInstance().copy(INPUT_UNHANDLED) + doReturn(handledTouchInput).`when`(engineView).getInputResultDetail() doReturn(100).`when`(toolbar).height doReturn(100f).`when`(toolbar).translationY @@ -526,6 +530,32 @@ class BrowserToolbarBehaviorTest { verify(yTranslator).forceExpandIfNotAlready(toolbar, -30f) } + @Test + fun `Behavior will not forceExpand when scrolling up and !shouldScroll if the touch was not yet handled in the browser`() { + val behavior = spy(BrowserToolbarBehavior(testContext, null, ToolbarPosition.BOTTOM)) + val yTranslator: BrowserToolbarYTranslator = mock() + behavior.yTranslator = yTranslator + behavior.initGesturesDetector(behavior.createGestureDetector()) + val toolbar: BrowserToolbar = spy(BrowserToolbar(testContext, null, 0)) + behavior.browserToolbar = toolbar + val engineView: EngineView = mock() + behavior.engineView = engineView + val handledTouchInput = InputResultDetail.newInstance() + doReturn(handledTouchInput).`when`(engineView).getInputResultDetail() + + doReturn(100).`when`(toolbar).height + doReturn(100f).`when`(toolbar).translationY + + val downEvent = TestUtils.getMotionEvent(ACTION_DOWN, 0f, 0f) + val moveEvent = TestUtils.getMotionEvent(ACTION_MOVE, 0f, 30f, downEvent) + + behavior.onInterceptTouchEvent(mock(), mock(), downEvent) + behavior.onInterceptTouchEvent(mock(), mock(), moveEvent) + + verify(behavior).tryToScrollVertically(-30f) + verify(yTranslator, never()).forceExpandIfNotAlready(toolbar, -30f) + } + @Test fun `onLayoutChild initializes browserToolbar and engineView`() { val toolbarView = BrowserToolbar(testContext) diff --git a/components/concept/engine/src/main/java/mozilla/components/concept/engine/InputResultDetail.kt b/components/concept/engine/src/main/java/mozilla/components/concept/engine/InputResultDetail.kt index 6cf982a94e8..f84dfec221e 100644 --- a/components/concept/engine/src/main/java/mozilla/components/concept/engine/InputResultDetail.kt +++ b/components/concept/engine/src/main/java/mozilla/components/concept/engine/InputResultDetail.kt @@ -7,8 +7,12 @@ package mozilla.components.concept.engine import android.view.MotionEvent import androidx.annotation.VisibleForTesting -// The below top-level values are following the same from [org.mozilla.geckoview.PanZoomController] +/** + * Don't yet have a response from the browser about how the touch was handled. + */ +const val INPUT_HANDLING_UNKNOWN = -1 +// The below top-level values are following the same from [org.mozilla.geckoview.PanZoomController] /** * The content has no scrollable element. * @@ -99,7 +103,7 @@ internal const val OVERSCROLL_DIRECTIONS_VERTICAL = 1 shl 1 * - whether the event can overscroll the page and in what direction. * * @param inputResult Indicates who will use the current [MotionEvent]. - * Possible values: [[INPUT_UNHANDLED], [INPUT_HANDLED], [INPUT_HANDLED_CONTENT]]
. + * Possible values: [[INPUT_HANDLING_UNKNOWN], [INPUT_UNHANDLED], [INPUT_HANDLED], [INPUT_HANDLED_CONTENT]]. * * @param scrollDirections Bitwise ORed value of the directions the page can be scrolled to. * This is the same as GeckoView's [org.mozilla.geckoview.PanZoomController.ScrollableDirections]. @@ -109,7 +113,7 @@ internal const val OVERSCROLL_DIRECTIONS_VERTICAL = 1 shl 1 */ @Suppress("TooManyFunctions") class InputResultDetail private constructor( - val inputResult: Int = INPUT_UNHANDLED, + val inputResult: Int = INPUT_HANDLING_UNKNOWN, val scrollDirections: Int = SCROLL_DIRECTIONS_NONE, val overscrollDirections: Int = OVERSCROLL_DIRECTIONS_NONE ) { @@ -182,6 +186,11 @@ class InputResultDetail private constructor( return InputResultDetail(newValidInputResult!!, newValidScrollDirections!!, newValidOverscrollDirections!!) } + /** + * The [EngineView] has not yet responded on how it handled the [MotionEvent]. + */ + fun isTouchHandlingUnknown() = inputResult == INPUT_HANDLING_UNKNOWN + /** * The [EngineView] handled the last [MotionEvent] to pan or zoom the content. */ @@ -275,6 +284,7 @@ class InputResultDetail private constructor( @VisibleForTesting internal fun getInputResultHandledDescription() = when (inputResult) { + INPUT_HANDLING_UNKNOWN -> INPUT_UNKNOWN_HANDLING_DESCRIPTION INPUT_HANDLED -> INPUT_HANDLED_TOSTRING_DESCRIPTION INPUT_HANDLED_CONTENT -> INPUT_HANDLED_CONTENT_TOSTRING_DESCRIPTION else -> INPUT_UNHANDLED_TOSTRING_DESCRIPTION @@ -340,6 +350,7 @@ class InputResultDetail private constructor( ) @VisibleForTesting internal const val TOSTRING_SEPARATOR = ", " + @VisibleForTesting internal const val INPUT_UNKNOWN_HANDLING_DESCRIPTION = "with unknown handling" @VisibleForTesting internal const val INPUT_HANDLED_TOSTRING_DESCRIPTION = "handled by the browser" @VisibleForTesting internal const val INPUT_HANDLED_CONTENT_TOSTRING_DESCRIPTION = "handled by the website" @VisibleForTesting internal const val INPUT_UNHANDLED_TOSTRING_DESCRIPTION = "unhandled" diff --git a/components/concept/engine/src/test/java/mozilla/components/concept/engine/InputResultDetailTest.kt b/components/concept/engine/src/test/java/mozilla/components/concept/engine/InputResultDetailTest.kt index c5a0605b40a..5113d1f306e 100644 --- a/components/concept/engine/src/test/java/mozilla/components/concept/engine/InputResultDetailTest.kt +++ b/components/concept/engine/src/test/java/mozilla/components/concept/engine/InputResultDetailTest.kt @@ -7,6 +7,7 @@ package mozilla.components.concept.engine import mozilla.components.concept.engine.InputResultDetail.Companion.INPUT_HANDLED_CONTENT_TOSTRING_DESCRIPTION import mozilla.components.concept.engine.InputResultDetail.Companion.INPUT_HANDLED_TOSTRING_DESCRIPTION import mozilla.components.concept.engine.InputResultDetail.Companion.INPUT_UNHANDLED_TOSTRING_DESCRIPTION +import mozilla.components.concept.engine.InputResultDetail.Companion.INPUT_UNKNOWN_HANDLING_DESCRIPTION import mozilla.components.concept.engine.InputResultDetail.Companion.OVERSCROLL_IMPOSSIBLE_TOSTRING_DESCRIPTION import mozilla.components.concept.engine.InputResultDetail.Companion.OVERSCROLL_TOSTRING_DESCRIPTION import mozilla.components.concept.engine.InputResultDetail.Companion.SCROLL_BOTTOM_TOSTRING_DESCRIPTION @@ -33,7 +34,7 @@ class InputResultDetailTest { @Test fun `GIVEN InputResultDetail WHEN newInstance() is called with default parameters THEN a new instance with default values is returned`() { - assertEquals(INPUT_UNHANDLED, inputResultDetail.inputResult) + assertEquals(INPUT_HANDLING_UNKNOWN, inputResultDetail.inputResult) assertEquals(SCROLL_DIRECTIONS_NONE, inputResultDetail.scrollDirections) assertEquals(OVERSCROLL_DIRECTIONS_NONE, inputResultDetail.overscrollDirections) } @@ -41,7 +42,9 @@ class InputResultDetailTest { @Test fun `GIVEN InputResultDetail WHEN newInstance() is called specifying overscroll enabled THEN a new instance with overscroll enabled is returned`() { inputResultDetail = InputResultDetail.newInstance(true) - assertEquals(INPUT_UNHANDLED, inputResultDetail.inputResult) + // Handling unknown but can overscroll. We need to preemptively allow for this, + // otherwise pull to refresh would not work for the entirety of the touch. + assertEquals(INPUT_HANDLING_UNKNOWN, inputResultDetail.inputResult) assertEquals(SCROLL_DIRECTIONS_NONE, inputResultDetail.scrollDirections) assertEquals(OVERSCROLL_DIRECTIONS_VERTICAL, inputResultDetail.overscrollDirections) } @@ -69,11 +72,20 @@ class InputResultDetailTest { @Test fun `GIVEN an InputResultDetail instance WHEN copy is called with new values THEN the invalid ones are filtered out`() { inputResultDetail = inputResultDetail.copy(42, 42, 42) - assertEquals(INPUT_UNHANDLED, inputResultDetail.inputResult) + assertEquals(INPUT_HANDLING_UNKNOWN, inputResultDetail.inputResult) assertEquals(SCROLL_DIRECTIONS_NONE, inputResultDetail.scrollDirections) assertEquals(OVERSCROLL_DIRECTIONS_NONE, inputResultDetail.overscrollDirections) } + @Test + fun `GIVEN an InputResultDetail instance with known touch handling WHEN copy is called with INPUT_HANDLING_UNKNOWN THEN this is not set`() { + inputResultDetail = inputResultDetail.copy(INPUT_HANDLED_CONTENT) + + inputResultDetail = inputResultDetail.copy(INPUT_HANDLING_UNKNOWN) + + assertEquals(INPUT_HANDLED_CONTENT, inputResultDetail.inputResult) + } + @Test fun `GIVEN an InputResultDetail WHEN equals is called with another object of different type THEN it returns false`() { assertFalse(inputResultDetail == Any()) @@ -103,15 +115,15 @@ class InputResultDetailTest { assertTrue(inputResultDetail == inputResultDetail) } - @Test - fun `GIVEN an InputResultDetail WHEN hashCode is called for same values objects THEN it returns the same result`() { - assertEquals(inputResultDetail.hashCode(), inputResultDetail.hashCode()) - - assertEquals(inputResultDetail.hashCode(), InputResultDetail.newInstance().hashCode()) - - inputResultDetail = inputResultDetail.copy(OVERSCROLL_DIRECTIONS_VERTICAL) - assertEquals(inputResultDetail.hashCode(), InputResultDetail.newInstance(true).hashCode()) - } + // @Test + // fun `GIVEN an InputResultDetail WHEN hashCode is called for same values objects THEN it returns the same result`() { + // assertEquals(inputResultDetail.hashCode(), inputResultDetail.hashCode()) + // + // assertEquals(inputResultDetail.hashCode(), InputResultDetail.newInstance().hashCode()) + // + // inputResultDetail = inputResultDetail.copy(OVERSCROLL_DIRECTIONS_VERTICAL) + // assertEquals(inputResultDetail.hashCode(), InputResultDetail.newInstance(true).hashCode()) + // } @Test fun `GIVEN an InputResultDetail WHEN hashCode is called for different values objects THEN it returns different results`() { @@ -148,7 +160,12 @@ class InputResultDetailTest { @Test fun `GIVEN an InputResultDetail WHEN getInputResultHandledDescription is called THEN returns a string describing who will handle the touch`() { - assertEquals(INPUT_UNHANDLED_TOSTRING_DESCRIPTION, inputResultDetail.getInputResultHandledDescription()) + assertEquals(INPUT_UNKNOWN_HANDLING_DESCRIPTION, inputResultDetail.getInputResultHandledDescription()) + + assertEquals( + INPUT_UNHANDLED_TOSTRING_DESCRIPTION, + inputResultDetail.copy(INPUT_UNHANDLED).getInputResultHandledDescription() + ) assertEquals( INPUT_HANDLED_TOSTRING_DESCRIPTION, @@ -231,6 +248,20 @@ class InputResultDetailTest { ) } + @Test + fun `GIVEN an InputResultDetail instance WHEN isTouchHandlingUnknown is called THEN it returns true only if the inputResult is INPUT_HANDLING_UNKNOWN`() { + assertTrue(inputResultDetail.isTouchHandlingUnknown()) + + inputResultDetail = inputResultDetail.copy(INPUT_HANDLED) + assertFalse(inputResultDetail.isTouchHandlingUnknown()) + + inputResultDetail = inputResultDetail.copy(INPUT_HANDLED_CONTENT) + assertFalse(inputResultDetail.isTouchHandlingUnknown()) + + inputResultDetail = inputResultDetail.copy(INPUT_UNHANDLED) + assertFalse(inputResultDetail.isTouchHandlingUnknown()) + } + @Test fun `GIVEN an InputResultDetail instance WHEN isTouchHandledByBrowser is called THEN it returns true only if the inputResult is INPUT_HANDLED`() { assertFalse(inputResultDetail.isTouchHandledByBrowser()) @@ -255,7 +286,7 @@ class InputResultDetailTest { @Test fun `GIVEN an InputResultDetail instance WHEN isTouchUnhandled is called THEN it returns true only if the inputResult is INPUT_UNHANDLED`() { - assertTrue(inputResultDetail.isTouchUnhandled()) + assertFalse(inputResultDetail.isTouchUnhandled()) inputResultDetail = inputResultDetail.copy(INPUT_HANDLED) assertFalse(inputResultDetail.isTouchUnhandled()) diff --git a/docs/changelog.md b/docs/changelog.md index 502585dee34..ab6a49cd3c5 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -11,6 +11,9 @@ permalink: /changelog/ * [Gecko](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt) * [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/.config.yml) +* **browser-toolbar** + * 🚒 Bug fixed [issue #10555](https://github.com/mozilla-mobile/android-components/issues/10555) - Prevent new touches from expanding a collapsed toolbar. Wait until there's a response from GeckoView on how the touch was handled to expand/collapse the toolbar. + * **feature-search** * 🌟️ New `AdsTelemetry` based on a web extension that identify whether there are ads in search results of particular providers for which a (Component.FEATURE_SEARCH to SERP_SHOWN_WITH_ADDS) Fact will be emitted and whether an ad was clicked for which a (Component.FEATURE_SEARCH to SERP_ADD_CLICKED) Fact will be emitted if the `AdsTelemetryMiddleware` is set for `BrowserStore`. * 🌟️ New `InContentTelemetry` based on a web extension that identify follow-on and organic web searches for which a (Component.FEATURE_SEARCH to IN_CONTENT_SEARCH) Fact will be emitted.