This mostly just moves all Suggest-related things from `UrlbarPrefs` to
`QuickSuggest`. To keep the patch small and make review easier, I tried to keep
it a straight move rather than making improvements, renames, and other
unnecessary changes. There's opportunity to improve and simplify some things,
and I'd like to do that in follow-ups.
Depends on D237281
Differential Revision: https://phabricator.services.mozilla.com/D237170
This makes a number of changes to support a new UI treatment for weather
suggestions in addition to the two existing treatments. Unfortunately we need
all three to experiment with, but hopefully after experimentation is done we can
remove the two existing treatments.
This patch refers to the new treatment as the "simplest" UI. The Figma is at [1]
although it doesn't reflect some details Josh and I have discussed. One of the
existing treatments is already called "simple", and this patch changes its name
to "simpler". The patch refers to the other existing treatment as "full". These
names are only informal because the Nimbus variable and fallback pref are
integers.
Because the new UI treatment is so simple, we can pretty much use the standard
rich suggestion UI for it. The existing treatments are dynamic. I say "pretty
much" because I did need to add a capability to rich suggestions,
`result.richSuggestionIconVariation`, which lets results set an `icon-variation`
attribute on their rows. Weather suggestions use it to choose the appropriate
weather icon in the CSS.
I also modified l10n string caching in a few ways to address a couple of
problems:
* I like how dynamic results can cache their strings lazily by setting
`cacheable` in their l10n objects. Standard results can only cache their
strings by adding them to `UrlbarView.#cacheL10nStrings()`. That method is
getting pretty big, and it also doesn't work well with strings that have
arguments since it's called to pre-cache strings before they're used, when
there are no arguments. So I extended `cacheable` support to all l10n objects,
not only those from dynamic results.
* Josh's spec calls for the temperature in the suggestion title to be bolded.
There's no good way to do that right now. We have highlights, but they aren't
useable for l10n strings. The simplest thing -- and what I've done before in
cases like this -- is to include `<strong>` tags in the l10n string itself.
That works except for when we have a cache hit with the l10n cache. In that
case, we set the `textContent`, which of course renders the tag literally. So
I added a `parseMarkup` option to l10n objects that says the cached string
should be set as `innerHTML` instead. Actually, for security I parse the
string into a sanitized document fragment. We could just always do this and
get rid of `parseMarkup`, but it seems wasteful since most l10n strings don't
need it.
* While addressing the previous two points, it made sense to move
`setElementL10n()` from the view to `L10nCache`. That way all these
cache-related functions are together.
Finally, Josh noticed one of the dark-mode colors in the icon SVG didn't have
enough contrast with the standard dark-mode panel background, so I updated that
to a new color he chose (from `#80808F` to `#9393A8`).
[1] https://www.figma.com/design/Hdi0oHB7trRcncyVAKZypO/accuweather-explorations?node-id=3548-15992&t=f4YUShEe5RkP2KEg-1
Differential Revision: https://phabricator.services.mozilla.com/D232703
This makes the "Show less frequently" behavior, prefs, and Nimbus variables for
weather sugestions more like those for Yelp suggestions. Yelp suggestions have
the best implementation for this and they're the model we should follow from now
on.
We don't have to worry too much about backward compatibility. Weather has never
been enabled by default except since late last week on Nightly. It has been
enabled in experiments, so there are some actual users out there who interacted
with them, but the worst that could happen is we'll lose the number of times
they clicked "show less frequently," so they may need to do that again if they
end up triggering weather suggestions again. This patch still respects the value
of the `weather.minKeywordLength` pref, which is the most important, so users
that previously incremented it will still see correct behavior.
Depends on D228742
Differential Revision: https://phabricator.services.mozilla.com/D228755
This adds a new Suggest backend for ML-based suggestions called
`SuggestBackendMl`. Before, with the JS and Rust backends, only one backend was
enabled at a time, but both the ML and Rust backends can be enabled at the same
time since we will want to serve suggestions from both for the foreseeable
future. Features can support ML suggestions by implementing the new
`BaseFeature.mlIntent` getter and handling ML suggestions in `makeResult()`.
Each feature can decide whether it supports ML suggestions and whether they
should be preferred over Rust suggestions.
I've updated the Yelp feature to hook into this, since Yelp suggestions are
supported by the ML model that Chidam is working on. If ML is enabled, then the
feature will only serve ML suggestions. I'm not sure if that's what we want long
term, but for now that will make it clear to people which backend is being used
while we develop this feature.
The `quickSuggestMlEnabled` variable/pref determines whether the ML backend is
enabled. The `yelpMlEnabled` variable/pref determines whether Yelp ML
suggestions are enabled. We can create similar variable/prefs for each feature
that supports ML suggestions so that they can be toggled independently of each
other.
Other changes:
Move the `is_sponsored` logic out of the Rust backend and into the provider.
Otherwise it would need to be duplicated in the ML backend too.
Depends on D224523
Differential Revision: https://phabricator.services.mozilla.com/D226736
This also removes VPN detection. That was only really necessary back when the
weather suggestion was zero-prefix, i.e., when it was shown simply by clicking
in the urlbar without typing anything. I think if you go to the trouble of
typing a weather keyword, we should show you the suggestion even if you're on a
VPN.
Differential Revision: https://phabricator.services.mozilla.com/D223874
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
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
