1657918 - Don't add an heuristic result in bookmarks/history/tabs search mode with an empty search string

Status: VERIFIED FIXED

(Firefox :: Address Bar, task, P2)

Description

[Marco Bonardo [:mak]]

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

[Harry Twyford]

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.

Comment 2

[Drew Willcoxon :adw]

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]

Comment 3

[Drew Willcoxon :adw]

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.

Comment 4

[Drew Willcoxon :adw]

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

Comment 5

[Drew Willcoxon :adw]

Attachment: Bug 1657918 - Don't add a heuristic result for empty searches in local search modes, and modify the view to allow it to stay open when empty.

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:

1. 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.
2. 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.
3. 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.

Comment 6

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b1486ea2d255be9a5ab36949c7e7bd16413480

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8961e024adfa
Don't add a heuristic result for empty searches in local search modes, and modify the view to allow it to stay open when empty. r=harry

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8961e024adfa

Comment 9

[Drew Willcoxon :adw]

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.

Comment 10

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified on latest Nightly 83.0a1 under Ubuntu 18.04 64-bit.