1484737 - Search engine aliases not rehighlighted when reshowing text (from autocompletion)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Ed Lee :Mardak]

From bug 1479806 comment 36, type "@amazon" then press down to some autocomplete entry (past the first result), then move back into the address bar.

Maybe there needs to be a check from "input"  ?? if reshowing the text triggers that…

Comment 1

[Ed Lee :Mardak]

Design agrees this should get rehighlighted but okay to not do so for 62. Also that when the highlight becomes more interactable, it should correctly visually show that it's a "button" as a hint to the user.

Comment 2

[Drew Willcoxon :adw]

We should probably check for the highlight on selection changes.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1484737 - Improve the handling of search alias highlighting in the urlbar.

This has two parts:

(1) urlbar already had a formatValue method. Right now, it only does the URL formatting (domain highlighting, crossing out https for mixed content pages) that we do when the urlbar is not focused. This patch generalizes that method into a kind of "any formatting you want to do, do it here" method, and it adds alias formatting.

formatValue is called by the base autocomplete binding when `value` is set. So it's called when the selection in the popup changes and the autocomplete controller subsequently sets the input value. (It's also called by urlbar on focus and blur.) And if anyone else sets the value directly, it'll be called then too of course.

But it's not called when you're just typing in the input, so I added a call in urlbar.onResultsAdded, where we were calling highlightSearchAlias, to handle the first heuristic result being added or modified as a result of what you type.

So I think that should cover all possible times we need to highlight the alias?

(2) Just looking at the selected result to get the alias in the input doesn't work all the time. If you click a search tile on newtab and then key around in the popup, sometimes when you key down to the one-off buttons, the input value reverts to the alias (it's the user-typed value I guess?), but at the time that the value setter is called during the revert, the popup's selected index is still the last selection in the popup. IOW the selected index doesn't match up with what's in the input.

Rather than deal with that, it seems safer to call PlacesSearchAutocompleteProvider.findMatchByAlias() on the first word in the input. But that has a couple of problems. It's async, and I noticed there can be a slight delay in the highlighting appearing. Also, we've already gotten the information returned by that method, when we generated the results in the first place, so it seems inelegant to call it again.

So what I've done instead is to cache aliases in the popup when results are added, and then just look up the first word in the input in these aliases. We shouldn't reset this cache until the first result of a new search comes in.

Comment 4

[Ed Lee :Mardak]

Comment on attachment 9002652 [details]
Bug 1484737 - Improve the handling of search alias highlighting in the urlbar.

Ed Lee :Mardak has approved the revision.

Comment 5

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e42c9b6acdba286b9d1865409b283742dd50d397

Comment 6

[Drew Willcoxon :adw]

According to that try push, there's a failure in browser_urlOverflow.js related to the scheme.  I'll figure that out next week.

Comment 7

[Marco Bonardo [:mak]]

when adding new reviewers, please always use blocking reviews, otherwise the review request is invisible in phabricator

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 9002652 [details]
Bug 1484737 - Improve the handling of search alias highlighting in the urlbar.

Marco Bonardo [::mak] has approved the revision.

Comment 9

[Drew Willcoxon :adw]

Ah, sorry.

Today is merge day, so I'll wait to land this until tomorrow or later, and then maybe request uplift.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b63ab504b03
Improve the handling of search alias highlighting in the urlbar. r=Mardak,mak

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/6b63ab504b03

Comment 12

[Drew Willcoxon :adw]

Setting firefox63 status to fix-optional.  Would be nice to uplift this.

Comment 13

[Drew Willcoxon :adw]

STR for QA:

1. Open a new tab
2. Click the @google Top Sites tile
3. Verify that "@google" is filled in the address bar and highlighted in blue, and the urlbar is focused
4. Type "foo" so that the urlbar contains "@google foo" and hit enter
5. Verify a search for "foo" is done on Google
6. Open a new tab
7. Click the @google tile again
8. Verify that "@google" is filled in the address bar and highlighted in blue, and the urlbar is focused
9. Press the Down arrow key to cycle through all the results in the urlbar popup, until you get back to the first @google result
10. Verify that "@google" is filled in the address bar and highlighted in blue
11. Remove all the text in the urlbar
12. Type "@google"
13. Verify that "@google" is highlighted in blue
14. Press the Down arrow key to cycle through all the results and the one-off search buttons in the urlbar popup
15. Verify that when "@google" is in the urlbar, it's highlighted in blue

Comment 14

[Drew Willcoxon :adw]

Attachment: Beta/63 patch

Approval Request Comment
[Feature/Bug causing the regression]:
@google search/shortcut tiles on the new tab page -- bug 1480505 added the blue highlight in the urlbar that this bug is about, and bug 1479806 is the main meta bug for this project

[User impact if declined]:
The original bug (bug 1480505) landed in 62, and this bug landed in 64.  So if declined, users will have to wait for two releases before this bug is fixed.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Not yet

[Needs manual test from QE? If yes, steps to reproduce]:
Yes, please see comment 13

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Low risk

[Why is the change risky/not risky?]:
The patch mostly refactors existing code

[String changes made/needed]:
None

Comment 15

[Pascal Chevrel:pascalc]

I'd like QE to verify the fix in Nightly before uplifting to Beta.

Comment 16

[Pascal Chevrel:pascalc]

Brindusat, could you check the fix in Nightly with the instructions in comment #13 before I uplift to beta? Thanks

Comment 17

[Cristian Craciun]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:64.0) Gecko/20100101 Firefox/64.0
Build ID: 20180913100107

I've verified this bug on the latest Nightly build (64.0a1) based on the STR from Comment 13 and I can confirm that the fix was implemented.

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9008248 [details] [diff] [review]
Beta/63 patch

Thank you, , uplift approved for 63 beta 6. I'd like it to be also verified on beta when it is landed , thanks

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c365b9595514

Comment 20

[Cristian Craciun]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180913141435

Verified as fixed on the latest Beta build. (63.0b6)