Bookmark keyword heuristic results are broken when a local shortcut is selected with the keyboard
Categories
(Firefox :: Address Bar, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox85 | --- | fixed |
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(2 files)
STR
- Set up a bookmark keyword, like a Bugzilla bookmark with a
b
keyword that can be used likeb <bug number>
- Type
b test
- 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.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
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 | ||
Comment 2•3 years ago
|
||
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
Comment 4•3 years ago
•
|
||
Backed out for perma failures.
Logs:
https://treeherder.mozilla.org/logviewer?job_id=323533745&repo=autoland&lineNumber=2766
Backout: https://hg.mozilla.org/integration/autoland/rev/b424ba535bfb6191e93b2d93f9375d5c092119ac
Assignee | ||
Comment 5•3 years ago
|
||
Looks like I need to update browser_oneOffs_searchSuggestions.js.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
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
Comment 8•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Description
•