Commit a64c6849 authored by MozLando's avatar MozLando
Browse files

Merge #8222 #8226

8222: Move and link top sites feature RFC. r=jonalmeida a=pocmo



8226: Closes #8209 - Use new onTouchEventForResult GV api r=pocmo a=Mugurell

Changes:
- previously NestedGV#onTouchEvent event would call onTouchEventForResult where
we would make possible the integration of GV in CoordinatorLayout.
Since NestedGV#onTouchEvent does not anymore call onTouchEventForResult I've
moved all code in onTouchEvent and then call onTouchEventForResult only for
ACTION_DOWN events as snorp recommended.
- we now must wait for onTouchEventForResult#GeckoResult<Int> and only then
call startNestedScroll(..) since otherwise when executing that method in
BrowserToolbarBottomBehavior we would return false since the MotionEvent is
at that time unhandled.
- only dispatch startNestedScroll and dispatchNestedPreScroll if GV returned
INPUT_RESULT_HANDLED.

In testing I saw most of the times GeckoResult resolves in < 5ms with bigger
timeouts showing a logarithmic growth. As such I think the change should be
imperceptible to users.

[video showing the same as before behavior in Fenix](https://drive.google.com/file/d/1WiQ5bXb-dBxYDA03snYSkxApxfhr7hgQ/view?usp=sharing

)
Co-authored-by: default avatarSebastian Kaspari <s.kaspari@gmail.com>
Co-authored-by: default avatarMugurell <Mugurell@users.noreply.github.com>
......@@ -6,7 +6,7 @@ internal object GeckoVersions {
/**
* GeckoView Nightly Version.
*/
const val nightly_version = "81.0.20200820093209"
const val nightly_version = "81.0.20200824094458"
/**
* GeckoView Beta Version.
......
......@@ -10,8 +10,11 @@ import androidx.annotation.VisibleForTesting
import androidx.core.view.NestedScrollingChild
import androidx.core.view.NestedScrollingChildHelper
import androidx.core.view.ViewCompat
import kotlinx.coroutines.launch
import mozilla.components.concept.engine.EngineView
import mozilla.components.support.ktx.android.view.toScope
import org.mozilla.geckoview.GeckoView
import org.mozilla.geckoview.PanZoomController.INPUT_RESULT_HANDLED
import org.mozilla.geckoview.PanZoomController.INPUT_RESULT_UNHANDLED
/**
......@@ -25,7 +28,7 @@ import org.mozilla.geckoview.PanZoomController.INPUT_RESULT_UNHANDLED
* https://github.com/takahirom/webview-in-coordinatorlayout
*/
@Suppress("TooManyFunctions")
@Suppress("TooManyFunctions", "ClickableViewAccessibility")
open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrollingChild {
@VisibleForTesting
......@@ -42,6 +45,8 @@ open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrolli
@VisibleForTesting
internal var childHelper: NestedScrollingChildHelper = NestedScrollingChildHelper(this)
private val coroutineScope = toScope()
/**
* Integer indicating how user's MotionEvent was handled.
*
......@@ -54,21 +59,14 @@ open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrolli
}
@Suppress("ComplexMethod")
override fun onTouchEventForResult(ev: MotionEvent): Int {
override fun onTouchEvent(ev: MotionEvent): Boolean {
val event = MotionEvent.obtain(ev)
val action = ev.actionMasked
val eventY = event.y.toInt()
if (action == MotionEvent.ACTION_DOWN) {
nestedOffsetY = 0
}
// Execute event handler from parent class in all cases
inputResult = handleEvent(event)
when (action) {
MotionEvent.ACTION_MOVE -> {
val allowScroll = !shouldPinOnScreen()
val allowScroll = !shouldPinOnScreen() && inputResult == INPUT_RESULT_HANDLED
var deltaY = lastY - eventY
if (allowScroll && dispatchNestedPreScroll(0, deltaY, scrollConsumed, scrollOffset)) {
......@@ -87,23 +85,46 @@ open class NestedGeckoView(context: Context) : GeckoView(context), NestedScrolli
}
MotionEvent.ACTION_DOWN -> {
// A new gesture started. Reset handled status and ask GV if it can handle this.
inputResult = INPUT_RESULT_UNHANDLED
updateInputResult(event)
nestedOffsetY = 0
lastY = eventY
startNestedScroll(ViewCompat.SCROLL_AXIS_VERTICAL)
// The event should be handled either by onTouchEvent,
// either by onTouchEventForResult, never by both.
// Early return if we sent it to updateInputResult(..) which calls onTouchEventForResult.
event.recycle()
return true
}
// We don't care about other touch events
MotionEvent.ACTION_UP, MotionEvent.ACTION_CANCEL -> stopNestedScroll()
}
// Execute event handler from parent class in all cases
val eventHandled = callSuperOnTouchEvent(event)
// Recycle previously obtained event
event.recycle()
return inputResult
return eventHandled
}
@VisibleForTesting
internal fun callSuperOnTouchEvent(event: MotionEvent): Boolean {
return super.onTouchEvent(event)
}
// Helper function to make testing of this method easier
internal fun handleEvent(event: MotionEvent): Int {
return super.onTouchEventForResult(event)
@VisibleForTesting
internal fun updateInputResult(event: MotionEvent) {
coroutineScope.launch {
super.onTouchEventForResult(event).await()?.let {
inputResult = it
startNestedScroll(ViewCompat.SCROLL_AXIS_VERTICAL)
}
}
}
override fun setNestedScrollingEnabled(enabled: Boolean) {
......
......@@ -6,6 +6,7 @@ package mozilla.components.browser.engine.gecko
import android.app.Activity
import android.content.Context
import android.view.MotionEvent
import android.view.MotionEvent.ACTION_CANCEL
import android.view.MotionEvent.ACTION_DOWN
import android.view.MotionEvent.ACTION_MOVE
......@@ -13,17 +14,20 @@ import android.view.MotionEvent.ACTION_UP
import androidx.core.view.NestedScrollingChildHelper
import androidx.core.view.ViewCompat.SCROLL_AXIS_VERTICAL
import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.support.test.any
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.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.any
import org.mockito.Mockito.anyInt
import org.mockito.Mockito.doReturn
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.robolectric.Robolectric.buildActivity
@RunWith(AndroidJUnit4::class)
......@@ -71,19 +75,29 @@ class NestedGeckoViewTest {
@Test
fun `verify onTouchEvent when ACTION_DOWN`() {
val nestedWebView = NestedGeckoView(context)
val nestedWebView = spy(NestedGeckoView(context))
val mockChildHelper: NestedScrollingChildHelper = mock()
val downEvent = mockMotionEvent(ACTION_DOWN)
val eventCaptor = argumentCaptor<MotionEvent>()
nestedWebView.childHelper = mockChildHelper
nestedWebView.onTouchEvent(mockMotionEvent(ACTION_DOWN))
nestedWebView.onTouchEvent(downEvent)
// We pass a deep copy to `updateInputResult`.
// Can't easily check for equality, `eventTime` should be good enough.
verify(nestedWebView).updateInputResult(eventCaptor.capture())
assertEquals(downEvent.eventTime, eventCaptor.value.eventTime)
verify(mockChildHelper).startNestedScroll(SCROLL_AXIS_VERTICAL)
verify(nestedWebView, times(0)).callSuperOnTouchEvent(any())
}
@Test
fun `verify onTouchEvent when ACTION_MOVE`() {
val nestedWebView = NestedGeckoView(context)
val nestedWebView = spy(NestedGeckoView(context))
val mockChildHelper: NestedScrollingChildHelper = mock()
nestedWebView.childHelper = mockChildHelper
nestedWebView.inputResult = INPUT_RESULT_HANDLED
doReturn(true).`when`(nestedWebView).callSuperOnTouchEvent(any())
doReturn(true).`when`(mockChildHelper).dispatchNestedPreScroll(
anyInt(), anyInt(), any(),
......@@ -104,18 +118,25 @@ class NestedGeckoViewTest {
nestedWebView.onTouchEvent(mockMotionEvent(ACTION_MOVE, y = 10f))
assertEquals(nestedWebView.nestedOffsetY, 6)
assertEquals(nestedWebView.lastY, 6)
// onTouchEventForResult should be called only for ACTION_DOWN
verify(nestedWebView, times(0)).updateInputResult(any())
}
@Test
fun `verify onTouchEvent when ACTION_UP or ACTION_CANCEL`() {
val nestedWebView = NestedGeckoView(context)
val nestedWebView = spy(NestedGeckoView(context))
val mockChildHelper: NestedScrollingChildHelper = mock()
nestedWebView.childHelper = mockChildHelper
doReturn(true).`when`(nestedWebView).callSuperOnTouchEvent(any())
nestedWebView.onTouchEvent(mockMotionEvent(ACTION_UP))
verify(mockChildHelper).stopNestedScroll()
nestedWebView.onTouchEvent(mockMotionEvent(ACTION_CANCEL))
verify(mockChildHelper, times(2)).stopNestedScroll()
// onTouchEventForResult should be called only for ACTION_DOWN
verify(nestedWebView, times(0)).updateInputResult(any())
}
}
\ No newline at end of file
}
......@@ -39,6 +39,7 @@ Before contributing, please review our [Community Participation Guidelines](http
* [0001 - Introducing a lightweight RFC process]({{ site.baseurl }}/rfc/0001-rfc-process)
* [0002 - Moving search state to BrowserState and introducing a SearchMiddleware]({{ site.baseurl }}/rfc/0002-search-state-in-browser-store)
* [0003 - Adding a `concept-base` component]({{ site.baseurl }}/rfc/0003-concept-base-component)
* [0004 - Introducing a Top Sites Feature]({{ site.baseurl }}/rfc/0004-top-sites-feature)
### Presentations
......
---
layout: page
title: Introduce a Top Sites Feature
permalink: /rfc/0006-top-sites-feature
permalink: /rfc/0004-top-sites-feature
---
* Start date: 2020-07-30
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment