For #17802: Match the design for three-dot menu navigation (#17875)

* Feature flag for toolbar menu redesign. Add new items to menu and reorder.

* Handle toolbar items in menu controller

* Remove bookmark from toolbar nav

* Respect toolbar position

* Inline toolbar orientation checks

* Add Todos for bookmark UI tests

* Make variable names consistent

app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt

@@ -12,6 +12,7 @@ import mozilla.appservices.places.BookmarkRoot
 import okhttp3.mockwebserver.MockWebServer
 import org.junit.After
 import org.junit.Before
+import org.junit.Ignore
 import org.junit.Rule
 import org.junit.Test
 import org.mozilla.fenix.R
@@ -31,6 +32,7 @@ import org.mozilla.fenix.ui.robots.navigationToolbar
 /**
  *  Tests for verifying basic functionality of bookmarks
  */
+@Ignore("To be re-implemented in https://github.com/mozilla-mobile/fenix/issues/17979")
 class BookmarksTest {
     /* ktlint-disable no-blank-line-before-rbrace */ // This imposes unreadable grouping.
 

app/src/androidTest/java/org/mozilla/fenix/ui/SettingsBasicsTest.kt

@@ -10,6 +10,7 @@ import androidx.test.uiautomator.UiDevice
 import okhttp3.mockwebserver.MockWebServer
 import org.junit.After
 import org.junit.Before
+import org.junit.Ignore
 import org.junit.Rule
 import org.junit.Test
 import org.mozilla.fenix.FenixApplication
@@ -105,6 +106,7 @@ class SettingsBasicsTest {
     }
 
     @Test
+    @Ignore("To be re-implemented in https://github.com/mozilla-mobile/fenix/issues/17979")
     fun toggleShowVisitedSitesAndBookmarks() {
         // Bookmarks a few websites, toggles the history and bookmarks setting to off, then verifies if the visited and bookmarked websites do not show in the suggestions.
         val page1 = getGenericAsset(mockWebServer, 1)

app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt

@@ -322,6 +322,7 @@ class SmokeTest {
 
     @Test
     // Verifies the Bookmark button in a tab's 3 dot menu
+    @Ignore("To be re-implemented in https://github.com/mozilla-mobile/fenix/issues/17979")
     fun mainMenuBookmarkButtonTest() {
         val defaultWebPage = TestAssetHelper.getGenericAsset(mockWebServer, 1)
 
@@ -979,6 +980,7 @@ class SmokeTest {
 
     @Test
     // Verifies that deleting a Bookmarks folder also removes the item from inside it.
+    @Ignore("To be re-implemented in https://github.com/mozilla-mobile/fenix/issues/17799")
     fun deleteNonEmptyBookmarkFolderTest() {
         val website = TestAssetHelper.getGenericAsset(mockWebServer, 1)
 

app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt

@@ -63,10 +63,11 @@ class DefaultToolbarMenu(
     val isPinningSupported: Boolean
 ) : ToolbarMenu {
 
-    private var currentUrlIsBookmarked = false
+    private var isCurrentUrlBookmarked = false
     private var isBookmarkedJob: Job? = null
-
+    private val isTopToolbarSelected = shouldReverseItems
-    private val selectedSession: TabSessionState? get() = store.state.selectedTab
+    private val selectedSession: TabSessionState?
+        get() = store.state.selectedTab
 
     override val menuBuilder by lazy {
         WebExtensionBrowserMenuBuilder(
@@ -146,24 +147,28 @@ class DefaultToolbarMenu(
 
         registerForIsBookmarkedUpdates()
 
-        val bookmark = BrowserMenuItemToolbar.TwoStateButton(
+        if (FeatureFlags.toolbarMenuFeature) {
-            primaryImageResource = R.drawable.ic_bookmark_filled,
+            BrowserMenuItemToolbar(listOf(back, forward, share, refresh))
-            primaryContentDescription = context.getString(R.string.browser_menu_edit_bookmark),
+        } else {
-            primaryImageTintResource = primaryTextColor(),
+            val bookmark = BrowserMenuItemToolbar.TwoStateButton(
-            // TwoStateButton.isInPrimaryState must be synchronous, and checking bookmark state is
+                primaryImageResource = R.drawable.ic_bookmark_filled,
-            // relatively slow. The best we can do here is periodically compute and cache a new "is
+                primaryContentDescription = context.getString(R.string.browser_menu_edit_bookmark),
-            // bookmarked" state, and use that whenever the menu has been opened.
+                primaryImageTintResource = primaryTextColor(),
-            isInPrimaryState = { currentUrlIsBookmarked },
+                // TwoStateButton.isInPrimaryState must be synchronous, and checking bookmark state is
-            secondaryImageResource = R.drawable.ic_bookmark_outline,
+                // relatively slow. The best we can do here is periodically compute and cache a new "is
-            secondaryContentDescription = context.getString(R.string.browser_menu_bookmark),
+                // bookmarked" state, and use that whenever the menu has been opened.
-            secondaryImageTintResource = primaryTextColor(),
+                isInPrimaryState = { isCurrentUrlBookmarked },
-            disableInSecondaryState = false
+                secondaryImageResource = R.drawable.ic_bookmark_outline,
-        ) {
+                secondaryContentDescription = context.getString(R.string.browser_menu_bookmark),
-            if (!currentUrlIsBookmarked) currentUrlIsBookmarked = true
+                secondaryImageTintResource = primaryTextColor(),
-            onItemTapped.invoke(ToolbarMenu.Item.Bookmark)
+                disableInSecondaryState = false
-        }
+            ) {
+                if (!isCurrentUrlBookmarked) isCurrentUrlBookmarked = true
+                onItemTapped.invoke(ToolbarMenu.Item.Bookmark)
+            }
 
-        BrowserMenuItemToolbar(listOf(back, forward, bookmark, share, refresh))
+            BrowserMenuItemToolbar(listOf(back, forward, bookmark, share, refresh))
+        }
     }
 
     // Predicates that need to be repeatedly called as the session changes
@@ -469,26 +474,28 @@ class DefaultToolbarMenu(
             onItemTapped.invoke(ToolbarMenu.Item.Settings)
         }
 
-        val menuItems = listOfNotNull(
+        val menuItems =
-            newTabItem,
+            listOfNotNull(
-            BrowserMenuDivider(),
+                if (isTopToolbarSelected) menuToolbar else null,
-            bookmarksItem,
+                newTabItem,
-            historyItem,
+                BrowserMenuDivider(),
-            downloadsItem,
+                bookmarksItem,
-            extensionsItem,
+                historyItem,
-            syncedTabsItem,
+                downloadsItem,
-            BrowserMenuDivider(),
+                extensionsItem,
-            findInPageItem,
+                syncedTabsItem,
-            desktopSiteItem,
+                BrowserMenuDivider(),
-            BrowserMenuDivider(),
+                findInPageItem,
-            addToHomeScreenItem.apply { visible = ::canAddToHomescreen },
+                desktopSiteItem,
-            addToTopSitesItem,
+                BrowserMenuDivider(),
-            saveToCollectionItem,
+                addToHomeScreenItem.apply { visible = ::canAddToHomescreen },
-            BrowserMenuDivider(),
+                addToTopSitesItem,
-            settingsItem,
+                saveToCollectionItem,
-            BrowserMenuDivider(),
+                BrowserMenuDivider(),
-            menuToolbar
+                settingsItem,
-        )
+                if (isTopToolbarSelected) null else BrowserMenuDivider(),
+                if (isTopToolbarSelected) null else menuToolbar
+            )
 
         menuItems
     }
@@ -512,7 +519,7 @@ class DefaultToolbarMenu(
                     )
                 }
                 .collect {
-                    currentUrlIsBookmarked = false
+                    isCurrentUrlBookmarked = false
                     updateCurrentUrlIsBookmarked(it.content.url)
                 }
         }
@@ -522,7 +529,7 @@ class DefaultToolbarMenu(
     internal fun updateCurrentUrlIsBookmarked(newUrl: String) {
         isBookmarkedJob?.cancel()
         isBookmarkedJob = lifecycleOwner.lifecycleScope.launch {
-            currentUrlIsBookmarked = bookmarksStorage
+            isCurrentUrlBookmarked = bookmarksStorage
                 .getBookmarksWithUrl(newUrl)
                 .any { it.url == newUrl }
         }