Don't add an heuristic result in bookmarks/history/tabs search mode with an empty search string
Categories
(Firefox :: Address Bar, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox81 | --- | verified |
People
(Reporter: mak, Assigned: adw)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
showing a search heuristic in these cases doesn't make sense.
Or probably in general it doesn't make sense to show a search heuristic result for the empty string, and that would be a better catch-all fix.
Comment 1•4 years ago
|
||
I spoke with Verdi and he agrees that we shouldn't show a search heuristic result for the empty string. In search modes that have nothing to show for the empty string (search engines with no form history, a local search mode with nothing to show) we should still open the view, but only show the one-offs flush against the input. This is covered in the InVision spec.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
Does it make sense to show the heuristic at all during local search modes? It's a little weird to be in the "Bookmarks" mode and have the heuristic and default action be "Search with Google." The InVision shows empty searches but not non-empty searches. If anything, a heuristic for bookmarks might be to open the bookmarks library with the given search... [Edited]
Assignee | ||
Comment 3•4 years ago
|
||
Something else this reveals is that if a query doesn't have any results, we close the view: https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/browser/components/urlbar/UrlbarView.jsm#507
That's not what we want when in search mode. As mentioned, we want to keep the view open with only the one-offs shown. I'm hitting that in tests now that I stopped returning a heuristic for local one-offs when the search string is empty. When I fixed it, the separator line between the results and one-offs is still shown, so then there are two separators: the ones above and below the results. We'll need to hide the bottom separator in this case like we do for search tips.
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
(In reply to Marco Bonardo [:mak] (OoO 09-23 Aug) from comment #0)
Or probably in general it doesn't make sense to show a search heuristic result for the empty string, and that would be a better catch-all fix.
This isn't possible in general without special-casing search tips, since the onboarding search tip is a heuristic because we want its button to appear selected. Since we must special-case something, I'm not sure it's a good idea doing it in general. AFAICT the only provider that provides a heuristic for the empty string other than search tips is search suggestions.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
This excludes the heuristic for empty searches when in search mode. I haven't
heard back from Verdi yet about excluding it for all searches in search mode. We
can add that in a follow-up if necessary.
Since we're now excluding the heuristic but we want the view to remain open,
it's possible for the view to be empty while it's open. I had to make some
changes to allow that to happen. There are three cases I want to call out:
- When the search string is empty, the view shows top sites. If you then enter
search mode and the resulting search doesn't return any results, we
previously closed the view. This patch keeps it open. An example of this
scenario is when you don't have any bookmarks and you click the bookmarks
one-off. - When the urlbar isn't focused, it's in search mode, the input is empty, and
the search didn't return any results, then focusing the urlbar didn't do
anything previously. This patch auto-opens the view. - When the input contains a single char and it's in search mode, deleting the
char previously closed the view if the empty search didn't return any
results. This patch keeps the view open.
When the view is empty, we also need to hide the one-offs' top border that
usually separates them from the results. Otherwise there are two separator lines
right next to each other, the one above the one-offs and the one at the bottom
edge of the input. I don't think there's any CSS selector that will let us
easily do this due to the DOM structure, so I added a new noresults
attribute
on the view for this.
I had to call context.searchString.trim()
to tell whether the search string is
empty. Since we do that in a bunch of places, I added
context.trimmedSearchString
, and a lot of this patch is replacing those trim
calls.
Assignee | ||
Comment 6•4 years ago
|
||
Comment 8•4 years ago
|
||
bugherder |
Assignee | ||
Comment 9•4 years ago
|
||
We got clarification from Verdi, and we want to do what the original of summary of this bug said: Don't show a heuristic for local search modes at all. I'll file a follow-up bug. We weren't sure in this bug, and we ended up not showing it for local search modes only when the search string is empty, so I'll also update the summary to reflect that.
Assignee | ||
Updated•4 years ago
|
Comment 10•4 years ago
|
||
I verified this issue using 82.0a1 (2020-09-14) on macOS 10.13 and Windows 10 x64.
Adrian could you help me with Ubuntu verification?
Comment 11•4 years ago
|
||
Verified on latest Nightly 83.0a1 under Ubuntu 18.04 64-bit.
Description
•