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
Please see bug 1832927 for background. This fixes the problem on 115 by moving
the sponsored/nonsponsored logic back to the provider.
I added a task to test_quicksuggest_topPicks.js, and I created xpcshell tests
for addons and dynamic Wikipedia with similar tasks. It would be nice to unify
this check for all quick suggest types somehow -- maybe a task in
test_quicksuggest.js, but that's not quite as simple as it seems because each
suggestion type has its own suggestion object and expected result payload. We
might also want to add more tasks to these new files. We can think about that
later because there are other opportunities for test consolidation too.
Differential Revision: https://phabricator.services.mozilla.com/D177952
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
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 builds on D176779 and modifies Firefox to look for weather keywords and the
minimum keyword length in remote settings. It will still prefer the keywords and
length in Nimbus, falling back to remote settings when Nimbus doesn't define
them. That will allow us to store default values in remote settings while making
sure an experiment population uses a specific set of keywords for the duration
of the experiment.
As before, if no keywords are defined or the minimum keyword length is zero, the
suggestion will be shown on zero prefix.
Previously when we were planning to store keywords in remote settings, we were
going to store them in the quick suggest config data. After talking with Nan, we
decided to create a new data type instead, which should be more robust and is
the right way to do it. The `weather` record in remote settings will look like
this:
```lang=js
{
"keywords": ["weather", "forecast"],
"min_keyword_length": 3
"min_keyword_length_cap": 6
}
```
`min_keyword_length_cap` isn't used in this revision. I'll use it in a follow
up.
I cp'ed test_weather.js to test_weather_keywords.js to keep keyword-related
tasks in one file. The original file was getting too big.
Depends on D176779
Differential Revision: https://phabricator.services.mozilla.com/D175827
This refactors quick suggest remote settings management so it will be easier to
support new types of remote settings suggestions. It builds on D177191.
Currently, RemoteSettingsClient only handles adM and Wikipedia suggestions. It
would be possible for it to support more suggestions types by adding a series of
`if` statements, one per suggestion type (to check if the suggestion type is
enabled), with each statement fetching a specific suggestion type's data.
However, that doesn't scale elegantly.
It would also be possible for RemoteSettingsClient to be agnostic about the
suggestions in remote settings. IOW it doesn't need to care that different types
of suggestions are stored in RS. It could treat them all as one big map from
keywords to suggestions. However, that would require uniformity and stability in
how suggestions are stored in RS, and since the Suggest project is not mature,
that's not the case right now, and it won't be the case any time soon.
Instead, this revision moves suggestion-specific logic from RemoteSettingsClient
to BaseFeature classes specific to each suggestion type. Each BaseFeature
registers itself with remote settings using the new QuickSuggestRemoteSettings
module, fetches its specific RS data, and builds whatever data structure is
appropriate for serving its suggestions. I expect most features to use a simple
map from keywords to suggestions, like adM/Wikipedia does, so I factored out the
map-related code into a new SuggestionsMap class. The weather feature is an
exception because only its keywords are stored in RS, not the suggestion itself.
UrlbarProviderQuickSuggest accesses remote settings suggestions by going through
QuickSuggestRemoteSettings. It only needs to make one call.
This design should work well when the cross-platform Rust component is ready.
We should only need to modify UrlbarProviderQuickSuggest so it fetches
suggestions from the Rust component instead of QuickSuggestRemoteSettings.
Depends on D177191
Differential Revision: https://phabricator.services.mozilla.com/D176779
This makes some changes to UrlbarProviderQuickSuggest that are much more modest
than my previously proposed refactorings in D176111 and D175998. After thinking
about it more and discussing it with @wstuckey and @daisuke, I'm not sure it's a
good idea to split UrlbarProviderQuickSuggest into multiple providers. In the
future, we will replace a lot of our desktop JS with a single cross-platform
Rust component that serves remote settings suggestions and possibly Merino
suggestions too. We should work with that in mind, and I don't think it makes a
lot of sense to have multiple urlbar providers all fetching from this one
component, even though it would make some things nicer, like being able to
isolate suggestion-specific UI code to each provider.
The main goal of this revision is to better prepare UrlbarProviderQuickSuggest
for many new types of suggestions:
* Add `#makeResult()`, which returns a new `UrlbarResult` appropriate for a
given suggestion
* Add `#makeDefaultResult()`, which returns a result for suggestions that either
don't need special handling or are unrecognized
* Remove the `RESULT_SUBTYPE` consts. The idea is that the client doesn't need
to care too much about the types of results Merino returns, except when
special handling is required (special UI, special telemetry, etc.)
* Add `telemetryType` to result payloads so that the result types recorded in
Glean are clear and well defined. `telemetryType` is also used to tell the
type of a result in general. For results that are served by Merino,
`telemetryType` is the Merino provider name
* Streamline legacy telemetry handling a little, although hopefully we won't be
doing too much legacy telemetry in the future
There are still open questions that this revision does not resolve, especially
the ability isolate suggestion-specific UI code in a nice way (dynamic result
types). I don't think this revision paints us into any corners.
Other changes:
* Properly document the `${source}_${type}` result types in metrics.yaml (added
in D174209)
* For Glean result types, replace "suggest_sponsor" with "merino_adm_sponsored"
and "rs_adm_sponsored", and replace "suggest_non_sponsor" with
"merino_adm_nonsponsored" and "rs_adm_nonsponsored". This is more consistent
with the `${source}_${providerName}` convention I'd like to establish.
* Remove code related to Nimbus exposures and a test
(browser_quicksuggest_bestMatch.js). We aren't using Nimbus exposures anymore.
We can always add it back if necessary.
* Don't record the custom contextual services pings for dynamic Wikipedia
results. These pings are only necessary for adM suggestions.
Differential Revision: https://phabricator.services.mozilla.com/D177191
