Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Do not capture search terms when user navigates away via app #21527

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

csadilek
Copy link
Contributor

We currently incorrectly record search terms when navigating from the search results page directly (via the app/toolbar) to any other page. This will make that other page be grouped under those likely unrelated search terms.

With this patch we only capture search terms when the user navigates via web content i.e., follows a link.

@csadilek csadilek self-assigned this Sep 28, 2021
@csadilek csadilek changed the title Do not capture search terms when user navigates away via app WIP: Do not capture search terms when user navigates away via app Sep 28, 2021
@grigoryk grigoryk changed the title WIP: Do not capture search terms when user navigates away via app Do not capture search terms when user navigates away via app Sep 28, 2021
Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to get a bit subtle, but I think the general sentiment is right - we don't want to capture searchTerms/referrer for direct navigation events.

I.e., when on search results (page A), when a user directly navigates either to a new search or a link via the awesomebar (page B), we should not record search terms from page A for metadata for page B.

Basically, we should treat direct navigation as a "fresh start". From the point of UpdateHistoryState events which trigger a "metadata write", there is no difference between an entirely new "navigation tree" (e.g. as caused by direct navigation) and, say, a click on a link within the page. So, we have to track additional state (e.g. the directLoadTriggered flag) and infer. It's a bit a subtle but makes sense from the point of view of the middleware.

This brings up an interesting side topic - a common usage pattern is to revise search terms after making an initial search. With this new behaviour, we're explicitly saying that a direct search "resets" a group being currently populated. It'll be interesting to explore if we can do this conditionally - e.g. if multiple searches are similar enough, perhaps, they should be collapsed? However, if something like that doesn't work perfectly (and it likely won't), it runs a risk of being quite annoying.

@grigoryk grigoryk added the pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] label Sep 28, 2021
Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
@csadilek
Copy link
Contributor Author

Yeah, ideally we should not have to hold additional state as this should already be represented in our HistoryState (or other new state): mozilla-mobile/android-components#11034

This would make this a lot less subtle and easier to read.

@mergify mergify bot merged commit 4596d4f into mozilla-mobile:main Sep 28, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this pull request Oct 1, 2021
…-mobile#21527)

Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants