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…
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
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.
Comment on attachment 9002652 [details] Bug 1484737 - Improve the handling of search alias highlighting in the urlbar. Ed Lee :Mardak has approved the revision.
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.
Ah, sorry. Today is merge day, so I'll wait to land this until tomorrow or later, and then maybe request uplift.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/6b63ab504b03 Improve the handling of search alias highlighting in the urlbar. r=Mardak,mak
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
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
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 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+
status-firefox63: fix-optional → fixed
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
You need to log in before you can comment on or make changes to this bug.