From 32941e7985857f765ebd80be52e627ab5953acfb 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) --- .../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 | 54 ++++++++++--------- 4 files changed, 36 insertions(+), 24 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 3b50a489fe7b..e4893305b12c 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 14d03757ad48..a20877bd3438 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 798cfd6274ee..4f98efed770c 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 4965a62e729a..d451e9136208 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 @@ -64,10 +64,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 { BrowserMenuBuilder( @@ -148,24 +149,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 @@ -483,6 +488,7 @@ class DefaultToolbarMenu( } val menuItems = listOfNotNull( + if (isTopToolbarSelected) menuToolbar else null, newTabItem, BrowserMenuDivider(), bookmarksItem, @@ -502,9 +508,9 @@ class DefaultToolbarMenu( BrowserMenuDivider(), settingsItem, BrowserMenuDivider(), - menuToolbar + if (isTopToolbarSelected) null else BrowserMenuDivider(), + if (isTopToolbarSelected) null else menuToolbar ) - menuItems } @@ -527,7 +533,7 @@ class DefaultToolbarMenu( ) } .collect { - currentUrlIsBookmarked = false + isCurrentUrlBookmarked = false updateCurrentUrlIsBookmarked(it.content.url) } } @@ -537,7 +543,7 @@ class DefaultToolbarMenu( internal fun updateCurrentUrlIsBookmarked(newUrl: String) { isBookmarkedJob?.cancel() isBookmarkedJob = lifecycleOwner.lifecycleScope.launch { - currentUrlIsBookmarked = bookmarksStorage + isCurrentUrlBookmarked = bookmarksStorage .getBookmarksWithUrl(newUrl) .any { it.url == newUrl } }