diff --git a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt index 326b92550..c1c2a8cbd 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt @@ -168,7 +168,7 @@ class HistoryMetadataMiddleware( // web content i.e., they followed a link, not if the user navigated directly via // toolbar. !directLoadTriggered && previousUrlIndex >= 0 -> { - // Once a tab is within the search group, only direct navigation event can change that. + // Once a tab is within the search group, only a direct load event (via the toolbar) can change that. val (searchTerms, referrerUrl) = if (tabMetadataHasSearchTerms) { tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl } else { @@ -184,15 +184,15 @@ class HistoryMetadataMiddleware( } // In certain redirect cases, we won't have a previous url in the history stack of the tab, // but will have the search terms already set on the tab from having gone through this logic - // for the redirecting url. - // In that case, we leave this tab within the search group it's already in. - tabMetadataHasSearchTerms -> { + // for the redirecting url. So we leave this tab within the search group it's already in + // unless a new direct load (via the toolbar) was triggered. + tabMetadataHasSearchTerms && !(directLoadTriggered && previousUrlIndex >= 0) -> { tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl } // We had no search terms, no history stack, and no parent. - // For example, this would be a search results page itself. - // For now, the original search results page is not part of the search group. - // See https://github.com/mozilla-mobile/fenix/issues/21659. + // This would be the case for any page loaded directly via the toolbar including + // a search results page itself. For now, the original search results page is not + // part of the search group: https://github.com/mozilla-mobile/fenix/issues/21659. else -> null to null } diff --git a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt index c84f98c67..2fe95d63e 100644 --- a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt @@ -149,7 +149,7 @@ class HistoryMetadataMiddlewareTest { assertEquals(2, this.count()) } - // Parent navigates away. Search terms are reset. + // Parent navigates away. store.dispatch(ContentAction.UpdateUrlAction(parentTab.id, "https://firefox.com")).joinBlocking() store.dispatch(ContentAction.UpdateSearchTermsAction(parentTab.id, "")).joinBlocking() store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website"), HistoryItem("Firefox", "https://firefox.com")), 1)).joinBlocking() @@ -186,6 +186,62 @@ class HistoryMetadataMiddlewareTest { } } + @Test + fun `GIVEN tab with search terms WHEN subsequent direct load occurs THEN search terms are not retained`() { + service = TestingMetadataService() + middleware = HistoryMetadataMiddleware(service) + store = BrowserStore( + middleware = listOf(middleware) + EngineMiddleware.create(engine = mockk()), + initialState = BrowserState() + ) + setupGoogleSearchEngine() + + val parentTab = createTab("https://google.com?q=mozilla+website", searchTerms = "mozilla website") + val tab = createTab("https://google.com?url=https://mozilla.org", parent = parentTab) + store.dispatch(TabListAction.AddTabAction(parentTab, select = true)).joinBlocking() + store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking() + + with((service as TestingMetadataService).createdMetadata) { + assertEquals(2, this.count()) + assertEquals("https://google.com?q=mozilla+website", this[0].url) + assertNull(this[0].searchTerm) + assertNull(this[0].referrerUrl) + + assertEquals("https://google.com?url=https://mozilla.org", this[1].url) + assertEquals("mozilla website", this[1].searchTerm) + assertEquals("https://google.com?q=mozilla+website", this[1].referrerUrl) + } + + // Both tabs load. + store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website")), 0)).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("", "https://google.com?url=mozilla+website")), currentIndex = 0)).joinBlocking() + with((service as TestingMetadataService).createdMetadata) { + assertEquals(2, this.count()) + } + + // Direct load occurs on child tab. Search terms should be cleared. + store.dispatch(EngineAction.LoadUrlAction(tab.id, "https://firefox.com")).joinBlocking() + store.dispatch(ContentAction.UpdateUrlAction(tab.id, "https://firefox.com")).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("", "https://google.com?url=mozilla+website"), HistoryItem("Firefox", "https://firefox.com")), 1)).joinBlocking() + with((service as TestingMetadataService).createdMetadata) { + assertEquals(3, this.count()) + assertEquals("https://firefox.com", this[2].url) + assertNull(this[2].searchTerm) + assertNull(this[2].referrerUrl) + } + + // Direct load occurs on parent tab. Search terms should be cleared. + store.dispatch(EngineAction.LoadUrlAction(parentTab.id, "https://firefox.com")).joinBlocking() + store.dispatch(ContentAction.UpdateUrlAction(parentTab.id, "https://firefox.com")).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website"), HistoryItem("Firefox", "https://firefox.com")), 1)).joinBlocking() + with((service as TestingMetadataService).createdMetadata) { + assertEquals(4, this.count()) + assertEquals("https://firefox.com", this[3].url) + assertNull(this[3].searchTerm) + assertNull(this[3].referrerUrl) + } + } + @Test fun `GIVEN normal tab has parent WHEN url is the same THEN nothing happens`() { val parentTab = createTab("https://mozilla.org", searchTerms = "mozilla website")