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 patch adds the keywords to the result title for TokenAliasEngine and
RestrictKeyword results. The TokenAliasEngine results may have more than one
keyword if the user has set a custom keyword for that engine. This can help
remind the user what keywords are available as they are typing or if they
have forgotten a keyword, they can type `@` to quickly see the list of keywords
to search with.
The result title looks like this:
For Engines:
@[keywords] - Search with [Engine]
For Restrict Keywords:
@[keyword] - Search [Restrict]
Differential Revision: https://phabricator.services.mozilla.com/D224017
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
We no longer need to start the add-on manager before the search service starts, so init it only when it is needed for installing add-ons.
Differential Revision: https://phabricator.services.mozilla.com/D216406
This makes a small change to the origins frecency by introducing a cut-off
of 90 days to the sum of frecencies in the origin. That will make the ranking
more recency prone. Bookmarks continue being autofilled regardless.
This also simplifies the threshold removing the stddev that we pretty much
never used.
Differential Revision: https://phabricator.services.mozilla.com/D201723
In the end I wasn't able to remove underscores on a lot of these since the base
`UrlbarProvider` class defines getters and methods with the same names, but at
least `results` is no longer "private".
Depends on D199176
Differential Revision: https://phabricator.services.mozilla.com/D199594
Until now we updated origins frecency using direct SQL triggers.
While that guaranteed good performance, it also had some downsides:
* replacing the algorithms is complicate, the current system only works
with a straight sum of page frecencies. We are planning to experiment with
different algorithms in the future.
* it requires using multiple temp tables and DELETE triggers, that is error
prone for consumers, that may forget to DELETE from the temp tables, and thus
break data coherency.
* there's not much atomicity, since the origins update must be triggered apart
and a crash would lose some of the changes
This patch is changing the behavior to be closer to the recalc_frecency one that
is already used for pages.
When a page is added, visited, or removed, recalc_frecency of its origin is set
to 1. Later frecency of invalidated origins will be recalculated in chunks.
While this is surely less efficient than the existing system, it solves the
problems presented above.
A threshold is recalculated at each chunk, and stored in the moz_meta table.
This patch continues using the old STATS in the moz_meta table, to allow for
easier downgrades. Once a new threshold will be introduced we'll be able to
stop updating those.
The after delete temp table is maintained because there's no more efficient way
to remove orphan origins promptly. Thus, after a removal from moz_places,
consumers MUST still DELETE from the temp table to cleanup orphan origins.
This also introduces a delayed removal of orphan origins when their frecency
becomes 0.
Differential Revision: https://phabricator.services.mozilla.com/D186070
The problem is that showing the dismissal acknowledgment tip does not remove the
result from the query context, so when the view later opens and reuses the
cached query context, it still has the dismissed result in it.
At first I thought I should modify `UrlbarView.acknowledgeDismissal()` so it
either invalidates the context cache or removes the result from the cached
context, but I saw that dismissals without the acknowledgment tip do not have
this problem. That's because they go through `UrlbarController.removeResult()`,
which removes the result from the context and then notifies the view.
The real problem is that I implemented dismissal acknowledgments wrong. They
should start by going through `UrlbarController.removeResult()` too.
This patch updates all callers of `acknowledgeDismissal()` so they call
`removeResult()` instead. To signal that an acknowledgment should be shown, they
first set a `result.acknowledgeDismissalL10n` property. When the view is
notified that a result was removed, it shows the tip if this property is
present.
Differential Revision: https://phabricator.services.mozilla.com/D187830
This does a few things:
* Unify the view implementations of rich suggestions, Firefox Suggest sponsored
results, and best match. I did this by using the best match implementation
and extending it to rich suggestions and Suggest sponsored.
* Use the unified implementation for Pocket suggestions too.
* Add a bottom-text concept since Pocket suggestions shown as top picks need to
show both a description and some text below it. (The actual bottom text per
result is added in D182632 since I didn't want to make this patch bigger than
necessary)
I have a couple motivations for these changes:
* I'm implementing Pocket suggestions, which need to show some text below the
suggestion title as well as the URL. I was going to just use the Firefox
Suggest sponsored approach, where the action text is wrapped below the title,
but that doesn't work because it can't show both the wrapped action text and
the URL.
* IMO we should use rich suggestions as the basis for all rows going forward,
i.e., unify the different row implementations around rich suggestions.
The reason I chose the best match implementation instead of the rich suggestions
implementation is because the grid-based approach of rich suggestions doesn't
work well when the URL also needs to be shown. The URL should be
baseline-aligned with the row title, which isn't easy to do when the URL is
outside the grid. The rich suggestions implementation also doesn't wrap the URL.
Other details:
* The `rich-suggestion=no-icon` attribute value is only used for styling, so we
can replace it with `@supports -moz-bool-pref()`. That lets us make the
`rich-suggestion` attribute a simple boolean.
* I kept the `isBestMatch` property for results since
`searchEngagementTelemetryGroup()` uses it to return "top_pick", and the view
also uses it to create the "Top pick" row/group label. It still has semantic
meaning so I think that's OK. It's no longer used by the view to create
different DOM or styling.
* Move `isRichSuggestion` from the payload to the result itself, since it's no
longer used for only one type of result. It's like `isBestMatch`, which is
also on the result.
* Add `richSuggestionIconSize` to the result too. The best match icon size
is 52. The Pocket best match icon size is 24 (but will have added padding and
a background color to make it appear 52px). IMO this is better than adding
rules for each type of suggestion to the CSS. It's cleaner and also indicates
what the "standard" icon sizes are.
Depends on D182580
Differential Revision: https://phabricator.services.mozilla.com/D182537
This keeps the current behavior where Firefox shows a suggestion as a top pick
when `is_top_pick` is true, but in addition the two best-match prefs must also
be true.
I cp'ed test_quicksuggest_bestMatch.js to test_quicksuggest_topPicks.js so that
we have a test specifically for top picks, and I added tasks for all preference
combinations. The terms "best match" and "top picks" are overloaded and I tried
to explain what they mean in the code comments I added.
Depends on D176114
Differential Revision: https://phabricator.services.mozilla.com/D177712
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
This adds a new pref, `browser.urlbar.weather.zeroPrefix`. When true, weather
suggestions are shown on "zero prefix" like they are now, which means they're
shown when the user focuses the urlbar and before they type anything. When the
pref is false, the weather provider will show suggestions only if the search
string matches a keyword in a set of keywords managed by `QuickSuggest.weather`.
My plan is to store the keywords in the quick suggest config object in remote
settings. Nan suggested this too. Currently the config does not contain any
keywords, but this patch can be tested by setting the pref to false, running the
following in the browser console, and then typing "weather" in the urlbar:
```lang=js
ChromeUtils.importESModule("resource:///modules/QuickSuggest.sys.mjs")
.QuickSuggest.remoteSettings._test_setConfig({
weather_keywords: ["weather"],
});
```
Other changes:
* It's possible for a keyword to match both the weather suggestion and a quick
suggest suggestion. Only the weather suggestion should be shown. This patch
modifies to the muxer for that.
* This modifies `UrlbarView.#rowLabel()` to show the "Top pick" (a.k.a. best
match) label for keyword-based weather suggestions, in addition to zero-prefix
suggestions like we currently do.
* This modifies the remote settings client so it becomes enabled when keyword-
based weather suggestions are enabled, so that the config can be accessed.
Depends on D168738
Differential Revision: https://phabricator.services.mozilla.com/D168757
`QuickSuggestTestUtils.ensureQuickSuggestInit()` was written before Merino, so
it assumes the suggestion objects passed in are remote settings suggestions.
This revision modifies it so Merino and remote settings suggestions can both be
passed in. That makes it a little nicer for tests that need to test Merino
suggestions in particular, like navigational suggestions, dynamic Wikipedia,
etc.
Another motivation for this change is that it makes it clear which type of
suggestion is being passed to `ensureQuickSuggestInit()`. Unfortunately Merino
suggestion objects are slightly different from remote settings result objects
(`block_id` vs. `id` for example), which are both different from UrlbarResult
objects, and it can be confusing when reading tests. Since "result" is the name
of remote settings objects used internally in the remote settings client, I've
used that term here, and I've updated all callers to use it instead of
"suggestion".
This also makes `MerinoTestUtils` and `QuickSuggestTestUtils` singletons.
Otherwise the new `MerinoTestUtils` instance used inside `QuickSuggestTestUtils`
isn't the same as the one used in the test that calls into
`QuickSuggestTestUtils`, which is very confusing. This made me realize it's a
good idea for these test utils objects to be singletons.
Finally I removed `is_top_pick` handling from the remote settings client and
remote settings suggestions, since the related test is now using Merino. I also
removed `_test_is_best_match` since only one test was using it and it's not
necessary.
Depends on D166019
Differential Revision: https://phabricator.services.mozilla.com/D166050
