Closed Bug 1678765 Opened 4 years ago Closed 4 years ago

Bookmark keyword heuristic results are broken when a local shortcut is selected with the keyboard

Categories

(Firefox :: Address Bar, defect, P3)

defect
Points:
3

Tracking

()

RESOLVED FIXED
85 Branch
Iteration:
85.2 - Nov 30 - Dec 13
Tracking Status
firefox85 --- fixed

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(2 files)

Attached image screenshot

STR

  1. Set up a bookmark keyword, like a Bugzilla bookmark with a b keyword that can be used like b <bug number>
  2. Type b test
  3. Arrow down to select a local shortcut one-off

The result keeps its "bugzilla.mozilla.org: test" title, but the "Search <local shortcut>" action text is right up against the title. I'll attach a screenshot.

When you press enter, we confirm the local search mode with "b test" as the query string. When you click the result, we perform the bookmark keyword, i.e., we visit https://bugzilla.mozilla.org/show_bug.cgi?id=test. So this is both a visual bug and an actual behavior inconsistency. We should confirm search mode when you click the result, same as pressing enter.

I found this while looking at bug 1678155. I might end up fixing the behavior inconsistency there since it's basically the same problem, not sure yet.

See Also: → 1678155

Bug 1678155 fixes the problem where clicking the result does the wrong thing, but it doesn't fix the "Search <local shortcut>" text's being flush against the result title.

Assignee: nobody → adw
Severity: S3 → S4
Status: NEW → ASSIGNED
Iteration: --- → 85.2 - Nov 30 - Dec 13

This was a little harder than it seemed like it should be because we show/hide
the title separator in two places, in UrlbarView.updateRow and in the CSS.
This patch gets rid of the show/hide in updateRow, so now we show/hide only in
the CSS. The decision to show/hide in updateRow was based on whether the
result has action text (actionSetter) or is a URL (setURL). We already had a
has-url attribute that corresponds directly to setURL, so adding a similar
has-action attribute that corresponds to actionSetter lets us show/hide only
in the CSS.

The second part of this patch is to actually fix the bug. For that, the existing
show-action-text attribute does what we want in the CSS; the only problem is
that we currently set it only when there's no selected row, but we need to also
set it when there's no selected row but a one-off is selected. We have a similar
block -- the one with a comment about restyling the heuristic to look like a
search result -- so I removed the block that currently sets/removes the
attribute and moved the set/removal there. I also renamed the attribute
restyled-search to better semantically describe what's happening, but if you
disagree I can restore the old name.

Depends on D97843

Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a507222dac63
Fix broken "Search with" action text for heuristic results that are restyled to look like search results. r=harry

Looks like I need to update browser_oneOffs_searchSuggestions.js.

Flags: needinfo?(adw)
Points: 2 → 3
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/449c45aa4088
Fix broken "Search with" action text for heuristic results that are restyled to look like search results. r=harry
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
QA Whiteboard: [qa-85b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: