If the user is modifying their search term, we suspect they don't immediately
want an autofilled url. makeQueryContext always enables autofill if
the allowAutofill is undefined.
Differential Revision: https://phabricator.services.mozilla.com/D233396
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 English restrict keyword strings to the enUS-searchFeature.ftl
file. The restrictKeywords providers return an array of keywords for their
l10nRestrictKeywords property. The first is the localized keyword, and the
second is the English keyword. If the user is in the English locale, the
providers return an array with one element: the English keyword.
Differential Revision: https://phabricator.services.mozilla.com/D230009
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
The purpose of this change is to cache persist state into an object so that in future patches, I can add other persisted search related state to it.
Differential Revision: https://phabricator.services.mozilla.com/D224122
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
This patch includes:
- A new UrlbarProviderRestrictKeywords class
- Showing localized Search with Bookmarks, Search with History, Search with Tabs results after the user types @
- Add search restrict keywords to preferences UI Search Shortcuts table
- Hiding search restrict keyword behind browser.urlbar.searchRestrictKeywords.featureGate pref
Differential Revision: https://phabricator.services.mozilla.com/D213697
There are a few ways to solve this problem and I tried to take a straightforward
approach. This patch makes text highlighting less automagical and surprising. It
forces dynamic-result providers to specify highlighted text content as part of
their view updates just like all other non-highlighted text and other
properties. To trigger highlighting, the view update can include a new
`highlights` property.
Except for Fakespot, there currently aren't any providers that take advantage of
the existing automagical highlighting behavior.
Depends on D216370
Differential Revision: https://phabricator.services.mozilla.com/D217020
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
The heuristic result is always kept first when receiving new results, that means
any result could be upgraded to be a heuristic result, including the first Top
Site, as in the zero prefix case there's no heuritic.
Thus it's safer to generate new DOM contents when upgrading between heuristic and
non-heuristic.
Also switch-tab results with container info have complex structure and should
generate new DOM contents, unless they are reused by other switch-tab results.
Unfortunately these changes are mostly visual and not easily testable, as it would
be very time consuming.
In general, for the long term, reusing the DOM of results is fragile and causes
nodes, attributes or classes to be ported over to incompatible results. We do it
to avoid flicker, but we must investigate better ways to do it.
Differential Revision: https://phabricator.services.mozilla.com/D204483
This makes sure the row label transfers to the dismissal acknowledgment tip, if
the dismissed row has a label.
With that fixed, there's one other cosmetic problem where the tip's top border
is right up against the row label. It doesn't look good. I added some additional
space between the label and border.
Differential Revision: https://phabricator.services.mozilla.com/D203559
This adds a "Local recommendations" group label for Yelp suggestions.
It also caches all Suggest group labels to avoid pop-in in the urlbar view. The
MDN and Pocket labels weren't being cached.
Differential Revision: https://phabricator.services.mozilla.com/D201157
Fixes code to properly run tests with the feature enabled.
Fixes code not considering payload.userContextId is set to -1 for private
windows.
Fixes a bug in the _openTabs Map where multiple open tabs to the same url are
not properly counted.
Differential Revision: https://phabricator.services.mozilla.com/D200036
This fixes the two `#addExposure()` calls in `UrlbarView.#updateResults()` so
that hidden exposures are properly counted. Right now we overcount them because
we record them in cases where the result would not have been shown anyway.
The new test tries to cover all possible `UrlbarView.#updateResults()` paths:
* When the view is full and a new result can't replace any of the old rows, the
new result is appended and hidden. If the query is then canceled, the new row
will never be shown
* When the vew isn't full, a new row can be appended and visible immediately
* When a new row replaces an old row, it can be visible immediately
Differential Revision: https://phabricator.services.mozilla.com/D199176
#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
This is a hack for the experiment and likely not ideal for screen readers, but better than what we have. I'm reaching out to a11y folks to figure out the right long-term solution here. I'll make an effort to get that ready for the experiment too, but would like to get this landed as a backup.
Differential Revision: https://phabricator.services.mozilla.com/D194501
