From 48ae7f0a8d8e739bc33cdc701ae08d45c8c104cb Mon Sep 17 00:00:00 2001 From: Mugurell Date: Thu, 6 Oct 2022 16:04:34 +0300 Subject: [PATCH] For #27016 - Ensure smooth search UX after the MR onboarding is closed If the app is opened from the search widget and the MR onboarding is shown then the backstack will have the following structure: - root, homeFragment, searchDialogFragment, onboardingFragment as opposed to otherwise - root, homeFragment, searchDialogFragment. This patch allows to avoid the MR onboarding fragment causing the SearchDialogFragment to not know that below it is the HomeFragment and consequently not applying transparency or propagate user touches to the parent Activity. --- .../fenix/search/SearchDialogFragment.kt | 42 ++++++++- .../fenix/search/SearchDialogFragmentTest.kt | 92 +++++++++++++++++++ 2 files changed, 130 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/org/mozilla/fenix/search/SearchDialogFragmentTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt b/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt index 2eb301a3340d..6b74da3d1781 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt @@ -5,6 +5,7 @@ package org.mozilla.fenix.search import android.Manifest +import android.annotation.SuppressLint import android.app.Activity import android.app.Dialog import android.content.Context @@ -24,6 +25,7 @@ import android.view.ViewStub import android.view.WindowManager import android.view.accessibility.AccessibilityEvent import android.view.inputmethod.InputMethodManager +import androidx.annotation.VisibleForTesting import androidx.appcompat.app.AlertDialog import androidx.appcompat.app.AppCompatDialogFragment import androidx.appcompat.content.res.AppCompatResources @@ -35,9 +37,10 @@ import androidx.core.graphics.drawable.toDrawable import androidx.core.net.toUri import androidx.core.view.isVisible import androidx.lifecycle.lifecycleScope +import androidx.navigation.NavBackStackEntry +import androidx.navigation.NavGraph import androidx.navigation.fragment.findNavController import androidx.navigation.fragment.navArgs -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch import mozilla.components.browser.domains.autocomplete.ShippedDomainsProvider @@ -202,7 +205,7 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { ) val fromHomeFragment = - findNavController().previousBackStackEntry?.destination?.id == R.id.homeFragment + getPreviousDestination()?.destination?.id == R.id.homeFragment toolbarView = ToolbarView( requireContext(), @@ -252,7 +255,7 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { requireComponents.core.engine.speculativeCreateSession(isPrivate) - when (findNavController().previousBackStackEntry?.destination?.id) { + when (getPreviousDestination()?.destination?.id) { R.id.homeFragment -> { // When displayed above home, dispatches the touch events to scrim area to the HomeFragment binding.searchWrapper.background = ColorDrawable(Color.TRANSPARENT) @@ -301,7 +304,7 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { setupConstraints(view) // When displayed above browser or home screen, dismisses keyboard when touching scrim area - when (findNavController().previousBackStackEntry?.destination?.id) { + when (getPreviousDestination()?.destination?.id) { R.id.browserFragment, R.id.homeFragment -> { binding.searchWrapper.setOnTouchListener { _, _ -> binding.searchWrapper.hideKeyboard() @@ -904,6 +907,37 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { } } + /** + * Gets the previous visible [NavBackStackEntry]. + * This skips over any [NavBackStackEntry] that is associated with a [NavGraph] or refers to this + * class as a navigation destination. + */ + @VisibleForTesting + @SuppressLint("RestrictedApi") + internal fun getPreviousDestination(): NavBackStackEntry? { + // This duplicates the platform functionality for "previousBackStackEntry" but additionally skips this entry. + + val descendingEntries = findNavController().backStack.descendingIterator() + // Throw the topmost destination away. + if (descendingEntries.hasNext()) { + descendingEntries.next() + } + + while (descendingEntries.hasNext()) { + val entry = descendingEntries.next() + // Using the canonicalName is safer - see https://github.com/mozilla-mobile/android-components/pull/10810 + // simpleName is used as a backup to avoid the not null assertion (!!) operator. + val currentClassName = this::class.java.canonicalName?.substringAfterLast('.') + ?: this::class.java.simpleName + + // Throw this entry away if it's the current top and ignore returning the base nav graph. + if (entry.destination !is NavGraph && !entry.destination.displayName.contains(currentClassName, true)) { + return entry + } + } + return null + } + companion object { private const val TAP_INCREASE_DPS = 8 private const val QR_FRAGMENT_TAG = "MOZAC_QR_FRAGMENT" diff --git a/app/src/test/java/org/mozilla/fenix/search/SearchDialogFragmentTest.kt b/app/src/test/java/org/mozilla/fenix/search/SearchDialogFragmentTest.kt new file mode 100644 index 000000000000..0c32f0117ea1 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/search/SearchDialogFragmentTest.kt @@ -0,0 +1,92 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.search + +import androidx.fragment.app.Fragment +import androidx.navigation.NavBackStackEntry +import androidx.navigation.NavController +import androidx.navigation.fragment.findNavController +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.unmockkStatic +import org.junit.After +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Before +import org.junit.Test +import java.util.ArrayDeque + +internal class SearchDialogFragmentTest { + private val navController: NavController = mockk() + private val fragment = SearchDialogFragment() + + @Before + fun setup() { + mockkStatic("androidx.navigation.fragment.FragmentKt") + every { any().findNavController() } returns navController + } + + @After + fun teardown() { + unmockkStatic("androidx.navigation.fragment.FragmentKt") + } + + @Test + fun `GIVEN this is the only visible fragment WHEN asking for the previous destination THEN return null`() { + every { navController.backStack } returns ArrayDeque(listOf(getDestination(fragmentName))) + + assertNull(fragment.getPreviousDestination()) + } + + @Test + fun `GIVEN this and FragmentB on top of this are visible WHEN asking for the previous destination THEN return null`() { + every { navController.backStack } returns ArrayDeque( + listOf( + getDestination(fragmentName), + getDestination("FragmentB"), + ), + ) + + assertNull(fragment.getPreviousDestination()) + } + + @Test + fun `GIVEN FragmentA, this and FragmentB are visible WHEN asking for the previous destination THEN return FragmentA`() { + val fragmentADestination = getDestination("FragmentA") + every { navController.backStack } returns ArrayDeque( + listOf( + fragmentADestination, + getDestination(fragmentName), + getDestination("FragmentB"), + ), + ) + + assertSame(fragmentADestination, fragment.getPreviousDestination()) + } + + @Test + fun `GIVEN FragmentA and this on top of it are visible WHEN asking for the previous destination THEN return FragmentA`() { + val fragmentADestination = getDestination("FragmentA") + every { navController.backStack } returns ArrayDeque( + listOf( + fragmentADestination, + getDestination(fragmentName), + ), + ) + + assertSame(fragmentADestination, fragment.getPreviousDestination()) + } +} + +private val fragmentName = SearchDialogFragment::class.java.canonicalName?.substringAfterLast('.')!! + +private fun getDestination(destinationName: String): NavBackStackEntry { + return mockk { + every { destination } returns mockk { + every { displayName } returns "test.id/$destinationName" + } + } +}