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

VERIFIED FIXED in Firefox 63

Status

()

P3
normal
VERIFIED FIXED
4 months ago
3 months ago

People

(Reporter: Mardak, Assigned: adw)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 wontfix, firefox63 verified, firefox64 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
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…
(Reporter)

Comment 1

4 months ago
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.
status-firefox62: --- → wontfix
status-firefox63: --- → wontfix
Priority: -- → P3
We should probably check for the highlight on selection changes.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Created attachment 9002652 [details]
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.
(Reporter)

Comment 4

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

Ed Lee :Mardak has approved the revision.
Attachment #9002652 - Flags: review+
According to that try push, there's a failure in browser_urlOverflow.js related to the scheme.  I'll figure that out next week.
when adding new reviewers, please always use blocking reviews, otherwise the review request is invisible in phabricator
Comment on attachment 9002652 [details]
Bug 1484737 - Improve the handling of search alias highlighting in the urlbar.

Marco Bonardo [::mak] has approved the revision.
Attachment #9002652 - Flags: review+
Ah, sorry.

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

Comment 10

3 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b63ab504b03
Improve the handling of search alias highlighting in the urlbar. r=Mardak,mak

Comment 11

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b63ab504b03
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Setting firefox63 status to fix-optional.  Would be nice to uplift this.
status-firefox63: wontfix → fix-optional
Flags: qe-verify+
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
Created attachment 9008248 [details] [diff] [review]
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
Attachment #9008248 - Flags: approval-mozilla-beta?
I'd like QE to verify the fix in Nightly before uplifting to Beta.
Brindusat, could you check the fix in Nightly with the instructions in comment #13 before I uplift to beta? Thanks
Flags: needinfo?(brindusa.tot)

Comment 17

3 months ago
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.
Flags: needinfo?(brindusa.tot)

Updated

3 months ago
status-firefox64: fixed → verified
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
Attachment #9008248 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 19

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c365b9595514
status-firefox63: fix-optional → fixed

Comment 20

3 months ago
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)
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
Depends on: 1491724
You need to log in before you can comment on or make changes to this bug.