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
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
