In the patch where I ensure the correct `searchMode` is added, I didn't consider the possibility a user could return to a default SERP. Thus, if there was a search mode chiclet, it should be cleared.
Differential Revision: https://phabricator.services.mozilla.com/D223141
This patch adds telemetry for when the user sees or interacts with restrict keyword results.
Following is an overview of the interactions that will trigger the telemetry to be recorded:
When the user types `@` and selects one of the restrict results, the scalar telemetry will be recorded:
- `urlbar.picked.restrict_keyword_tabs`
- `urlbar.picked.restrict_keyword_bookmarks`
- `urlbar.picked.restrict_keyword_history`
- `urlbar.picked.restrict_keyword_actions`
For example, we can select the tabs result in multiple ways for the telemetry to be recorded:
- type`@` and click on tabs result
- type`@` and arrow down to tabs result and press enter
- type partial keyword `@t` and press enter to autofill
- type partial keyword `@t` and press tab and then enter
When the user types `@` and **does not** select one of the restrict results, the glean abandonment will be recorded:
```
groups:
"general,general,general,general,general,general,restrict_keyword,restrict_keyword,restrict_keyword,restrict_keyword",
results:
"search_engine,search_engine,search_engine,search_engine,search_engine,search_engine,restrict_keyword_bookmarks,restrict_keyword_tabs,restrict_keyword_history,restrict_keyword_actions",
```
When the user types `@` and **does** select one of the restrict results, the glean engagement will be recorded:
```
groups:
"general,general,general,general,general,general,restrict_keyword,restrict_keyword,restrict_keyword,restrict_keyword",
results:
"search_engine,search_engine,search_engine,search_engine,search_engine,search_engine,restrict_keyword_bookmarks,restrict_keyword_tabs,restrict_keyword_history,restrict_keyword_actions",
selected_result: `restrict_keyword_${category}`,
```
For the `selected_result` in engagement telemetry, the `category` variable will be `tabs, bookmarks, history, or actions`.
Differential Revision: https://phabricator.services.mozilla.com/D222355
There's two main changes:
- Change `getSearchTermIfDefaultSerpUri` to enable it work for any app-provided engine.
- Show the correct Search Chicklet / Unified Search Mode Chicklet when the user loads a non-default SERP. Recall that Persisted Search is enabled in Nightly independent of Scotch Bonnet.
**Issue 1:**
`this.searchMode` is not always known or consistent with the non-default search page when `setURI` is called.
Some scenarios:
- Clear search mode, switch tab, return to the original. The absence of search mode seems acceptable because the user was potentially in the middle of modifying their search or navigating to a URL.
- Clear search mode and reload a non-default SERP.
- Switch search mode, but reload non-default SERP.
- Go back/forward in tabhistory to return to a non-default SERP.
In the latter three cases, if the search terms were to persist again, I'd expect the most accurate search mode to be present.
**Issue 2:**
`userTypedValue` has been used as an indicator for whether or not search terms should persist. However, entering search mode causes userTypedValue to have a value, so that it can be restored when switching back to the current tab and across sessions. Thus, on blur, I have a slightly more complicated check to ensure the revert button is consistent regardless of whether the engine is default or non-default.
**Issue 3:**
Lastly, it's possible for the revert button to show up when switching from tab to tab while the address bar is focused. I added an additional check in this patch since it's a small addition, but I can move it out if it overcomplicates things.
I'm not 100% confident my solution is ideal since I ended up having to add hacky awaits in `browser_UrlbarInput_searchTerms_searchMode.js` for the search mode to appear...
Differential Revision: https://phabricator.services.mozilla.com/D222846
When focusing the urlbar with the mouse we must avoid modifying or
shifting the URL to support operations like double-click or drag to select.
When focusing with the keyboard though we don't have such restrictions
and we can make editing and selection more visually coherent.
This patch is not intercepting commands on mainKeyset, because F6 and
TAB bypass all of that. We instead detect focus following a keydown,
though we must avoid untrimming on TabSelect shortcuts.
Note direct calls to .focus() without a peripheral follow the mouse
behavior, because code (external callers) and tests rely on that.
Differential Revision: https://phabricator.services.mozilla.com/D222138
Please see the bug for context and motivation.
I found a problem with the `terminal` calculation I added in bug 1915507
D220501. Basing it on query contexts isn't quite right. Ideally, for visible
results, `terminal` will be true iff the result is in `view.visibleResults` at
the end of the session, i.e., iff the result is recorded in the engagement or
abandonment event. Anything else would be confusing.
During the last query in a session, the view will be full of stale results from
previous queries. As the view updates itself, it will mark these rows as stale
and call `addExposure()` for them, passing in the current (final) query context.
It's not incorrect to add exposures since these rows are in fact visible. And if
the final query is canceled, they'll remain visible and actually be terminal
results. However, if the final query finishes without being canceled, the view
will remove these rows, and they won't be terminal at all, but since the view
called `addExposure()` for them with the final query context, `TelemetryEvent`
will think they are.
For visible exposures, the `terminal` calculation needs to be based on the
actual visible rows at the end of the session, same as how the results in the
engagement/abandonment events are determined. For hidden exposures, we can just
use `queryContext.results`.
Differential Revision: https://phabricator.services.mozilla.com/D221912
This makes `browser_search_firefoxSuggest.js` more realistic by installing a
Nimbus experiment to change the Suggest scenario instead of manually setting the
scenario via the helper function. (The "scenario" just means whether Suggest is
enabled or not, basically. "history" means it's not enabled; "offline" and
"online" mean it's enabled. The difference between the latter two is that in
"online" the user has opted in to the Merino server, and that only means one of
the Suggest prefs is true instead of false.)
I noticed `browser_privacy_firefoxSuggest.js` does not check the visibility of
its Suggest section at all, so I added similar tasks to it and factored out the
common helpers into `head.js`.
Depends on D221097
Differential Revision: https://phabricator.services.mozilla.com/D221099
In addition to addressing the bug, this also adds some exposure tests to make
sure that they can be triggered in non-Suggest locales and that non-exposure
suggestions are not triggered.
Depends on D220501
Differential Revision: https://phabricator.services.mozilla.com/D221097
This replaces the current potential exposures implementation with "keyword
exposures." The current implementation is based on a keyword list defined in
Nimbus. Keyword exposures improve on the following drawbacks to that approach:
* Potential exposures can't be recorded for existing result types without
duplicating their keywords in the Nimbus list. The ability to record potential
exposures for existing types is an idea from DS (Dave).
* They're unrelated to `exposure` telemetry, so we don't get an `exposure` event
for them in the main ping, making it hard to correlate them with engagements
and abandonments. This is another drawback pointed out by DS (Dave). (By
design, we don't want to correlate individual keywords with
engagements/abandonments for privacy reasons, but we do want to know whether a
potential exposure was triggered during an engagement/abandonment without
knowing the matching keywords.)
* Storing the keywords in Nimbus means the Nimbus recipe is at least as large as
the keyword list. It's probably not a great idea to put thousands of keywords
in the recipe JSON.
* The keyword-matching strategy is simplistic exact matching. It can't do more
sophisticated strategies used for real results, like how we recently started
using FTS in Suggest.
Keyword exposures as implemented by this revision improve on all that. Summary:
* Keyword exposures are implemented on top of existing `exposure` telemetry.
They can be enabled for any result type, and they are always "in addition to"
`exposure` telemetry.
* They're enabled with a new bool Nimbus variable. When true, they're recorded
for all results that trigger `exposure` telemetry.
* They work with the new `Exposure` Rust suggestions (bug 1915317, bug 1893086,
result type "rust_exposure"). In combination, `Exposure` suggestions and
keyword exposures are the new way to do "potential exposures," i.e., exposure
telemetry for hypothetical suggestions that includes matching keywords. Like
every other Rust suggestion type, `Exposure` suggestions take their keywords
from remote settings and can do sophisticated matching.
Other changes in this revision:
* Keyword exposures are recorded on each instance of a matched result. With the
current approach, potential exposures are recorded per unique matched keyword.
That gives us more info. I cleared this idea with Dave.
* Properly detect `terminal` cases by comparing the final query context with the
context at the time of the exposure. The current detection is wrong because
it's only based on search strings, which doesn't work because the final search
string could also have been typed earlier in the session.
* In the feature manifest, change the branch of the related `setPref` variables
from `default` to `user`. When an experiment is uninstalled, the Nimbus client
[does *not* restore previous defaults](https://searchfox.org/mozilla-central/rev/446ff34da077a31d5550359480cb327f729c027b/toolkit/components/normandy/lib/PrefUtils.sys.mjs#119) for prefs set on the default branch. That
breaks some of the tests and also doesn't seem good for users.
Differential Revision: https://phabricator.services.mozilla.com/D220501
This integrates Rust exposure suggestions with desktop. Exposure suggestions are
a part of the replacement for the existing potential exposures feature
(bug 1881875). When we want to test potential exposures in the future, we can
add new exposure suggestions to remote settings and tell the Rust component to
ingest them and return them in queries. When the Rust component returns an
exposure suggestion, desktop will record it in exposure telemetry. (The other
part of the replacement is keyword exposure telemetry in bug 1915507 D220501).
More details about the design of exposure suggestions here:
* Bug 1893086
* https://github.com/mozilla/application-services/pull/6343
The way desktop tells the Rust component about different types of exposure
suggestions is through the new "provider constraints" feature. `ingest()` and
`query()` can take a `SuggestionProviderConstraints` object that changes what's
ingested and queried. Therefore, we need to re-ingest exposure suggestions when
the provider constraints change. Right now, exposure suggestions are the only
kind of suggestions that use provider constraints.
Depends on D220359
Differential Revision: https://phabricator.services.mozilla.com/D220359
Please see the bug for details.
This makes some changes to search strings and URLs in the tests that might look
unnecessary. They're due to switching to the QuickSuggestTestUtils helpers in
the head files instead of hardcoding the suggestions data. It's possible to
force the helpers to use the existing search strings and URLs that are used in
these tests, but it's good to be consistent with other tests that use these
helpers because it makes everything easier to understand.
Depends on D220352
Differential Revision: https://phabricator.services.mozilla.com/D219943
The error was:
ERROR: Unknown target name: "appendix b: using the webextensions api directly".
This is out of date documentation as we no longer support webextensions for dynamic result types.
Differential Revision: https://phabricator.services.mozilla.com/D220532
This keeps track of the enabled suggestion types that have been ingested while
the app is running. I call those types "fresh". In the code, they're stored in
the `#ingestedSuggestionTypes` set. Types that have not been ingested and types
that are disabled are called "stale". When a feature is updated, we update the
staleness bookkeeping in `#ingestedSuggestionTypes` and ingest only enabled
stale types.
This is similar to how each feature has an `#isEnabled` property. It's our
cached value of the last-known state that we can compare to the current state.
Depends on D219943
Differential Revision: https://phabricator.services.mozilla.com/D220352
Please see the bug for details.
This makes some changes to search strings and URLs in the tests that might look
unnecessary. They're due to switching to the QuickSuggestTestUtils helpers in
the head files instead of hardcoding the suggestions data. It's possible to
force the helpers to use the existing search strings and URLs that are used in
these tests, but it's good to be consistent with other tests that use these
helpers because it makes everything easier to understand.
Depends on D219942
Differential Revision: https://phabricator.services.mozilla.com/D219943
This replaces `UrlbarResult.exposureResultType` and `exposureResultHidden` with
a single simple value called `exposureTelemetry`. Please see the bug for the
rationale.
Other changes:
* For convenience, make `UrlbarPrefs.get("exposureResults")` return a `Set` so
consumers don't have to parse the string value.
* Simplify handling of result properties in `check_results()` (xpcshell tests)
* Add more tasks/checks to test_exposure.js
Depends on D219939
Differential Revision: https://phabricator.services.mozilla.com/D219942
As part of bug 1914535, I'd like to move some Suggest xpcshell test helpers from
head.js to QuickSuggestTestUtils so that other tests outside the quicksuggest
directory can use them and also so that Suggest helpers are more centralized.
This doesn't make any functional changes.
Differential Revision: https://phabricator.services.mozilla.com/D219939
