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

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Bruce]

Attachment: 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.

Comment 1

[Drew Willcoxon :adw]

Thanks for filing this.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1491724 - Search engine keyword should be highlighted only when the search will actually be performed on the corresponding search engine

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 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
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

Comment 5

[Bruce]

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.

Comment 6

[Bruce]

Attachment: Keyword briefly highlighted.gif

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7e5bf501404a

Comment 8

[Drew Willcoxon :adw]

Bruce, thank you.  I'll file a new bug for that.

Comment 9

[laszlo.bialis]

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.

Comment 10

[Drew Willcoxon :adw]

Attachment: Beta/63 patch

[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

Comment 11

[Pascal Chevrel:pascalc]

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.