Closed Bug 1189101 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Search, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox42 --- affected
firefox48 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

(Whiteboard: [ui][fxsearch])

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(kev)
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.
Rank: 35
Priority: -- → P3
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #8712971 - Flags: feedback?(adw)
BTW, the patch also makes the behavior that Petruta described in comment 1 consistent with the main searchbar's.
Comment on attachment 8712971 [details] [diff] [review]
WIP

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

Looks good to me.
Attachment #8712971 - Flags: feedback?(adw) → feedback+
I finally revisited this and don't think it needs any further changes. Carrying the f+ forward as r+.
Attachment #8712971 - Attachment is obsolete: true
Attachment #8728756 - Flags: review+
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).
Attachment #8728756 - Attachment description: Patch (same as WIP, rebased) → Patch (updated a test, rebased)
Attachment #8728756 - Flags: review+ → review?(adw)
Attachment #8728756 - Flags: review?(adw) → review+
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
https://hg.mozilla.org/mozilla-central/rev/87d2ff402f69
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Flags: needinfo?(kev)
You need to log in before you can comment on or make changes to this bug.