For #7503 - Show Start browsing cfr at fresh install #7705

iorgamgabriel · 2022-09-21T12:04:45Z

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

QA Needed

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to build-focus-debug or build-klar-debug for changes targeting Klar,
click View task in Taskcluster,
click the Artifacts row,
click to download any of the apks listed here which use an appropriate name for each CPU architecture.

GitHub Automation

Fixes #7503

Mugurell · 2022-10-24T11:47:34Z

app/src/test/java/org/mozilla/focus/browser/integration/InputToolbarIntegrationTest.kt

+    fun `GIVEN app fresh install WHEN input toolbar integration is starting THEN start browsing scope is populated`() {
+        inputToolbarIntegration.start()
+
+        assertNotEquals(null, inputToolbarIntegration.startBrowsingCfrScope)


Suggested change

assertNotEquals(null, inputToolbarIntegration.startBrowsingCfrScope)

assertNotNull(inputToolbarIntegration.startBrowsingCfrScope)

nit: We could verify that the scope was null before the inputToolbarIntegration.start() call.

Mugurell · 2022-10-24T12:12:36Z

app/src/test/java/org/mozilla/focus/browser/integration/InputToolbarIntegrationTest.kt

+    fun `GIVEN app fresh install WHEN input toolbar integration is stoping THEN start browsing scope is canceled`() {
+        inputToolbarIntegration.stop()
+
+        assertEquals(null, inputToolbarIntegration.startBrowsingCfrScope?.cancel())


Here it looks like we are asserting that startBrowsingCfrScope?.cancel() returns null.
If we need to test that inputToolbarIntegration.stop() stopped the scope we could use something similar to

assertFalse(inputToolbarIntegration.startBrowsingCfrScope?.isActive ?: true)

in which case a previous

assertTrue(inputToolbarIntegration.startBrowsingCfrScope?.isActive ?: false)

would also help adding mroe confidence in the assert.

Mugurell · 2022-10-24T12:27:29Z

app/src/test/java/org/mozilla/focus/browser/integration/InputToolbarIntegrationTest.kt

+            shippedDomainsProvider,
+            customDomainProviderResult,


nit: Seems like these are simple mocks needed just to be able to build InputToolbarIntegration. They could be initialized in place with mock() calls.

Mugurell · 2022-10-24T12:28:27Z

app/src/test/java/org/mozilla/focus/browser/integration/InputToolbarIntegrationTest.kt

+
+    @Before
+    fun setUp() {
+        Dispatchers.setMain(testDispatcher)


Not sure why this is needed (together with UnconfinedTestDispatcher and code from onAfter)?

Mugurell · 2022-10-24T12:31:09Z

app/src/main/res/values/strings.xml

@@ -964,6 +964,9 @@
    !-- This is the title of promote search widget dialog. -->
    <string name="promote_search_widget_dialog_title">Browsing history cleared! 🎉</string>

+    <!-- CFR for Start your private browsing session-->


Generally it is preferred to avoid acronyms. Maybe localizers don't know these internal namings.
Would be useful to reformulate this to avoid the CFR or at least offer more context. I see for other such strings that the comment contains (Banner Info Message).

Mugurell · 2022-10-24T12:34:54Z

app/src/main/java/org/mozilla/focus/state/AppReducer.kt

@@ -187,6 +188,13 @@ private fun showEraseTabsCfrChanged(state: AppState, action: AppAction.ShowErase
    return state.copy(showEraseTabsCfr = action.value)
 }

+/**
+ * The state of start browsing  CFR changed


nit: This comment provides little useful info. If a comment is really needed "Update whether the start browsing CFR should be shown or not" may be just a little bit better.
I see that there are many comments which follow this structure so maybe a separate ticket to update the documentation for all these methods would be nice for a good first issue.

Mugurell · 2022-10-24T12:36:08Z

app/src/main/java/org/mozilla/focus/input/InputToolbarIntegration.kt

@@ -105,9 +123,60 @@ class InputToolbarIntegration(
    override fun start() {
        useCustomDomainProvider = settings.shouldAutocompleteFromCustomDomainList()
        useShippedDomainProvider = settings.shouldAutocompleteFromShippedDomainList()
+        observeStartBrowserCfrVisibility()


nit: Since the new CFR is to be shown just once we could avoid always observing the store with another if (showStartBrowsingCfr) check here.

Mugurell · 2022-10-24T12:38:47Z

app/src/main/java/org/mozilla/focus/browser/integration/BrowserToolbarIntegration.kt

+                                    fontSize = 16.sp,
+                                    letterSpacing = 0.5.sp,
+                                    lineHeight = 24.sp,


Seems like this configuration is common to all CFRs.
Could it be extracted in a new typography?

Mugurell

LGTM with one question about the dismiss behavior and a few proposed changes about the cfr text typographies.

Mugurell · 2022-10-25T10:29:26Z

app/src/main/java/org/mozilla/focus/input/InputToolbarIntegration.kt

+                            onDismiss = { onDismissStartBrowsingCfr() },
+                            text = {
+                                Text(
+                                    style = focusTypography.cfrTextStyle,


nit: This style should be applied to all CFRs.

Mugurell · 2022-10-25T10:32:22Z

app/src/test/java/org/mozilla/focus/browser/integration/InputToolbarIntegrationTest.kt

+/* 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.focus.browser.integration


nit: missing newline between this and the license text.

Mugurell · 2022-10-25T10:39:51Z

app/src/main/java/org/mozilla/focus/ui/theme/FocusTypography.kt

@@ -121,4 +122,10 @@ val focusTypography: FocusTypography
            fontSize = 14.sp,
            color = PhotonColors.LightGrey05,
        ),
+        cfrTextStyle = TextStyle(
+            fontFamily = metropolis,


I think this font should be removed. The other CFRs which don't yet use this typography don't have it declared.
After applying the new patch this start browsing CFR looks different than expected.

Mugurell · 2022-10-25T10:47:50Z

app/src/main/java/org/mozilla/focus/input/InputToolbarIntegration.kt

+                                ),
+                                popupVerticalOffset = 0.dp,
+                            ),
+                            onDismiss = { onDismissStartBrowsingCfr() },


The onDismiss callback informs of the user taped on the X button or not - maybe the CFR was dismissed by pressing outside of it.
Do we need to consider these separate scenarios?

Done . Based on this bug #7653 cfrs should not be dismissed if the users taps anywhere on the screen.

lobontiumira · 2022-10-27T08:35:10Z

Verified as implemented on a local Focus Nightly build 108.0a1 with the following devices:

Google Pixel 6 (Android 13),
Lenovo tablet M10 (Android 10),
Oppo Reno 6 (Android 12),
Samsung Galaxy Note 8 (Android 9), and
Xiaomi mi4i (Android 5.0.2).

After dismissing the new onboarding, the start browsing CFR is displayed.
Rotating the device, dismisses the CFR. But, after sending the app in the background, and re-opening it, the CFR is displayed again.
Tapping the "X" button from the top-right corner of the CFR, dismisses the CFR.

iorgamgabriel · 2022-10-27T09:30:14Z

@Mergifyio backport releases_v107.0

mergify · 2022-10-27T09:30:54Z

backport releases_v107.0

✅ Backports have been created

#7928 For #7503 - Show Start browsing cfr at fresh install (backport #7705) has been created for branch releases_v107.0

iorgamgabriel marked this pull request as draft September 21, 2022 12:04

iorgamgabriel force-pushed the 7503 branch from 2bb4c8b to 0279e57 Compare September 21, 2022 13:22

iorgamgabriel force-pushed the 7503 branch from 0279e57 to 5d8cc03 Compare September 30, 2022 14:01

Mugurell mentioned this pull request Oct 18, 2022

For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects mozilla-mobile/android-components#12828

Merged

4 tasks

iorgamgabriel force-pushed the 7503 branch 2 times, most recently from 61ccf4d to 08dc7d6 Compare October 19, 2022 13:15

Mugurell reviewed Oct 24, 2022

View reviewed changes

iorgamgabriel force-pushed the 7503 branch from 08dc7d6 to dcf5d8e Compare October 24, 2022 15:08

iorgamgabriel marked this pull request as ready for review October 25, 2022 07:43

iorgamgabriel force-pushed the 7503 branch 2 times, most recently from 5b19511 to e654619 Compare October 25, 2022 08:52

iorgamgabriel requested a review from Mugurell October 25, 2022 10:03

Mugurell approved these changes Oct 25, 2022

View reviewed changes

iorgamgabriel force-pushed the 7503 branch 3 times, most recently from 7cdadc4 to 0b7c73e Compare October 25, 2022 12:06

iorgamgabriel added 🛬 needs landing PRs that are ready to land and removed 🛬 needs landing PRs that are ready to land labels Oct 25, 2022

iorgamgabriel force-pushed the 7503 branch 3 times, most recently from b77c489 to 35fa6cc Compare October 26, 2022 06:24

For mozilla-mobile#7503 - Show Start browsing cfr at fresh install

e300109

iorgamgabriel force-pushed the 7503 branch from 35fa6cc to e300109 Compare October 26, 2022 07:34

iorgamgabriel added the 🛬 needs landing PRs that are ready to land label Oct 26, 2022

mergify bot merged commit a0a0916 into mozilla-mobile:main Oct 26, 2022

lobontiumira added the QAVerified label Oct 27, 2022

mergify bot mentioned this pull request Oct 27, 2022

For #7503 - Show Start browsing cfr at fresh install (backport #7705) #7928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #7503 - Show Start browsing cfr at fresh install #7705

For #7503 - Show Start browsing cfr at fresh install #7705

iorgamgabriel commented Sep 21, 2022 •

edited by github-actions bot

Mugurell Oct 24, 2022

iorgamgabriel Oct 24, 2022

Mugurell Oct 24, 2022

iorgamgabriel Oct 24, 2022

Mugurell Oct 24, 2022

iorgamgabriel Oct 24, 2022

Mugurell Oct 24, 2022

iorgamgabriel Oct 24, 2022

Mugurell Oct 24, 2022

iorgamgabriel Oct 24, 2022

Mugurell Oct 24, 2022

iorgamgabriel Oct 25, 2022

Mugurell Oct 24, 2022

iorgamgabriel Oct 24, 2022

Mugurell Oct 24, 2022

iorgamgabriel Oct 24, 2022

Mugurell left a comment

Mugurell Oct 25, 2022

iorgamgabriel Oct 25, 2022

Mugurell Oct 25, 2022

iorgamgabriel Oct 25, 2022

Mugurell Oct 25, 2022

iorgamgabriel Oct 25, 2022

Mugurell Oct 25, 2022

iorgamgabriel Oct 25, 2022

lobontiumira commented Oct 27, 2022

iorgamgabriel commented Oct 27, 2022

mergify bot commented Oct 27, 2022

	assertNotEquals(null, inputToolbarIntegration.startBrowsingCfrScope)
	assertNotNull(inputToolbarIntegration.startBrowsingCfrScope)

For #7503 - Show Start browsing cfr at fresh install #7705

For #7503 - Show Start browsing cfr at fresh install #7705

Conversation

iorgamgabriel commented Sep 21, 2022 • edited by github-actions bot

Pull Request checklist

QA

To download an APK when reviewing a PR:

GitHub Automation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugurell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobontiumira commented Oct 27, 2022

iorgamgabriel commented Oct 27, 2022

mergify bot commented Oct 27, 2022

✅ Backports have been created

iorgamgabriel commented Sep 21, 2022 •

edited by github-actions bot