Closed Bug 1491724 Opened 2 years ago Closed 2 years ago

Search engine keyword should be highlighted only when the search will actually be performed on the corresponding search engine

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox64 --- verified

People

(Reporter: bruce.bugz, Assigned: adw)

References

Details

Attachments

(4 files)

Attached image Capture.PNG
Configure Firefox as thus:
a) Have, say, Google set as the default search engine
b) Give a keyword of, say, `w` (without backticks) to the 'Wikipedia (en)' search engine
c) Check 'Show search suggestions in address bar results'
d) Uncheck everything under 'When using the address bar, suggest:'

Not really necessary to configure exactly as above, just that doing all of the above will make the STR easier to follow. FWIW, (d) can be skipped entirely (adapt the STR accordingly), (c) is default, and (a) can be whichever search engine came default for your Firefox.

STR:
1. In the address bar, type `w test`.
2. Notice that `w` is highlighted, as expected. Press enter. You are taken to a Wikipedia page, as expected.
3. In the address bar, type `w test` again.
4. Use cursor to scroll down to the second suggestion.

AR:
i) `w` is still highlighted in the address bar.
ii) But the selected entry says 'w test -- Search with *Google*', and has a lens icon instead of Wikipedia's icon
iii) If you press enter, the search is indeed performed with Google.

ER: `w` should not be highlighted if it is being interpreted as part of the search query to be sent to the default search engine.
Component: Activity Streams: Newtab → Address Bar
Thanks for filing this.
Assignee: nobody → adw
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Blocks: 1484737
My reasoning in https://bugzilla.mozilla.org/show_bug.cgi?id=1484737#c3 was wrong. I assumed that if a result started with a search alias then it had to be a search alias result, but that's clearly not true.

This is such a pain to get right, and all the messy logic in this patch reflects that.
Comment on attachment 9011064 [details]
Bug 1491724 - Search engine keyword should be highlighted only when the search will actually be performed on the corresponding search engine

Marco Bonardo [::mak] has approved the revision.
Attachment #9011064 - Flags: review+
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e5bf501404a
Search engine keyword should be highlighted only when the search will actually be performed on the corresponding search engine r=mak
Blocks: 1493536
Testing on 64.0a1 (2018-09-30) in WIndows 10, I noticed a small issue:

STR:
1. Give a keyword of `w` to the 'Wikipedia (en)' search engine
2. Type `w test` and press Enter.
3. Click on the url bar.

AR: `w` is briefly highlighted before the whole url is selected.
https://hg.mozilla.org/mozilla-central/rev/7e5bf501404a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Bruce, thank you.  I'll file a new bug for that.
Depends on: 1495614
Flags: qe-verify+
Hi,

I've reproduced the issue on Release 62.0.2 (64-bit), checked on Nightly 64.0a1 Build ID: 20181002100236 on Win 10 x64, macOS 10.12 and Ubuntu 16.04 and the issue is fixed, the Search engine keyword is highlighted only when the search will actually be performed on the corresponding search engine.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1497515
Attached patch Beta/63 patchSplinter Review
[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1480505

User impact if declined: We want to uplift other bugs related to activity stream and search keywords -- see bug 1496772. We might as well uplift this bug fix, too.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Been baking on Nightly for 1.5 weeks now, verified, and has automated tests

String changes made/needed: None
Attachment #9016493 - Flags: approval-mozilla-beta?
Comment on attachment 9016493 [details] [diff] [review]
Beta/63 patch

We already shipped our last beta last week and this is not a fix for an identified 63 regression but regular bug work for a feature with non default preferences set that would also require testing on beta from QA too, this doesn't seem like a good candidate for late betas or release, sorry.
Attachment #9016493 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.