1126250 - Show one-off buttons when clicking the magnifying glass

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Mockup

We can also reveal the one-off buttons when the magnifying glass is clicked.

It makes sense to hide the history results though. Mainly because of privacy concerns, but also because they would push down the »Add XYZ« row on pages that provide open search considerably.

Comment 1

[Florian Quèze [:florian]]

Thanks for the mockup! :)

Did you really mean to add yet another "or search with:" string here? I was hoping the "Search with:" string we already added for the case where the user presses the down arrow on an empty searchbox was good for that too?

I wonder if this could also be an opportunity to improve the UI when suggestions are disabled (we do have a separate bug on file for this though - bug 1109636, so we don't have to dig into it now).

Comment 2

[Florian Quèze [:florian]]

Note: depending on how much we want to uplift this fix (which depends on whether we really need a new string), we may wontfix bug 1123311.

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yes, I've added the »or« to make it a little more discoverable that the default is also a search provider you can use. It's a small thing, but it helps.

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Actually, after looking into it one more time: forget comment #3 and let's use the string we already have (»Search with:«).

Comment 5

[Florian Quèze [:florian]]

Attachment: WIP Patch

This already works, and I took the opportunity to also fix the keyboard navigation in that case, as it's dramatically easier to do now that bug 1124904 / bug 1102038 have landed.

I'm not requesting review yet because I have only tested the CSS changes on Mac, and I would like to write some tests for the keyboard navigation changes.

Comment 6

[Marco Mucci [:MarcoM]]

Hi Florian, can you provide a point value.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking as either this bug or bug 1123311 should be fixed in 37.

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch with tests

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

Comment 9

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/50bf2ec4b075

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8566581 [details] [diff] [review]
Patch with tests

Approval Request Comment
[Feature/regressing bug #]: fixes/hides a regression caused by bug 1121550; but is generally an improvement for the new search UI (bug 1088660)
[User impact if declined]: Useless "Search with:" line displayed in the small search panel when clicking on the glass icon without anything typed in the searchbox. 
[Describe test coverage new/current, TreeHerder]: the patch contains tests.
[Risks and why]: IMO acceptable for aurora or the beginning of a beta cycle.
[String/UUID change made/needed]: none.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8566581 [details] [diff] [review]
Patch with tests

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

::: browser/components/search/test/browser_searchbar_openpopup.js
@@ +463,5 @@
>    promise = promiseEvent(searchPopup, "popuphidden");
>    EventUtils.synthesizeKey("VK_ESCAPE", {});
>    yield promise;
> +
> +  textbox.value = "";

Note to whoever will uplift the patch: this one-line change isn't needed on 37; it's only fixing an issue with the test landed in bug 1114707, which didn't reach 37.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8566581 [details] [diff] [review]
Patch with tests

Although this patch has not yet landed on m-c, this behaviour is currently broken in 37. I'm going to approve for uplift before the merge to Beta. I'm glad to see that there are tests included in the patch and would appreciate this bug being verified on 37 before Beta 1. Aurora+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/415cffd81d86

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/50bf2ec4b075

Comment 15

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

I checked that the one-off search engines are displayed too when there is no text in the search field using Firefox 37 beta 1 and Developer Edition 38.0a2 2015-02-25 under win 7 64-bit, Ubuntu 14.04 64-bit and Mac OSX 10.10.

Comment 16

[David Lawrence [:dkl]]

Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl