Bug 1918698 - Removed the Tablet navigation bar CFR. r=android-review…

…ers,gmalekpour Differential Revision: https://phabricator.services.mozilla.com/D222285
mozilla · Sep 18, 2024 · 7c8638f · 7c8638f
1 parent b05f0ac
commit 7c8638f
Show file tree

Hide file tree

Showing 7 changed files with 5 additions and 159 deletions.
diff --git a/mobile/android/fenix/app/metrics.yaml b/mobile/android/fenix/app/metrics.yaml
@@ -11997,32 +11997,6 @@ address_toolbar:
     notification_emails:
       - [email protected]
     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:
-      - [email protected]
-    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:
-      - [email protected]
-    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
@@ -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/...enix/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenter.kt b/...enix/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
@@ -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
@@ -153,9 +153,6 @@
     <!-- Menu -->
     <string name="pref_key_menu_cfr" translatable="false">pref_key_menu_cfr</string>
 
-    <!-- Tablet Navigation Bar -->
-    <string name="pref_key_tablet_toolbar_navigation_cfr" translatable="false">pref_key_tablet_toolbar_navigation_cfr</string>
-
     <!-- Privacy Pop Window -->
     <string name="pref_key_privacy_pop_window" translatable="false">pref_key_privacy_pop_window</string>
 

diff --git 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 @@
 
     <!-- Tablet navigation bar "contextual feature recommendation" (CFR) -->
     <!-- Text for the title displayed in the contextual feature recommendation popup promoting the tablet navigation bar. -->
-    <string name="tablet_nav_bar_cfr_title">New: one-tap back and forward arrows</string>
+    <string name="tablet_nav_bar_cfr_title" moz:removedIn="132" tools:ignore="UnusedResources">New: one-tap back and forward arrows</string>
     <!-- Text for the message displayed in the contextual feature recommendation popup promoting the tablet navigation bar. -->
-    <string name="tablet_nav_bar_cfr_message">Enjoy quicker navigation that’s always at your fingertips.</string>
+    <string name="tablet_nav_bar_cfr_message" moz:removedIn="132" tools:ignore="UnusedResources">Enjoy quicker navigation that’s always at your fingertips.</string>
 
     <!-- Text for the info dialog when camera permissions have been denied but user tries to access a camera feature. -->
     <string name="camera_permissions_needed_message">Camera access needed. Go to Android settings, tap permissions, and tap allow.</string>

diff --git a/.../app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarCFRPresenterTest.kt b/.../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<View>(R.id.mozac_browser_toolbar_security_indicator) } returns anchor