Replace UrlbarProvider.pickResult() and blockResult() with onEngagement()
Categories
(Firefox :: Address Bar, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox114 | --- | verified |
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
See the revision summary.
Assignee | ||
Comment 1•2 years ago
|
||
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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
Changing this to qe-verify+ -- this is a refactor so there should not be any changes visible to the user, but it's a big refactor with a large potential for regressions, so it's prudent to flag this for QA. And if any regressions are found in the near future related to clicks on urlbar results, they may be due to this bug.
There are no specific STR, we just want to make sure clicks on all the different types of urlbar results are still handled properly.
Comment 4•2 years ago
|
||
bugherder |
Comment 5•1 year ago
|
||
We verified this while performing the 114 Nightly Search Regression run across all OSs, regression bug 1831529 was submitted.
Description
•