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
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 fixes the bug by not starting fetches until a config is set from either
remote settings or Nimbus. By "config" I mean keywords basically, but we sync
more than keywords -- the min keyword length and min keyword length cap -- so
that's why I use different term. So, before remote settings is synced, no config
will be set, so no fetches will happen, so the suggestion will be null. When the
urlbar provider starts a query, it will see the suggestion is null and not add a
result. Once a config is set from RS or Nimbus, we'll start fetching.
Currently we allow zero prefix when keywords or the min keyword length aren't
set. This patch removes that functionality because on second thought, there's
not a safe and obvious way to support zero prefix using these keyword-related
config properties/variables by themselves, and zero prefix isn't a feature
requirement anymore anyway. If we wanted to keep supporting it with these
properties/variables, there are a few options, and I don't like any of them:
* If `keywords` is undefined or null, use zero prefix. This is dangerous because
we may make a mistake in RS or Nimbus and forget to set it. Also, we use null
as the default value for the Nimbus var, and since we use UrlbarPrefs to
access Nimbus vars, there's no good way to tell whether null was set
intentionally or not.
* If `keywords` is an empty array, use zero prefix. This is awkward because the
user can now increment the min keyword length, which means the keywords array
kept by `Weather` can become empty when the min keyword length is incremented
a lot. In that case, no suggestion should be shown at all.
* If `min_keyword_length` is zero/falsey, use zero prefix. This has the same
problems as the first point above.
If we do need to support zero prefix again in the future, I think we should add
an RS property/Nimbus variable that makes it clear what's happening, e.g., a
`useZeroPrefix` boolean.
I removed the exposure scalar because it's entirely based on zero prefix. We can
use Glean for that now anyway.
I also noticed weather suggestions are case insenstive, so I fixed that and
added a test task.
Differential Revision: https://phabricator.services.mozilla.com/D177448
This fixes the bug by not starting fetches until a config is set from either
remote settings or Nimbus. By "config" I mean keywords basically, but we sync
more than keywords -- the min keyword length and min keyword length cap -- so
that's why I use different term. So, before remote settings is synced, no config
will be set, so no fetches will happen, so the suggestion will be null. When the
urlbar provider starts a query, it will see the suggestion is null and not add a
result. Once a config is set from RS or Nimbus, we'll start fetching.
Currently we allow zero prefix when keywords or the min keyword length aren't
set. This patch removes that functionality because on second thought, there's
not a safe and obvious way to support zero prefix using these keyword-related
config properties/variables by themselves, and zero prefix isn't a feature
requirement anymore anyway. If we wanted to keep supporting it with these
properties/variables, there are a few options, and I don't like any of them:
* If `keywords` is undefined or null, use zero prefix. This is dangerous because
we may make a mistake in RS or Nimbus and forget to set it. Also, we use null
as the default value for the Nimbus var, and since we use UrlbarPrefs to
access Nimbus vars, there's no good way to tell whether null was set
intentionally or not.
* If `keywords` is an empty array, use zero prefix. This is awkward because the
user can now increment the min keyword length, which means the keywords array
kept by `Weather` can become empty when the min keyword length is incremented
a lot. In that case, no suggestion should be shown at all.
* If `min_keyword_length` is zero/falsey, use zero prefix. This has the same
problems as the first point above.
If we do need to support zero prefix again in the future, I think we should add
an RS property/Nimbus variable that makes it clear what's happening, e.g., a
`useZeroPrefix` boolean.
I removed the exposure scalar because it's entirely based on zero prefix. We can
use Glean for that now anyway.
I also noticed weather suggestions are case insenstive, so I fixed that and
added a test task.
Differential Revision: https://phabricator.services.mozilla.com/D177448
This increments the minimum keyword length when the user clicks the "Show less
frequently" command for the weather suggestion. It adds a pref to keep track of
the current min length. If the pref is zero, we use the min length in Nimbus or
remote settings.
There is a limit to the number of times "Show less frequently" can be clicked.
This patch calls it the cap. Once the cap is reached, the min length can't be
incremented any more and the command is not shown in the menu again. The cap can
be set in Nimbus and remote settings.
This patch also modifies UrlbarPrefs by making it possible to remove observers.
`Weather` needs to listen for changes to the `weather.minKeywordLength` pref,
and when the weather feature is disabled, it needs to stop listening.
Depends on D175827
Differential Revision: https://phabricator.services.mozilla.com/D177218
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 implements the weather suggestion result menu UI and builds on D174941.
References:
* [Spec]( https://www.figma.com/file/Hdi0oHB7trRcncyVAKZypO/accuweather-explorations?node-id=2421%3A62540&t=29w6wH3UYchqBxqX-1) (See "A11y review" in the sidebar)
* [Clickable prototype](https://www.figma.com/proto/Hdi0oHB7trRcncyVAKZypO/accuweather-explorations?page-id=2192%3A42825&node-id=2394-52468&viewport=246%2C526%2C0.12&scaling=min-zoom&starting-point-node-id=2394%3A52468&show-proto-sidebar=1) (See "Revised 4/3" in the sidebar)
There are a couple important points about the menu. First, one of the commands,
"Report inaccurate location", is specific to weather suggestions, or at least
location-based suggestions. I don't think it's a good idea to centralize all
commands in UrlbarView, and in general I'd like to stop centralizing handling of
different result types in the view and input, so I added a new provider method
called `getResultCommands()`.
Second, the spec calls for a menu separator and a submenu so the user can select
a reason they don't want to see the result, so the return value of
`getResultCommands()` is flexible enough to support those two things, and I
modified `#populateResultMenu()` too.
These new commands will be recorded in Glean engagement telemetry as new
`engagement_type` values, same as "dismiss" and "help" currently are.
This patch doesn't implement handling of two of the commands, "Report inaccurate
location" and "Show less frequently", because I wanted to keep it focused on the
fundamentals described above.
Depends on D174941
Differential Revision: https://phabricator.services.mozilla.com/D174994
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 bug hits [this path](https://searchfox.org/mozilla-central/rev/3ba3d0a57b6419206f82f80cd6c30faf59397664/browser/components/urlbar/UrlbarView.sys.mjs#549-550) in `#autoOpen`, where it re-uses the current rows as
they exist. Overflow and underflow events aren't fired in that case, so the
`overflow` attribute isn't updated. The bug does not happen when the `else`
branch is hit because `onQueryResults()` clears the rows when the view isn't
open, and after that the rows are rebuilt.
This patch makes us hit the `else` path in this case by storing the width of the
input when the view is closed. If the stored width is different from the current
width, then the overflow state may be incorrect. Taking the `else` branch makes
us go through `onQueryResults()` and clear the rows before opening the view.
This fixes this bug and bug 1759857.
Differential Revision: https://phabricator.services.mozilla.com/D173596
