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 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 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
If a provider onEngagement method causes the address bar panel to close when
a result is picked, we may record consecutive engagement and abandonment events
for the same search session.
Differential Revision: https://phabricator.services.mozilla.com/D206110
The problem has to do with hidden exposures whose hypothetical rows can't
immediately be made visible either because they would overflow the filled-up
view or because a misplaced result was seen.
The relevant part of `#updateResults()` is [here](https://searchfox.org/mozilla-central/rev/6a2a2a52d7e544a2fd5678d04991a7e78b694f22/browser/components/urlbar/UrlbarView.sys.mjs#1289-1291). When `canBeVisible` is false,
we effectively forget about that hidden-exposure result, which is not right. For
normal results, we also have that `canBeVisible` logic (a few lines down from
the part I linked), but the difference is that we create a hidden row for them
and then unhide the row when the query finishes and stale rows are removed.
We're missing an analog to that logic for hidden-exposure results, so we're
undercounting them.
Fortunately it's not too hard to fix. I added a new kind of exposure for
`TelemetryEvent` to track called "tentative" exposures. Tentative exposures are
entirely analogous to this case where a row would be added but hidden, and then
unhidden when stale rows are removed.
While I was modifying the test, I rephrased the comments for existing tasks so
hopefully they're easier to understand.
Differential Revision: https://phabricator.services.mozilla.com/D205155
#selectedElement may end up pointing to disconnected nodes. And so the public
.selectedElement getter.
This is how it was happening: a first call to onQueryResults adds and selects a
heuristic result. Then a second call to onQueryResults brings a new heuristic
result that requires new content (not compatible with the previous one), so the
old heuristic is emptied out, and new DOM is generated.
Because the code in onQueryResults relies on .selectedElement, at the second
invokation it thinks the selection is still valid, and doesn't select the new
heuristic. In reality .selectedElement at that time is pointing to a removed
DOM node.
The patch introduces a #rawSelectedElement and converts #selectedElement
into a getter.
Plus some minor logging improvements, and removing unused #mainContainer property.
Differential Revision: https://phabricator.services.mozilla.com/D195779
The problem is the two weather commands that keep the view open,
"inaccurate_location" and "show_less_frequently", aren't included in the
criteria that set `isSessionOngoing` to true.
As the comment above `isSessionOngoing` says, we should find a better way to
determine whether the session remains ongoing than hardcoding a list of
commands. I didn't try to do that here since we're time constrained and need to
uplift this to 114.
Differential Revision: https://phabricator.services.mozilla.com/D177558
This change adds support for exposure based experiments by allowing
a Nimbus variable/pref to specify the urlbar provider that should
trigger an exposure event as well as a secondary boolean variable/pref
that controls the visibility of the exposed result. The exposure should
be registered when a result 'can be added' but may or may not be shown
based on the value of the `displayExposureProvider` variable.
* Add exposure event to metrics.yaml
* Add new Nimbus variable (`exposureProvider`) to specify the urlbar
providers that should trigger exposure events .
* Add new Nimbus variable (`displayExposureProvider`) that controls the visibility
of the provider results that matched the `exposureProvider` variable.
Differential Revision: https://phabricator.services.mozilla.com/D174209
This removes `UrlbarProvider.pickResult()` and `blockResult()` in favor of
handling picks and dismissals through `onEngagement()`. A number of providers
use those two methods, so this revision touches a lot of files.
Handling dismissals through `onEngagement()` means `UrlbarInput.pickResult()`
can no longer tell whether a result is successfully dismissed, so it can't
remove the result anymore. (Maybe `onEngagement()` could return some value
indicating it dismissed the result, but I don't want to go down that road.)
Instead, I split `UrlbarController.handleDeleteEntry()` into two methods: a
public one that removes the result and notifies listeners, and a private one
that handles dismissing the selected result internally in
UrlbarController. Providers that have dismissable results should now implement
`onEngagement()` and call `controller.removeResult()`.
I made some other improvements to engagement handling. There's still room for
more but this patch is big enough already.
Other notable changes:
Include the engaged result in engagement notifications so providers have easy
access to it and can respond to clicks and dismissals more easily. That also
lets us stop passing `selIndex` and `provider` to `engagementEvent.record()`
since now it can compute those from the passed-in result.
Add the concept of `isSessionOngoing` to engagement notifications so providers
can tell whether an engagement ended the search session. Right now, providers
like quick suggest that record a bunch of provider-specific legacy telemetry
assume that `onEngagement()` ends the session, but that's no longer true.
Unify result buttons and result menu commands by setting
`element.dataset.command` on buttons (hopefully we can remove buttons soon, at
least the ones that aren't tip buttons)
Make sure we always notify providers on engagement even on dismissals or
when skipping legacy telemetry
Move dismissal of restyled search suggestions and history results from
`UrlbarController.handleDeleteEntry()` to the Places provider
Move dismissal of form history results from
`UrlbarController.handleDeleteEntry()` to the search suggestions provider
In the Places provider, remove the unused `_addSearchEngineMatch()` method. Also
remove the code in the "searchengine" case that creates a non-search-history
result. This code is unreached because the only time the provider creates a
"searchengine" match it also sets `isSearchHistory` to true.
In `UrlbarTestUtils.promiseAutocompleteResultPopup()`, change the default value
of the `fireInputEvent` param from false to true. This is necessary because
without a starting input event, the start event info in `engagementEvent` will
be null, so when `engagementEvent.record()` is called at the end of the
engagement, it will bail, and providers will not be notified of the engagement.
IMO true is a better default value anyway because input events will typically be
fired when the user performs a search.
Differential Revision: https://phabricator.services.mozilla.com/D174941
In the future, due to the result menu there may be other engagement types that
do not close the view, so we may need to revisit this, but for now the fix is
pretty simple.
Differential Revision: https://phabricator.services.mozilla.com/D174038
