From 7c8638f5174c86a1d5ebdbd586c6c8df6e193fe6 Mon Sep 17 00:00:00 2001 From: t-p-white Date: Wed, 18 Sep 2024 09:43:42 +0000 Subject: [PATCH] Bug 1918698 - Removed the Tablet navigation bar CFR. r=android-reviewers,gmalekpour Differential Revision: https://phabricator.services.mozilla.com/D222285 --- mobile/android/fenix/app/metrics.yaml | 26 ------- .../fenix/browser/BaseBrowserFragment.kt | 4 +- .../toolbar/BrowserToolbarCFRPresenter.kt | 78 +------------------ .../java/org/mozilla/fenix/utils/Settings.kt | 8 -- .../src/main/res/values/preference_keys.xml | 3 - .../fenix/app/src/main/res/values/strings.xml | 4 +- .../toolbar/BrowserToolbarCFRPresenterTest.kt | 41 ---------- 7 files changed, 5 insertions(+), 159 deletions(-) diff --git a/mobile/android/fenix/app/metrics.yaml b/mobile/android/fenix/app/metrics.yaml index 18342e280253dd..c20de70ae670ba 100644 --- a/mobile/android/fenix/app/metrics.yaml +++ b/mobile/android/fenix/app/metrics.yaml @@ -11997,32 +11997,6 @@ address_toolbar: notification_emails: - android-probes@mozilla.com expires: never - tablet_navigation_cfr_shown: - type: event - description: | - Tablet navigation bar CFR was shown to the user. - bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1908425 - data_reviews: - - https://phabricator.services.mozilla.com/D218373 - data_sensitivity: - - interaction - notification_emails: - - android-probes@mozilla.com - expires: never - tablet_navigation_cfr_dismissed: - type: event - description: | - A user has dismissed the tablet navigation bar CFR. - bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1908425 - data_reviews: - - https://phabricator.services.mozilla.com/D218373 - data_sensitivity: - - interaction - notification_emails: - - android-probes@mozilla.com - expires: never customization_settings: dynamic_toolbar: diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt index 6b3c45f909a8b7..f4220db1b14ba7 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt @@ -1503,9 +1503,7 @@ abstract class BaseBrowserFragment : content = { FirefoxTheme { Column { - if (!activity.isMicrosurveyPromptDismissed.value && - !(context.isTablet() && context.settings().shouldShowTabletNavigationCFR) - ) { + if (!activity.isMicrosurveyPromptDismissed.value) { currentMicrosurvey?.let { if (isToolbarAtBottom) { updateBrowserToolbarForMicrosurveyPrompt(browserToolbar) diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt index b6c218a3ce1930..f19f22a5008a08 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt @@ -46,13 +46,11 @@ import mozilla.components.concept.engine.EngineSession.CookieBannerHandlingStatu import mozilla.components.lib.state.ext.flowScoped import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifAnyChanged import mozilla.telemetry.glean.private.NoExtras -import org.mozilla.fenix.GleanMetrics.AddressToolbar import org.mozilla.fenix.GleanMetrics.CookieBanners import org.mozilla.fenix.GleanMetrics.Shopping import org.mozilla.fenix.GleanMetrics.TrackingProtection import org.mozilla.fenix.R import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.isTablet import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.settings.SupportUtils.SumoTopic.TOTAL_COOKIE_PROTECTION import org.mozilla.fenix.shopping.DefaultShoppingExperienceFeature @@ -85,7 +83,7 @@ internal var CFR_MINIMUM_NUMBER_OPENED_TABS = 5 * @param onShoppingCfrDisplayed Triggered when CFR is displayed to the user. * @param shoppingExperienceFeature Used to determine if [ShoppingExperienceFeature] is enabled. */ -@Suppress("LongParameterList", "LongMethod") +@Suppress("LongParameterList") class BrowserToolbarCFRPresenter( private val context: Context, private val browserStore: BrowserStore, @@ -177,23 +175,6 @@ class BrowserToolbarCFRPresenter( } } - ToolbarCFR.TABLET_NAVIGATION -> { - scope = browserStore.flowScoped { flow -> - flow - .mapNotNull { it.selectedTab } - .map { it.content.progress } - .transformWhile { progress -> - emit(progress) - progress != 100 - } - .filter { popup == null && it == 100 } - .collect { - scope?.cancel() - showTabletNavigationCFR() - } - } - } - ToolbarCFR.NONE -> { // no-op } @@ -242,10 +223,6 @@ class BrowserToolbarCFRPresenter( ToolbarCFR.COOKIE_BANNERS } - context.isTablet() && - settings.shouldShowTabletNavigationCFR && - settings.navigationToolbarEnabled -> ToolbarCFR.TABLET_NAVIGATION - shoppingExperienceFeature.isEnabled && settings.shouldShowReviewQualityCheckCFR -> whichShoppingCFR() @@ -504,62 +481,11 @@ class BrowserToolbarCFRPresenter( show() } } - - @VisibleForTesting - @Suppress("LongMethod") - internal fun showTabletNavigationCFR() { - CFRPopup( - anchor = toolbar.findViewById( - R.id.mozac_browser_toolbar_navigation_actions, - ), - properties = CFRPopupProperties( - popupAlignment = INDICATOR_CENTERED_IN_ANCHOR, - popupBodyColors = listOf( - getColor(context, R.color.fx_mobile_layer_color_gradient_end), - getColor(context, R.color.fx_mobile_layer_color_gradient_start), - ), - popupVerticalOffset = CFR_TO_ANCHOR_VERTICAL_PADDING.dp, - dismissButtonColor = getColor(context, R.color.fx_mobile_icon_color_oncolor), - indicatorDirection = if (settings.toolbarPosition == ToolbarPosition.TOP) { - CFRPopup.IndicatorDirection.UP - } else { - CFRPopup.IndicatorDirection.DOWN - }, - ), - onDismiss = { - AddressToolbar.tabletNavigationCfrDismissed.record(NoExtras()) - settings.shouldShowTabletNavigationCFR = false - settings.lastCfrShownTimeInMillis = System.currentTimeMillis() - }, - title = { - FirefoxTheme { - Text( - text = context.getString(R.string.tablet_nav_bar_cfr_title), - color = FirefoxTheme.colors.textOnColorPrimary, - style = FirefoxTheme.typography.subtitle2, - ) - } - }, - text = { - FirefoxTheme { - Text( - text = context.getString(R.string.tablet_nav_bar_cfr_message), - color = FirefoxTheme.colors.textOnColorPrimary, - style = FirefoxTheme.typography.body2, - ) - } - }, - ).run { - AddressToolbar.tabletNavigationCfrShown.record(NoExtras()) - popup = this - show() - } - } } /** * The CFR to be shown in the toolbar. */ private enum class ToolbarCFR { - TCP, SHOPPING, SHOPPING_OPTED_IN, ERASE, COOKIE_BANNERS, TABLET_NAVIGATION, NONE + TCP, SHOPPING, SHOPPING_OPTED_IN, ERASE, COOKIE_BANNERS, NONE } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 4cc8355b068426..d9fee8ba2b78cc 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -1780,14 +1780,6 @@ class Settings(private val appContext: Context) : PreferencesHolder { default = true, ) - /** - * Indicates if Tablet's navigation address bar buttons CFR should be displayed to the user. - */ - var shouldShowTabletNavigationCFR by booleanPreference( - key = appContext.getPreferenceKey(R.string.pref_key_tablet_toolbar_navigation_cfr), - default = true, - ) - /** * Indicates if the menu CFR should be displayed to the user. */ diff --git a/mobile/android/fenix/app/src/main/res/values/preference_keys.xml b/mobile/android/fenix/app/src/main/res/values/preference_keys.xml index 20853ea121d19e..151b8d813296aa 100644 --- a/mobile/android/fenix/app/src/main/res/values/preference_keys.xml +++ b/mobile/android/fenix/app/src/main/res/values/preference_keys.xml @@ -153,9 +153,6 @@ pref_key_menu_cfr - - pref_key_tablet_toolbar_navigation_cfr - pref_key_privacy_pop_window diff --git a/mobile/android/fenix/app/src/main/res/values/strings.xml b/mobile/android/fenix/app/src/main/res/values/strings.xml index ef5cf04a8eca6b..427a86f51e6db0 100644 --- a/mobile/android/fenix/app/src/main/res/values/strings.xml +++ b/mobile/android/fenix/app/src/main/res/values/strings.xml @@ -116,9 +116,9 @@ - New: one-tap back and forward arrows + New: one-tap back and forward arrows - Enjoy quicker navigation that’s always at your fingertips. + Enjoy quicker navigation that’s always at your fingertips. Camera access needed. Go to Android settings, tap permissions, and tap allow. diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt index 61def52fec5a58..d5a1961dc46e2c 100644 --- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt +++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt @@ -221,7 +221,6 @@ class BrowserToolbarCFRPresenterTest { every { shouldShowTotalCookieProtectionCFR } returns false every { shouldShowReviewQualityCheckCFR } returns false every { shouldShowEraseActionCFR } returns false - every { shouldShowTabletNavigationCFR } returns false }, ) @@ -471,42 +470,6 @@ class BrowserToolbarCFRPresenterTest { verify(exactly = 1) { presenter.showShoppingCFR(false) } } - @Test - fun `GIVEN using a tablet and haven't seen the navigation buttons CFR before WHEN the page is fully loaded THEN show the navigation buttons CFR`() { - val tab = createTab(url = "") - val browserStore = createBrowserStore( - tab = tab, - selectedTabId = tab.id, - ) - - val presenter = createPresenterThatShowsCFRs( - context = mockk { - every { isTablet() } returns true - }, - browserStore = browserStore, - settings = mockk { - every { shouldShowTotalCookieProtectionCFR } returns false - every { shouldShowEraseActionCFR } returns false - every { shouldShowReviewQualityCheckCFR } returns false - every { shouldShowTabletNavigationCFR } returns true - every { navigationToolbarEnabled } returns true - }, - ) - - presenter.start() - - assertNotNull(presenter.scope) - - browserStore.dispatch(ContentAction.UpdateProgressAction(tab.id, 14)).joinBlocking() - verify(exactly = 0) { presenter.showTabletNavigationCFR() } - - browserStore.dispatch(ContentAction.UpdateProgressAction(tab.id, 99)).joinBlocking() - verify(exactly = 0) { presenter.showTabletNavigationCFR() } - - browserStore.dispatch(ContentAction.UpdateProgressAction(tab.id, 100)).joinBlocking() - verify { presenter.showTabletNavigationCFR() } - } - /** * Creates and return a [spyk] of a [BrowserToolbarCFRPresenter] that can handle actually showing CFRs. */ @@ -521,7 +484,6 @@ class BrowserToolbarCFRPresenterTest { every { openTabsCount } returns 5 every { shouldShowReviewQualityCheckCFR } returns false every { shouldShowEraseActionCFR } returns false - every { shouldShowTabletNavigationCFR } returns false }, toolbar: BrowserToolbar = mockk(), isPrivate: Boolean = false, @@ -530,7 +492,6 @@ class BrowserToolbarCFRPresenterTest { every { showTcpCfr() } just Runs every { showShoppingCFR(any()) } just Runs every { showEraseCfr() } just Runs - every { showTabletNavigationCFR() } just Runs } /** @@ -542,7 +503,6 @@ class BrowserToolbarCFRPresenterTest { every { getString(R.string.tcp_cfr_message) } returns "Test" every { getColor(any()) } returns 0 every { getString(R.string.pref_key_should_show_review_quality_cfr) } returns "test" - every { isTablet() } returns false }, anchor: View = mockk(relaxed = true), browserStore: BrowserStore = mockk(), @@ -552,7 +512,6 @@ class BrowserToolbarCFRPresenterTest { every { openTabsCount } returns 5 every { shouldShowCookieBannersCFR } returns true every { shouldShowReviewQualityCheckCFR } returns true - every { shouldShowTabletNavigationCFR } returns true }, toolbar: BrowserToolbar = mockk { every { findViewById(R.id.mozac_browser_toolbar_security_indicator) } returns anchor