diff --git a/app/metrics.yaml b/app/metrics.yaml index fc7037ed37..a87fae1561 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -2521,23 +2521,6 @@ bookmarks_management: metadata: tags: - Bookmarks - open_all_in_private_tabs: - type: event - description: | - A user opened a folder of bookmarks at once in new private tabs. - bugs: - - https://github.com/mozilla-mobile/fenix/issues/974 - - https://github.com/mozilla-mobile/fenix/issues/11404 - data_reviews: - - https://github.com/mozilla-mobile/fenix/pull/21212 - data_sensitivity: - - interaction - notification_emails: - - android-probes@mozilla.com - expires: never - metadata: - tags: - - Bookmarks edited: type: event description: | 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 fa3e2a82cd..eb643054c3 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt @@ -301,54 +301,9 @@ class BookmarksTest { verifyExistingOpenTabs("Test_Page_1") verifyExistingOpenTabs("Test_Page_2") verifyExistingOpenTabs("Test_Page_3") - swipeTabRight("Test_Page_1") - swipeTabRight("Test_Page_2") - swipeTabRight("Test_Page_3") - // no more tabs should be presents and auto close tab tray - verifyTabTrayIsClosed() - } - } - - @Test - fun openAllInPrivateTabsTest() { - val nbPages = 4 - val webPages = List(nbPages) { - TestAssetHelper.getGenericAsset(mockWebServer, it + 1) - } - - homeScreen { - }.openThreeDotMenu { - }.openBookmarks { - createFolder("root") - createFolder("sub", "root") - createFolder("empty", "root") - }.closeMenu { - } - - browserScreen { - createBookmark(webPages[0].url, "root") - createBookmark(webPages[1].url, "root") - createBookmark(webPages[2].url, "sub") - // out of folder and should not be opened - createBookmark(webPages[3].url) - }.openTabDrawer { closeTab() - } - - browserScreen { - }.openThreeDotMenu { - }.openBookmarks { - }.openThreeDotMenu("root") { - }.clickOpenAllInPrivateTabs { - verifyTabTrayIsOpened() - verifyPrivateModeSelected() - - verifyExistingOpenTabs("Test_Page_1") - verifyExistingOpenTabs("Test_Page_2") - verifyExistingOpenTabs("Test_Page_3") - swipeTabRight("Test_Page_1") - swipeTabRight("Test_Page_2") - swipeTabRight("Test_Page_3") + closeTab() + closeTab() // no more tabs should be presents and auto close tab tray verifyTabTrayIsClosed() } diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuBookmarksRobot.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuBookmarksRobot.kt index 035aa87ffa..25f6ec3754 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuBookmarksRobot.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/ThreeDotMenuBookmarksRobot.kt @@ -59,13 +59,6 @@ class ThreeDotMenuBookmarksRobot { return TabDrawerRobot.Transition() } - fun clickOpenAllInPrivateTabs(interact: TabDrawerRobot.() -> Unit): TabDrawerRobot.Transition { - openAllInPrivateTabsButton().click() - - TabDrawerRobot().interact() - return TabDrawerRobot.Transition() - } - fun clickDelete(interact: BookmarksRobot.() -> Unit): BookmarksRobot.Transition { deleteButton().click() @@ -85,8 +78,6 @@ private fun openInNewTabButton() = onView(withText("Open in new tab")) private fun openInPrivateTabButton() = onView(withText("Open in private tab")) -private fun openAllInTabsButton() = onView(withText("Open all in new tabs")) - -private fun openAllInPrivateTabsButton() = onView(withText("Open all in private tabs")) +private fun openAllInTabsButton() = onView(withText("Open All Bookmarks")) private fun deleteButton() = onView(withText("Delete")) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt index b46307e150..69205b9331 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt @@ -45,7 +45,7 @@ interface BookmarkController { fun handleCopyUrl(item: BookmarkNode) fun handleBookmarkSharing(item: BookmarkNode) fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode) - fun handleBookmarkFolderOpening(folder: BookmarkNode, mode: BrowsingMode) + fun handleBookmarkFolderOpening(folder: BookmarkNode) /** * Handle bookmark nodes deletion @@ -183,10 +183,10 @@ class DefaultBookmarkController( } else if (firstLaunch) onOpenAllInTabsEmpty.invoke() } - override fun handleBookmarkFolderOpening(folder: BookmarkNode, mode: BrowsingMode) { + override fun handleBookmarkFolderOpening(folder: BookmarkNode) { // potentially heavy function with a lot of bookmarks to open => use a coroutine scope.launch { - recursiveBookmarkFolderOpening(folder, true, mode) + recursiveBookmarkFolderOpening(folder, true) } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt index bdac4350ad..c4b6c86c01 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt @@ -81,12 +81,7 @@ class BookmarkFragmentInteractor( override fun onOpenAllInTabs(folder: BookmarkNode) { require(folder.type == BookmarkNodeType.FOLDER) - bookmarksController.handleBookmarkFolderOpening(folder, BrowsingMode.Normal) - } - - override fun onOpenAllInPrivateTabs(folder: BookmarkNode) { - require(folder.type == BookmarkNodeType.FOLDER) - bookmarksController.handleBookmarkFolderOpening(folder, BrowsingMode.Private) + bookmarksController.handleBookmarkFolderOpening(folder) } override fun onDelete(nodes: Set) { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt index 249535312e..2d678f7ec2 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt @@ -27,7 +27,6 @@ class BookmarkItemMenu( OpenInNewTab, OpenInPrivateTab, OpenAllInTabs, - OpenAllInPrivateTabs, Delete, ; } @@ -111,16 +110,6 @@ class BookmarkItemMenu( } else { null }, - if (hasAtLeastOneChild) { - maybeCreateMenuItem( - itemType, - BookmarkNodeType.FOLDER, - context.getString(R.string.bookmark_menu_open_all_in_privates_button), - Item.OpenAllInPrivateTabs, - ) - } else { - null - }, TextMenuCandidate( text = context.getString(R.string.bookmark_menu_delete_button), textStyle = TextStyle(color = context.getColorFromAttr(R.attr.textWarning)), diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt index c609cfdd72..853a20009a 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt @@ -86,13 +86,6 @@ interface BookmarkViewInteractor : SelectionInteractor { */ fun onOpenAllInTabs(folder: BookmarkNode) - /** - * Opens all bookmark items in new private tabs. - * - * @param folder the bookmark folder containing all items to open in new private tabs - */ - fun onOpenAllInPrivateTabs(folder: BookmarkNode) - /** * Deletes a set of bookmark nodes. * diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt index 153238c393..8131c92b6c 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt @@ -46,7 +46,6 @@ class BookmarkNodeViewHolder( BookmarkItemMenu.Item.OpenInNewTab -> interactor.onOpenInNormalTab(item) BookmarkItemMenu.Item.OpenInPrivateTab -> interactor.onOpenInPrivateTab(item) BookmarkItemMenu.Item.OpenAllInTabs -> interactor.onOpenAllInTabs(item) - BookmarkItemMenu.Item.OpenAllInPrivateTabs -> interactor.onOpenAllInPrivateTabs(item) BookmarkItemMenu.Item.Delete -> interactor.onDelete(setOf(item)) } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 5064890615..b5c9cabf4b 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -858,8 +858,6 @@ Open in private tab Open all in new tabs - - Open all in private tabs Delete diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt index 535365735a..4a8a7c0529 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt @@ -381,7 +381,7 @@ class BookmarkControllerTest { } recurseFind(tree, it) }, - ).handleBookmarkFolderOpening(tree, BrowsingMode.Normal) + ).handleBookmarkFolderOpening(tree) assertTrue(showTabTrayInvoked) verifyOrder { @@ -392,40 +392,6 @@ class BookmarkControllerTest { } } - @Test - fun `handleBookmarkFolderOpening should open all bookmarks in private tabs`() { - var showTabTrayInvoked = false - createController( - showTabTray = { - showTabTrayInvoked = true - }, - loadBookmarkNode = { - fun recurseFind(item: BookmarkNode, guid: String): BookmarkNode? { - if (item.guid == guid) { - return item - } else { - item.children?.iterator()?.forEach { - val res = recurseFind(it, guid) - if (res != null) { - return res - } - } - return null - } - } - recurseFind(tree, it) - }, - ).handleBookmarkFolderOpening(tree, BrowsingMode.Private) - - assertTrue(showTabTrayInvoked) - verifyOrder { - addNewTabUseCase.invoke(item.url!!, private = true) - addNewTabUseCase.invoke(item.url!!, private = true) - addNewTabUseCase.invoke(childItem.url!!, private = true) - homeActivity.browsingModeManager.mode = BrowsingMode.Private - } - } - @Test fun `handleBookmarkFolderOpening for an empty folder should show toast`() { var onOpenAllInTabsEmptyInvoked = false @@ -436,7 +402,7 @@ class BookmarkControllerTest { loadBookmarkNode = { subfolder }, - ).handleBookmarkFolderOpening(subfolder, BrowsingMode.Normal) + ).handleBookmarkFolderOpening(subfolder) assertTrue(onOpenAllInTabsEmptyInvoked) } diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt index c5556975ff..8b670bf229 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt @@ -175,16 +175,7 @@ class BookmarkFragmentInteractorTest { interactor.onOpenAllInTabs(tree) verify { - bookmarkController.handleBookmarkFolderOpening(tree, BrowsingMode.Normal) - } - } - - @Test - fun `open all bookmarks item in new private tabs`() { - interactor.onOpenAllInPrivateTabs(tree) - - verify { - bookmarkController.handleBookmarkFolderOpening(tree, BrowsingMode.Private) + bookmarkController.handleBookmarkFolderOpening(tree) } } diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt index a356018983..82a1ec5b0e 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt @@ -81,13 +81,12 @@ class BookmarkItemMenuTest { folderItems = menu.menuItems(BookmarkNodeType.FOLDER, "") } assertNotNull(folderItems) - assertEquals(4, folderItems!!.size) + assertEquals(3, folderItems!!.size) - val (edit, openAll, openAllPrivate, delete) = folderItems!! + val (edit, openAll, delete) = folderItems!! assertEquals("Edit", edit.text) - assertEquals("Open all in new tabs", openAll.text) - assertEquals("Open all in private tabs", openAllPrivate.text) + assertEquals("Open All Bookmarks", openAll.text) assertEquals("Delete", delete.text) edit.onClick() @@ -96,13 +95,37 @@ class BookmarkItemMenuTest { openAll.onClick() assertEquals(Item.OpenAllInTabs, lastItemTapped) - openAllPrivate.onClick() - assertEquals(Item.OpenAllInPrivateTabs, lastItemTapped) - delete.onClick() assertEquals(Item.Delete, lastItemTapped) } + @Test + fun `delete item has special styling`() { + val deleteItem = menu.menuItems(BookmarkNodeType.SEPARATOR).last() + assertEquals("Delete", deleteItem.text) + assertEquals( + TextStyle(color = context.getColorFromAttr(R.attr.textWarning)), + deleteItem.textStyle, + ) + + deleteItem.onClick() + + assertEquals(Item.Delete, lastItemTapped) + } + + @Test + fun `edit item appears for folders`() { + val folderItems = menu.menuItems(BookmarkNodeType.FOLDER) + assertEquals(2, folderItems.size) + val (edit, delete) = folderItems + + assertEquals("Edit", edit.text) + edit.onClick() + + assertEquals(Item.Edit, lastItemTapped) + assertEquals("Delete", delete.text) + } + @Test fun `all item appears for sites`() = runBlocking { var siteItems: List? = null