This removes "best match" as its own separate Firefox Suggest feature. In the
future, whether or not a suggestion is a best match (a.k.a. top pick) will be
determined by relevant product requirements. I've confirmed this with Nive.
Here's a summary of changes:
* Removes prefs and Nimbus variables related to best match
* Removes the "Top pick" checkbox in about:preferences
* Removes support for the `best_match` quick suggest config property. This
property was removed from the config in remote settings a while ago.
* Removes legacy telemetry scalars related to best match. These scalars were
added years ago for the original best match experiment and before we started
using Glean. In the case of non-sponsored suggestions, the scalars have not
been recorded at all for some time. In the case of sponsored suggestions, they
can now be recorded again due to the recent addition of sponsored priority
suggestions, but they are superseded by Glean.
Differential Revision: https://phabricator.services.mozilla.com/D190516
This expands D190144 so we check for actions as well as URLs. I also modified
tests for the affected row types so they test the visibility of the separator.
Differential Revision: https://phabricator.services.mozilla.com/D190494
This sets `has-url` on dynamic-result-type rows when their view template
contains a `.urlbarView-url` element. This fixes the missing separator in both
addon and weather rows.
This does not address one remaining problem: When `wrap` is set, the separator
is still missing in addon rows because they don't handle wrapping. Prior to
bug 1855639, they just always showed their title separator, and since they don't
wrap their URL, that was fine. I'd like to handle that separately by converting
addon rows from dynamic to standard. I filed bug 1857137 for that.
Differential Revision: https://phabricator.services.mozilla.com/D190144
The new Rust implementation of Suggest stores icons for some suggestion types in
its Sqlite database, and it returns these icons to consumers as byte arrays. To
show these icons in the view quickly and without any overhead, we can create
`Blob` URLs from the arrays. Blob URLs need to be revoked when we're done with
them to avoid leaking the backing data.
This patch implements some simple lifetime management of blob URLs in the view.
The first time the view shows a result, it creates a blob URL for the result's
icon. The view caches the URL while it remains open, so as the user continues to
type and possibly match the same result many times, the view will use the same
blob URL each time. When the view closes, it revokes the URL. This seems like a
reasonable, natural lifetime for these URLs, and the implementation is simple.
Depends on D189452
Differential Revision: https://phabricator.services.mozilla.com/D189615
The detail value for overflow events is:
0 - vertical
1 - horizontal
2 - both
This could cause the urlbar overflow state to be incorrect in some
cases.
Not sure how to test this, I found it via code inspection.
Differential Revision: https://phabricator.services.mozilla.com/D189547
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 enables addon suggestions by default (treatment B, no stars) for all
Suggest users. As discussed on Slack, I did not remove the related prefs and
Nimbus variables in case something goes wrong and we need to disable the feature
ASAP. In that case, we can ship a Nimbus rollout to re-disable it. After a
release cycle or two without any problems, we can do a follow-up that removes
them. I also left the treatment A implementation. We can remove that in a
follow-up too.
Since I did leave the prefs and variables, I only had to change their default
values to enable the feature with treatment B.
One problem is that addon suggestions should be best matches, but we don't want
to set `bestMatch.enabled` to true by default. To work around that, I set the
related best-match properties on the `UrlbarResult` inside `AddonSuggestions`
instead of delegating it to the quick suggest provider. Product has requested
that we remove the "Top pick" checkbox from about:preferences, and at that time
we should also remove the `bestMatch.enabled` pref. In the future, results
should be best matches on a case-by-case basis.
This makes a few UI changes that aren't strictly related but I wanted to fix
since more users will now see these suggestions:
* Cache the "Firefox extension" group label to prevent pop-in.
* Align the URL and row title by baseline. Right now the URL is too high above
the title.
* Set `inline-margin-end` of the icon so it's symmetrical with the starting
margin/padding. The current value of 8px is hardcoded and too small.
Differential Revision: https://phabricator.services.mozilla.com/D187640
Different providers have different time characteristics, reusing results across
providers may thus cause flicker of results coming from the fastest ones.
Differential Revision: https://phabricator.services.mozilla.com/D183507
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 goes through the previous changes in the dependencies of bug 877389, and does two things:
1) Remove instances of \n
2) Change reporting of exceptions so that they are passed as separate arguments. This should result
in an improved display of the exception in the browser console, should it occur.
Differential Revision: https://phabricator.services.mozilla.com/D180843
This implements most parts of Pocket suggestions. They don't need any special UI
or a dynamic result type because they're only shown as the usual best match rows
or non-best match rows.
Still to do:
* Implement the "Show less frequently" behavior once we decide what the keywords
will be and how that will work.
* Implement the bottom "Mozilla Pocket" text inside the suggestion row once it's
finalized. We can use the same technique we use to show the "Sponsored" bottom
text for adM suggestions.
Other changes this makes:
* Replace the `type=bestmatch` attribute with an `bestmatch` attribute. That
lets best-match rows have a `type` too, in this case `"pocket"` (actually
either `"rs_pocket"` or `"merino_pocket"`, since I'm using the telemetry
result type).
* Improve how UrlbarProviderQuickSuggest delegates to individual features when
getting result commands, view updates, handling commands, etc., so that we
don't need to add new `case` statements for each new type of suggestion.
Differential Revision: https://phabricator.services.mozilla.com/D180059
This uses the new label but only when the addon suggestion is shown as a best
match. Otherwise the suggestion will be shown in the usual Firefox Suggest group
so it should use that label.
I chose `-brand-product-name`, which is always "Firefox" even on Nightly. I
don't think it makes sense to talk about "Nightly extensions".
Differential Revision: https://phabricator.services.mozilla.com/D179868
This patch refactors the SearchService private initialized variables and how
the SearchService stores its initialization status. These changes capture whether
the SearchService has succeeded, failed or hasn't finished initialization yet.
There are also changes made to UrlbarSearchUtils on handling when SearchService
has failed to initialize or when it hasn't finished initializing yet and we wait.
In the case where the SearchService has failed on initialization, We allow
the code to continue so that it can reach UrlbarProviderPlaces. Once we
are able to reach UrlbarProviderPlaces, we can make database calls for the user's
history and bookmarks. This allows the user to interact with the addressbar and
search their history and bookmarks even if SearchService has failed to initialize.
Differential Revision: https://phabricator.services.mozilla.com/D176936
The problem is the row remains selected after its content is replaced with the
dismissal acknowledgment tip. This patch clears the selection and selects the
tip's "Got it" button if the row was selected.
I didn't add a general browser test for feedback and dismissal acknowledgments
earlier, so I added one now. I cp'ed the weather test and used it as a starting
point since it does test the acknowledgments but is specific to the weather
suggestion. Even though some of it now duplicates the new test, I left it intact
because we still need a test for the weather-specific commands, and the amount
of duplicated code isn't very much.
I modified the test helper function that opens the result menu so that the row
is not selected when `byMouse` is true. That way I can make sure the selection
doesn't change when the dismissed row is not selected. I used
`EventUtils.promiseElementReadyForUserInput()` to synthesize a mousemove over
the row to make the menu button visible. That function has a bug where the
interval is added after `onHit` is called and the promise is resolved, leaving
the interval ticking forever. Moving the `synthesizeMouseAtCenter()` after the
timeout is created fixes the problem.
Differential Revision: https://phabricator.services.mozilla.com/D178264
This does two things:
* Modify `nsXULPopupManager::ShowPopup()` so it calls `ShowPopupAsNativeMenu()`
as long as an anchor wasn't passed in, and only on Mac.
* Modify `-[MOZMenuOpeningCoordinator _openMenu:atScreenPosition:forView:withAppearance:]`
so it also takes a `aIsContextMenu` param. If the param is true, we synthesize
a right-click event and pop up a context menu as usual. If it's false, we use
`-[NSMenu popUpMenuPositioningItem:atLocation:inView:]` instead.
The reason this works is because `-[NSMenu popUpMenuPositioningItem:atLocation:inView:]`
opens the menu in a sensible place when the x-y coords are near the right edge
of the screen. In contrast, `+[NSMenu popUpContextMenu:withEvent:forView:]` will
anchor the menu's top-right corner to the mouse cursor when near the right edge.
Differential Revision: https://phabricator.services.mozilla.com/D177355
