Closed Bug 1657918 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Address Bar, task, P2)

task
Points:
3

Tracking

()

VERIFIED FIXED
81 Branch
Iteration:
81.2 - Aug 10 - Aug 23
Tracking Status
firefox81 --- verified

People

(Reporter: mak, Assigned: adw)

References

Details

Attachments

(1 file)

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.

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.

Points: --- → 2
Priority: P3 → P2
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 81.2 - Aug 10 - Aug 23

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]

Flags: needinfo?(mverdi)

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.

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

Points: 2 → 3

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.

Pushed by dwillcoxon@mozilla.com: 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
Regressions: 1659011
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

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.

Flags: needinfo?(mverdi)
Summary: Don't add an heuristic result in bookmarks/history/tabs search mode → Don't add an heuristic result in bookmarks/history/tabs search mode with an empty search string

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?

Flags: needinfo?(adrian.florinescu)

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

Status: RESOLVED → VERIFIED
Flags: needinfo?(adrian.florinescu)
Regressions: 1668389
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: