From 42cc4cb452a34ca8c6fcdcb7340e97945b4871c2 Mon Sep 17 00:00:00 2001 From: Elise Richards Date: Thu, 4 Mar 2021 17:22:13 -0600 Subject: [PATCH] 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 --- .../org/mozilla/fenix/ui/BookmarksTest.kt | 2 + .../mozilla/fenix/ui/SettingsBasicsTest.kt | 2 + .../java/org/mozilla/fenix/ui/SmokeTest.kt | 2 + .../components/toolbar/DefaultToolbarMenu.kt | 91 ++++++++++--------- 4 files changed, 55 insertions(+), 42 deletions(-) diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt index 3b50a489fe..e4893305b1 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt +++ b/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. diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/SettingsBasicsTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/SettingsBasicsTest.kt index 14d03757ad..a20877bd34 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/SettingsBasicsTest.kt +++ b/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) diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt index 798cfd6274..4f98efed77 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt +++ b/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) diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt index 10018c2274..256c4341ef 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt +++ b/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 selectedSession: TabSessionState? get() = store.state.selectedTab + private val isTopToolbarSelected = shouldReverseItems + 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( - primaryImageResource = R.drawable.ic_bookmark_filled, - primaryContentDescription = context.getString(R.string.browser_menu_edit_bookmark), - primaryImageTintResource = primaryTextColor(), - // TwoStateButton.isInPrimaryState must be synchronous, and checking bookmark state is - // relatively slow. The best we can do here is periodically compute and cache a new "is - // bookmarked" state, and use that whenever the menu has been opened. - isInPrimaryState = { currentUrlIsBookmarked }, - secondaryImageResource = R.drawable.ic_bookmark_outline, - secondaryContentDescription = context.getString(R.string.browser_menu_bookmark), - secondaryImageTintResource = primaryTextColor(), - disableInSecondaryState = false - ) { - if (!currentUrlIsBookmarked) currentUrlIsBookmarked = true - onItemTapped.invoke(ToolbarMenu.Item.Bookmark) - } + if (FeatureFlags.toolbarMenuFeature) { + BrowserMenuItemToolbar(listOf(back, forward, share, refresh)) + } else { + val bookmark = BrowserMenuItemToolbar.TwoStateButton( + primaryImageResource = R.drawable.ic_bookmark_filled, + primaryContentDescription = context.getString(R.string.browser_menu_edit_bookmark), + primaryImageTintResource = primaryTextColor(), + // TwoStateButton.isInPrimaryState must be synchronous, and checking bookmark state is + // relatively slow. The best we can do here is periodically compute and cache a new "is + // bookmarked" state, and use that whenever the menu has been opened. + isInPrimaryState = { isCurrentUrlBookmarked }, + secondaryImageResource = R.drawable.ic_bookmark_outline, + secondaryContentDescription = context.getString(R.string.browser_menu_bookmark), + secondaryImageTintResource = primaryTextColor(), + 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( - newTabItem, - BrowserMenuDivider(), - bookmarksItem, - historyItem, - downloadsItem, - extensionsItem, - syncedTabsItem, - BrowserMenuDivider(), - findInPageItem, - desktopSiteItem, - BrowserMenuDivider(), - addToHomeScreenItem.apply { visible = ::canAddToHomescreen }, - addToTopSitesItem, - saveToCollectionItem, - BrowserMenuDivider(), - settingsItem, - BrowserMenuDivider(), - menuToolbar - ) + val menuItems = + listOfNotNull( + if (isTopToolbarSelected) menuToolbar else null, + newTabItem, + BrowserMenuDivider(), + bookmarksItem, + historyItem, + downloadsItem, + extensionsItem, + syncedTabsItem, + BrowserMenuDivider(), + findInPageItem, + desktopSiteItem, + BrowserMenuDivider(), + addToHomeScreenItem.apply { visible = ::canAddToHomescreen }, + addToTopSitesItem, + saveToCollectionItem, + BrowserMenuDivider(), + 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 } }