1895144 - [Menu Redesign] Show the snackbar after bookmarking a page

Status: VERIFIED FIXED

(Firefox for Android :: Bookmarks, task, P1)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

As a followup to Bug 1885628, we need to do the following after clicking on the "Bookmark this page" menu item in the "Save" submenu:
(1) We need to dismiss the menu after clicking on "Bookmark this page".
(2) We need to display a snackbar indicating whether or not the bookmark was saved successfully or indicate an error occurred.

This should follow closely to https://searchfox.org/mozilla-central/rev/6eb9b2d8f23c03ff6e203502a476cbdb0358a807/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt#1692-1739

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1895144 - Part 1: Introduce SnackbarBinding for observing the snackbar state and displaying the snackbar

• Refactors the existing fragment result listener for translation in progress into a LifecycleAwareFeature.
• The goal of this is extract out this code from BrowserFragment and make a Feature class that will handle multiple fragment results (eg, Translations, Bookmarks, etc) that will be coming from the menu redesign.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1895144 - Part 2: Display the snackbar after bookmarking a page

• Implements displaying a snackbar after adding a bookmark. This is done by setting a fragment result listener
  that is handled by SnackbarFeature, which will respond to the provided results to display the appropriate
  snackbar message.
• We dismiss the menu dialog after adding a bookmark.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b3477ad6504
Part 1: Introduce SnackbarBinding for observing the snackbar state and displaying the snackbar r=android-reviewers,matt-tighe
https://hg.mozilla.org/integration/autoland/rev/f76dc877a156
Part 2:  Display the snackbar after bookmarking a page r=android-reviewers,matt-tighe

Comment 4

[tszentpeteri]

Backed out for causing fenix debug failures related to privatebrowsing.DefaultPrivateBrowsingControllerTest.

• Backout link
• Push with failures
• Failure Log:

[task 2024-07-02T06:37:49.475Z] SUITE: org.mozilla.fenix.home.privatebrowsing.DefaultPrivateBrowsingControllerTest
[task 2024-07-02T06:37:49.475Z]   TEST: WHEN private browsing learn more link is clicked THEN open support page in browser
[task 2024-07-02T06:37:49.575Z]   TEST-UNEXPECTED-FAIL | org.mozilla.fenix.home.privatebrowsing.DefaultPrivateBrowsingControllerTest.WHEN private browsing learn more link is clicked THEN open support page in browser | java.lang.AssertionError: Verification failed: call 1 of 1: HomeActivity(#790).openToBrowserAndLoad(eq(https://support.mozilla.org/en-US/kb/common-myths-about-private-browsing?as=u&utm_source=inproduct), eq(true), eq(FromHome), null(), null(), eq(false), eq(mozilla.components.concept.engine.EngineSession$LoadUrlFlags@0), eq(false), null(), null())). Only one matching call to HomeActivity(#790)/openToBrowserAndLoad(String, Boolean, BrowserDirection, String, SearchEngine, Boolean, LoadUrlFlags, Boolean, HistoryMetadataKey, Map) happened, but arguments are not matching:
[task 2024-07-02T06:37:49.575Z] [0]: argument: https://support.mozilla.org/ar/kb/common-myths-about-private-browsing?as=u&utm_source=inproduct, matcher: eq(https://support.mozilla.org/en-US/kb/common-myths-about-private-browsing?as=u&utm_source=inproduct), result: -
[task 2024-07-02T06:37:49.575Z] [1]: argument: true, matcher: eq(true), result: +
[task 2024-07-02T06:37:49.575Z] [2]: argument: FromHome, matcher: eq(FromHome), result: +
[task 2024-07-02T06:37:49.575Z] [3]: argument: null, matcher: null(), result: +
[task 2024-07-02T06:37:49.575Z] [4]: argument: null, matcher: null(), result: +
[task 2024-07-02T06:37:49.575Z] [5]: argument: false, matcher: eq(false), result: +
[task 2024-07-02T06:37:49.575Z] [6]: argument: mozilla.components.concept.engine.EngineSession$LoadUrlFlags@0, matcher: eq(mozilla.components.concept.engine.EngineSession$LoadUrlFlags@0), result: +
[task 2024-07-02T06:37:49.575Z] [7]: argument: false, matcher: eq(false), result: +
[task 2024-07-02T06:37:49.575Z] [8]: argument: null, matcher: null(), result: +
[task 2024-07-02T06:37:49.575Z] [9]: argument: null, matcher: null(), result: +
[task 2024-07-02T06:37:49.575Z] 
[task 2024-07-02T06:37:49.575Z] Stack trace:
[task 2024-07-02T06:37:49.575Z]                                                     io.mockk.impl.InternalPlatform.captureStackTrace                                                                  (InternalPlatform.kt:130)                    
[task 2024-07-02T06:37:49.575Z]                                                       io.mockk.impl.stub.MockKStub.handleInvocation                                                                   (MockKStub.kt:253)                           
[task 2024-07-02T06:37:49.575Z]                     io.mockk.impl.instantiation.JvmMockFactoryHelper$mockHandler$1.invocation                                                                         (JvmMockFactoryHelper.kt:24)                 
[task 2024-07-02T06:37:49.575Z]                                              io.mockk.proxy.jvm.advice.Interceptor.call                                                                               (Interceptor.kt:21)                          
[task 2024-07-02T06:37:49.575Z]                                                     org.mozilla.fenix.HomeActivity.openToBrowserAndLoad                                                               (HomeActivity.kt:1019)                       
[task 2024-07-02T06:37:49.575Z]                                                     org.mozilla.fenix.HomeActivity.openToBrowserAndLoad$default                                                       (HomeActivity.kt:996)                        
[task 2024-07-02T06:37:49.575Z] org.mozilla.fenix.home.privatebrowsing.controller.DefaultPrivateBrowsingController.handleLearnMoreClicked                                                             (PrivateBrowsingController.kt:47)            
[task 2024-07-02T06:37:49.575Z]        org.mozilla.fenix.home.privatebrowsing.DefaultPrivateBrowsingControllerTest.WHEN private browsing learn more link is clicked THEN open support page in browser (DefaultPrivateBrowsingControllerTest.kt:64) 
[task 2024-07-02T06:37:49.575Z]                                      jdk.internal.reflect.NativeMethodAccessorImpl.invoke0                                                                            (NativeMethodAccessorImpl.java:-2)N          
[task 2024-07-02T06:37:49.575Z]                                      jdk.internal.reflect.NativeMethodAccessorImpl.invoke                                                                             (NativeMethodAccessorImpl.java:77)           
[task 2024-07-02T06:37:49.575Z]                                  jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke                                                                             (DelegatingMethodAccessorImpl.java:43)       
[task 2024-07-02T06:37:49.575Z]                                                           java.lang.reflect.Method.invoke                                                                             (Method.java:568)                            
[task 2024-07-02T06:37:49.575Z]                                          org.junit.runners.model.FrameworkMethod$1.runReflectiveCall                                                                  (FrameworkMethod.java:59)                    
[task 2024-07-02T06:37:49.575Z]                                org.junit.internal.runners.model.ReflectiveCallable.run                                                                                (ReflectiveCallable.java:12)                 
[task 2024-07-02T06:37:49.575Z]                                            org.junit.runners.model.FrameworkMethod.invokeExplosively                                                                  (FrameworkMethod.java:56)                    
[task 2024-07-02T06:37:49.575Z]                                 org.junit.internal.runners.statements.InvokeMethod.evaluate                                                                           (InvokeMethod.java:17)                       
[task 2024-07-02T06:37:49.575Z]                                   org.junit.internal.runners.statements.RunBefores.evaluate                                                                           (RunBefores.java:26)                         
[task 2024-07-02T06:37:49.575Z]                                                   org.junit.runners.ParentRunner$3.evaluate                                                                           (ParentRunner.java:306)                      
[task 2024-07-02T06:37:49.575Z]                                         org.junit.runners.BlockJUnit4ClassRunner$1.evaluate                                                                           (BlockJUnit4ClassRunner.java:100)            
[task 2024-07-02T06:37:49.575Z]                                                     org.junit.runners.ParentRunner.runLeaf                                                                            (ParentRunner.java:366)                      
[task 2024-07-02T06:37:49.575Z]                                           org.junit.runners.BlockJUnit4ClassRunner.runChild                                                                           (BlockJUnit4ClassRunner.java:103)            
[task 2024-07-02T06:37:49.575Z]                                           org.junit.runners.BlockJUnit4ClassRunner.runChild                                                                           (BlockJUnit4ClassRunner.java:63)             
[task 2024-07-02T06:37:49.575Z]                                                   org.junit.runners.ParentRunner$4.run                                                                                (ParentRunner.java:331)                      
[task 2024-07-02T06:37:49.575Z]                                                   org.junit.runners.ParentRunner$1.schedule                                                                           (ParentRunner.java:79)                       
[task 2024-07-02T06:37:49.575Z]                                                     org.junit.runners.ParentRunner.runChildren                                                                        (ParentRunner.java:329)                      
[task 2024-07-02T06:37:49.575Z]                                                     org.junit.runners.ParentRunner.access$100                                                                         (ParentRunner.java:66)                       
[task 2024-07-02T06:37:49.575Z]                                                   org.junit.runners.ParentRunner$2.evaluate                                                                           (ParentRunner.java:293)                      
[task 2024-07-02T06:37:49.575Z]                                                   org.junit.runners.ParentRunner$3.evaluate                                                                           (ParentRunner.java:306)                      
[task 2024-07-02T06:37:49.575Z]                                                     org.junit.runners.ParentRunner.run                                                                                (ParentRunner.java:413)                      
[task 2024-07-02T06:37:49.575Z]                 org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass                                                                       (JUnitTestClassExecutor.java:112)            
[task 2024-07-02T06:37:49.575Z]                 org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute                                                                            (JUnitTestClassExecutor.java:58)             
[task 2024-07-02T06:37:49.575Z]                 org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute                                                                            (JUnitTestClassExecutor.java:40)             
[task 2024-07-02T06:37:49.575Z]        org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass                                                                   (AbstractJUnitTestClassProcessor.java:60)    
[task 2024-07-02T06:37:49.575Z]                      org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass                                                                   (SuiteTestClassProcessor.java:52)            
[task 2024-07-02T06:37:49.575Z]                                    jdk.internal.reflect.GeneratedMethodAccessor113.invoke                                                                             (-:-1)                                       
[task 2024-07-02T06:37:49.575Z]                                  jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke                                                                             (DelegatingMethodAccessorImpl.java:43)       
[task 2024-07-02T06:37:49.575Z]                                                           java.lang.reflect.Method.invoke                                                                             (Method.java:568)                            
[task 2024-07-02T06:37:49.575Z]                                    org.gradle.internal.dispatch.ReflectionDispatch.dispatch                                                                           (ReflectionDispatch.java:36)                 
[task 2024-07-02T06:37:49.575Z]                                    org.gradle.internal.dispatch.ReflectionDispatch.dispatch                                                                           (ReflectionDispatch.java:24)                 
[task 2024-07-02T06:37:49.575Z]                            org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch                                                                           (ContextClassLoaderDispatch.java:33)         
[task 2024-07-02T06:37:49.575Z]     org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke                                                                             (ProxyDispatchAdapter.java:94)               
[task 2024-07-02T06:37:49.575Z]                                                                 jdk.proxy2.$Proxy5.processTestClass                                                                   (-:-1)                                       
[task 2024-07-02T06:37:49.575Z]                          org.gradle.api.internal.tasks.testing.worker.TestWorker$2.run                                                                                (TestWorker.java:176)                        
[task 2024-07-02T06:37:49.575Z]                            org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName                                                       (TestWorker.java:129)                        
[task 2024-07-02T06:37:49.575Z]                            org.gradle.api.internal.tasks.testing.worker.TestWorker.execute                                                                            (TestWorker.java:100)
[task 2024-07-02T06:37:49.575Z]                            org.gradle.api.internal.tasks.testing.worker.TestWorker.execute                                                                            (TestWorker.java:60)                         
[task 2024-07-02T06:37:49.575Z]                     org.gradle.process.internal.worker.child.ActionExecutionWorker.execute                                                                            (ActionExecutionWorker.java:56)              
[task 2024-07-02T06:37:49.575Z]        org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call                                                                               (SystemApplicationClassLoaderWorker.java:113)
[task 2024-07-02T06:37:49.575Z]        org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call                                                                               (SystemApplicationClassLoaderWorker.java:65) 
[task 2024-07-02T06:37:49.575Z]                         worker.org.gradle.process.internal.worker.GradleWorkerMain.run                                                                                (GradleWorkerMain.java:69)                   
[task 2024-07-02T06:37:49.575Z]                         worker.org.gradle.process.internal.worker.GradleWorkerMain.main                                                                               (GradleWorkerMain.java:74)                   
[task 2024-07-02T06:37:49.575Z] 
[task 2024-07-02T06:37:49.575Z]   TEST: WHEN private mode is deselected on home from behind search THEN handle mode change
[task 2024-07-02T06:37:49.775Z]   SUCCESS

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/723b7457e505
Part 1: Introduce SnackbarBinding for observing the snackbar state and displaying the snackbar r=android-reviewers,matt-tighe
https://hg.mozilla.org/integration/autoland/rev/ebbb303dc9e6
Part 2:  Display the snackbar after bookmarking a page r=android-reviewers,matt-tighe

Comment 6

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/723b7457e505
https://hg.mozilla.org/mozilla-central/rev/ebbb303dc9e6

Comment 7

[Vasilica Tamas (Android QA) [:vtamas]]

Attachment: BookmarkSnackbar.png

I confirm that the snackbar is successfully displayed on Firefox Nightly 132 (2024-09-04) on Google Pixel 8 (Android 14) and Lenovo Tab P11 Pro (Android 13). But the text is different compared with the snackbar displayed using the old menu:

• new menu: Saved in "Bookmarks" EDIT snackbar is displayed.
• old menu: Bookmark saved! EDIT snackbar is displayed.

Is the new bookmark snackbar design intended, or should we keep the old version?

Comment 8

[Chris Peterson [:cpeterson]]

@ Vasilica: sorry I didn't see your needinfo request sooner!

@ gl or Matt: the Saved in "Bookmarks" snackbar message was added in Bookmarks Evolution bug 1902054 to replace the Bookmark saved! message, but is the new message format gated by the new menu design's feature flag? Why would Vasilica only see the new message with the new menu, as shown in the attached screenshot?

Comment 9

[Chris Peterson [:cpeterson]]

The Saved in "Bookmarks" snackbar message is the bookmark_saved_in_folder_snackbar string. The Bookmark saved! is bookmark_saved_snackbar:

https://searchfox.org/mozilla-central/rev/0386e3e47504286ee176ab373a0562262e7c98b4/mobile/android/fenix/app/src/main/res/values/strings.xml#1157-1160

gl's code changes add some new bookmark_saved_snackbar uses, but no new bookmark_saved_in_folder_snackbar uses:

https://hg.mozilla.org/mozilla-central/rev/723b7457e505
https://hg.mozilla.org/mozilla-central/rev/ebbb303dc9e6

So why isn't the bookmark_saved_in_folder_snackbar string shown when Vasilica uses the same STR in the old menu?

Comment 10

[Matt Tighe [:matt-tighe]]

We only landed it for the new menu at first, but it will be introduced to the old menu when https://bugzilla.mozilla.org/show_bug.cgi?id=1919619 lands

Comment 11

[Vasilica Tamas (Android QA) [:vtamas]]

Thanks for clarifications! Based on Comment 7 I am marking this bug as Verified.