From 7811c60399879ee16bc0dfef52b43deea87116ec Mon Sep 17 00:00:00 2001 From: Mugurell Date: Tue, 6 Apr 2021 17:57:40 +0300 Subject: [PATCH] For #10032 - Handle menu touches in expandable menu on Samsung devices Refactored the code so that touches/scrolls are still swallowed while the menu is expanding after which ExpandableLayout should let all MotionEvents pass through to be handled by menu items children. --- .../browser/menu/view/ExpandableLayout.kt | 20 +++-- .../browser/menu/view/ExpandableLayoutTest.kt | 76 +++++++++++-------- docs/changelog.md | 3 + 3 files changed, 62 insertions(+), 37 deletions(-) diff --git a/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt b/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt index d90abe4e633..0f1ce2e3426 100644 --- a/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt +++ b/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt @@ -140,10 +140,6 @@ internal class ExpandableLayout private constructor(context: Context) : FrameLay false // Allow click listeners firing for children. } MotionEvent.ACTION_MOVE -> { - if (isExpandInProgress || !isCollapsed) { - return true - } - if (isScrollingUp(ev)) { expand() true @@ -157,13 +153,23 @@ internal class ExpandableLayout private constructor(context: Context) : FrameLay return callParentOnInterceptTouchEvent(ev) } } + } else { + return if (ev != null && !isTouchingTheWrappedView(ev)) { + // If the menu is expanded but smaller than the parent height + // and the user touches above the menu, in the empty space. + blankTouchListener?.invoke() + true + } else if (isExpandInProgress) { + // Swallow all menu touches while the menu is expanding. + true + } else { + callParentOnInterceptTouchEvent(ev) + } } - - return callParentOnInterceptTouchEvent(ev) } @VisibleForTesting - internal fun shouldInterceptTouches() = isCollapsed || parentHeight > expandedHeight + internal fun shouldInterceptTouches() = isCollapsed && !isExpandInProgress @VisibleForTesting internal fun isTouchingTheWrappedView(ev: MotionEvent): Boolean { diff --git a/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt b/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt index 4c16fe47184..3e352826c2e 100644 --- a/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt +++ b/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt @@ -142,18 +142,54 @@ class ExpandableLayoutTest { } @Test - fun `GIVEN ExpandableLayout WHEN onInterceptTouchEvent is called for an event for which should not be intercepted THEN super is called`() { + fun `GIVEN an expanded menu WHEN onInterceptTouchEvent is called for a touch on the menu THEN super is called`() { + val blankTouchListener = spy {} val expandableLayout = spy(ExpandableLayout.wrapContentInExpandableView( - FrameLayout(testContext), 1) { } + FrameLayout(testContext), 1, blankTouchListener) ) val event: MotionEvent = mock() doReturn(false).`when`(expandableLayout).shouldInterceptTouches() + doReturn(true).`when`(expandableLayout).isTouchingTheWrappedView(any()) expandableLayout.onInterceptTouchEvent(event) + verify(blankTouchListener, never()).invoke() verify(expandableLayout).callParentOnInterceptTouchEvent(event) } + @Test + fun `GIVEN a menu currently expanding WHEN onInterceptTouchEvent is called for a touch on the menu THEN the touch is swallowed`() { + val blankTouchListener = spy {} + val expandableLayout = spy(ExpandableLayout.wrapContentInExpandableView( + FrameLayout(testContext), 1, blankTouchListener) + ) + val event: MotionEvent = mock() + doReturn(false).`when`(expandableLayout).shouldInterceptTouches() + doReturn(true).`when`(expandableLayout).isTouchingTheWrappedView(any()) + expandableLayout.isExpandInProgress = true + + expandableLayout.onInterceptTouchEvent(event) + + verify(blankTouchListener, never()).invoke() + verify(expandableLayout, never()).callParentOnInterceptTouchEvent(event) + } + + @Test + fun `GIVEN an expanded menu WHEN onInterceptTouchEvent is called for a outside the menu THEN super blankTouchListener is invoked`() { + val blankTouchListener = spy {} + val expandableLayout = spy(ExpandableLayout.wrapContentInExpandableView( + FrameLayout(testContext), 1, blankTouchListener) + ) + val event: MotionEvent = mock() + doReturn(false).`when`(expandableLayout).shouldInterceptTouches() + doReturn(false).`when`(expandableLayout).isTouchingTheWrappedView(any()) + + expandableLayout.onInterceptTouchEvent(event) + + verify(blankTouchListener).invoke() + verify(expandableLayout, never()).callParentOnInterceptTouchEvent(event) + } + @Test fun `GIVEN ExpandableLayout WHEN onInterceptTouchEvent is called for ACTION_CANCEL or ACTION_UP THEN the events are not intercepted`() { val expandableLayout = spy(ExpandableLayout.wrapContentInExpandableView( @@ -246,23 +282,8 @@ class ExpandableLayoutTest { FrameLayout(testContext), 1) { } ) val actionDown = MotionEvent.obtain(0, 0, ACTION_MOVE, 0f, 0f, 0) - doReturn(true).`when`(expandableLayout).shouldInterceptTouches() - expandableLayout.isExpandInProgress = true - - assertTrue(expandableLayout.onInterceptTouchEvent(actionDown)) - verify(expandableLayout, never()).expand() - verify(expandableLayout, never()).callParentOnInterceptTouchEvent(any()) - } - - @Test - fun `GIVEN the wrappedView is expanded but smaller than the parent WHEN onInterceptTouchEvent is called for scroll events THEN these events are intercepted`() { - val expandableLayout = spy(ExpandableLayout.wrapContentInExpandableView( - FrameLayout(testContext), 1) { } - ) - val actionDown = MotionEvent.obtain(0, 0, ACTION_MOVE, 0f, 0f, 0) - doReturn(true).`when`(expandableLayout).shouldInterceptTouches() - expandableLayout.isExpandInProgress = false - expandableLayout.isCollapsed = false + doReturn(false).`when`(expandableLayout).shouldInterceptTouches() + doReturn(false).`when`(expandableLayout).isTouchingTheWrappedView(any()) assertTrue(expandableLayout.onInterceptTouchEvent(actionDown)) verify(expandableLayout, never()).expand() @@ -277,7 +298,6 @@ class ExpandableLayoutTest { val actionDown = MotionEvent.obtain(0, 0, ACTION_MOVE, 0f, 0f, 0) doReturn(true).`when`(expandableLayout).shouldInterceptTouches() doReturn(false).`when`(expandableLayout).isScrollingUp(any()) - expandableLayout.isExpandInProgress = false assertFalse(expandableLayout.onInterceptTouchEvent(actionDown)) verify(expandableLayout, never()).expand() @@ -292,7 +312,6 @@ class ExpandableLayoutTest { val actionDown = MotionEvent.obtain(0, 0, ACTION_MOVE, 0f, 0f, 0) doReturn(true).`when`(expandableLayout).shouldInterceptTouches() doReturn(true).`when`(expandableLayout).isScrollingUp(any()) - expandableLayout.isExpandInProgress = false assertTrue(expandableLayout.onInterceptTouchEvent(actionDown)) verify(expandableLayout).expand() @@ -340,27 +359,24 @@ class ExpandableLayoutTest { } @Test - fun `GIVEN ExpandableLayout not collapsed and the wrappedView smaller than it's parent WHEN shouldInterceptTouches is called THEN it returns true`() { + fun `GIVEN ExpandableLayout not collapsed WHEN shouldInterceptTouches is called THEN it returns false`() { val expandableLayout = ExpandableLayout.wrapContentInExpandableView( FrameLayout(testContext), 1 ) { } expandableLayout.isCollapsed = false - expandableLayout.parentHeight = 101 - expandableLayout.expandedHeight = 100 - assertTrue(expandableLayout.shouldInterceptTouches()) + assertFalse(expandableLayout.shouldInterceptTouches()) } @Test - fun `GIVEN ExpandableLayout not collapsed and the wrappedView bigger than it's parent WHEN shouldInterceptTouches is called THEN it returns false`() { + fun `GIVEN ExpandableLayout currently expanding WHEN shouldInterceptTouches is called THEN it returns false`() { val expandableLayout = ExpandableLayout.wrapContentInExpandableView( FrameLayout(testContext), 1 ) { } - expandableLayout.isCollapsed = false - expandableLayout.parentHeight = 101 - expandableLayout.expandedHeight = 102 + expandableLayout.isCollapsed = true + expandableLayout.isExpandInProgress = false - assertFalse(expandableLayout.shouldInterceptTouches()) + assertTrue(expandableLayout.shouldInterceptTouches()) } @Test diff --git a/docs/changelog.md b/docs/changelog.md index b151f4f3722..2e25dcd6746 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -12,6 +12,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-menu**: + * 🚒 Bug fixed [issue #](https://github.com/mozilla-mobile/android-components/issues/10032) - Fix a recent issue with ExpandableLayout - user touches on an expanded menu might not have any effect on Samsung devices. + * **browser-menu**: * 🚒 Bug fixed [issue #](https://github.com/mozilla-mobile/android-components/issues/10005) - Fix a recent issue with BrowserMenu#show() - endOfMenuAlwaysVisible not being applied.