1189101 - Cycling the default engine via the keyboard in the in-content search UI includes only one-click engines

Status: RESOLVED FIXED

(Firefox :: Search, defect, P3)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Currently you can only cycle through engines that are visible as one-off buttons. As specified in https://bugzilla.mozilla.org/show_bug.cgi?id=1110678#c2.

Comment 1

[Petruta Horea [:phorea], Desktop QA]

I've noticed that cycling through engines on about:newtab and about:home starts always with the first search engine from the list while on the search bar, it continues with the next search engine.

So if the 4th search engine is selected as default, Ctrl+Down Arrow will set the 5th one as default in search bar, while doing the same operation on those about: pages, the 1st search engine will be set as default.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: WIP

This fixes the bug, but I still need to check if there are any other consumers of ContentSearch.jsm's GetState API, and maybe update some comments. I'll request review after that.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

BTW, the patch also makes the behavior that Petruta described in comment 1 consistent with the main searchbar's.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8712971 [details] [diff] [review]
WIP

Review of attachment 8712971 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch (updated a test, rebased)

I finally revisited this and don't think it needs any further changes. Carrying the f+ forward as r+.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8728756 [details] [diff] [review]
Patch (updated a test, rebased)

My bad - this patch updates the browser_ContentSearch test (the WIP didn't do this).

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e32c805e570a

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

https://hg.mozilla.org/integration/fx-team/rev/87d2ff402f699eac02138627518240880da05866
Bug 1189101 - In-content search: include all engines when cycling with cmd+up/down. r=adw

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87d2ff402f69