Closed Bug 1088660 (fx34-searchui) Opened 10 years ago Closed 9 years ago

Improve the search bar UI to support one-off searches

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
13

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
37.1
Tracking Status
firefox34 + fixed
firefox35 + fixed
firefox36 + fixed

People

(Reporter: florian, Assigned: florian)

References

(Depends on 8 open bugs)

Details

Attachments

(9 files, 28 obsolete files)

164.03 KB, image/png
Details
122.13 KB, image/png
Details
160.16 KB, image/png
Details
7.98 KB, application/x-xpinstall
Details
106.84 KB, patch
florian
: review+
Details | Diff | Splinter Review
106.87 KB, patch
florian
: review+
Details | Diff | Splinter Review
45.63 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
8.12 KB, patch
florian
: review+
Details | Diff | Splinter Review
2.38 KB, patch
florian
: review+
Details | Diff | Splinter Review
The current search bar encourages me to google "wikipedia keyword" instead of searching "keyword" directly with the wikipedia search engine, because if I select the wikipedia engine, I'll still have it selected for the next search.

I'm going to work here on the implementation of a prototype to improve one-off searches.
Attached patch Patch (Mac only) (obsolete) — Splinter Review
First version of the prototype, implemented as a patch. Works only on Mac (because I made the CSS changes only in browser/themes/osx).

I'll now work on converting this to an add-on, for easier testing.
Attached image Screenshot of attachment 8511036 (obsolete) —
Attached patch Patch (Mac only) v2 (obsolete) — Splinter Review
- This updated patch takes into account the feedback I got in #ux.
- This no longer modifies existing bindings, so simplify the conversation to an add-on.
Attachment #8511036 - Attachment is obsolete: true
Attached image Screenshot on Mac
Attachment #8511042 - Attachment is obsolete: true
Attached patch Patch (Mac only) v3 (obsolete) — Splinter Review
Fixed icon sizing on retina screens.
Attachment #8511107 - Attachment is obsolete: true
Attached file Add-on, Mac only (obsolete) —
This is the same thing, as a restartless add-on.

I haven't looked at the Windows/Linux CSS yet, so try this only on Mac for now.

I don't know if there are compatibility issues with older Firefox versions, so I've marked it as compatible only with current nightlies.

Known bug: the first time the panel is shown after the add-on is installed or the browser is started, no suggestions are shown in the panel. Closing the panel and reopening it is enough for the bug to disappear. I have no idea of what's causing this, as I can't reproduce it when applying the patch-version to my local build. Not sure how important this is to fix.
I love this.  Only caveat is that the one-shot searches should be keyboard-accessible for a11y/keyboard only use.
Oh, and I'd be interested to see how the 65x26 icons work in this context, they'd definitely be more visible/identifiable.  That or 32x32 might be better.
Comment on attachment 8511158 [details] [diff] [review]
Patch (Mac only) v3

This patch already has some bitrot, due to http://hg.mozilla.org/mozilla-central/rev/a890f2283b74#l2.49
Attached patch CSS for Windows (obsolete) — Splinter Review
Attached image Screenshot on Windows (obsolete) —
(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> Created attachment 8512018 [details]
> Screenshot on Windows

Very Nice!
Can we remove the rounded corners on Windows?
Removing rounded corners? Turn in your UX badge!

Any chance we can get a working add-on with the Windows styling by tomorrow AM PDT?
Attached patch Linux CSS (obsolete) — Splinter Review
I fixed the CSS on Linux. Unfortunately on Linux there's a bug I'll need to debug tomorrow: the click handler used for the one off buttons doesn't receive events for left clicks (middle click works).
Attached image Screenshot on Linux
Comment on attachment 8512285 [details] [diff] [review]
Linux CSS

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

The fix for the linux bug mentioned in comment 14 is to add this before the click handler:
      <handler event="mousedown"><![CDATA[
        // Required to receive click events from the buttons on Linux.
        event.preventDefault();
      ]]></handler>

::: browser/themes/linux/searchbar.css
@@ +109,5 @@
> +  font-weight: normal;
> +  background-color: rgb(245, 245, 245);
> +  border-top: 1px solid #ccc;
> +  margin: 0 1px;
> +  padding: 3px 6px;

This line should be changed to "padding: 3px 5px;" for the icon of the current search engine to be actually aligned with the magnifying glass icon.
(In reply to Philipp Sackl [:phlsa] from comment #12)
> (In reply to Florian Quèze [:florian] [:flo] from comment #11)
> > Created attachment 8512018 [details]
> > Screenshot on Windows
> 
> Very Nice!
> Can we remove the rounded corners on Windows?

I think you are right that it looks better on Windows without rounded corners. Especially as without tweaking the corners the panel gets automatically a shadow that match the shadow of the awesomebar's panel.
Attached patch Patch v4 (obsolete) — Splinter Review
Patch working on all 3 platforms, including all the changes I mentioned above.
Attachment #8511158 - Attachment is obsolete: true
Attachment #8512017 - Attachment is obsolete: true
Attachment #8512018 - Attachment is obsolete: true
Attachment #8512285 - Attachment is obsolete: true
Attachment #8511110 - Attachment description: Screenshot of attachment 8511107 → Screenshot on Mac
I tested this on current nightlies on Windows 7/Mac 10.9/Ubuntu 13.10.
Attachment #8511165 - Attachment is obsolete: true
Attached patch Flare v2 (obsolete) — Splinter Review
This patch applies on top of attachment 8512583 [details] [diff] [review]. It addresses most of the requests I've received so far.

Here is what I currently have left in my todo list:
- implement the keyboard navigation behavior spec'ed by Philipp
- show a menu when clicking the magnifying glass icon of the search box.
- port all the theme updates to the Windows and Linux themes.
- [if we still care about having these changes as an add-on] update the add-on (the in-content version of the pref pane may be challenging to port to the add-on, as in-content preferences use preprocessing in several places).
- Check how much of this applies to mozilla-beta / what needs fixing/porting.

If you've asked me for anything that's neither in that list nor in the current version of the prototype, please remind me :-).

I just pushed to try (Mac only for now) so that you can test the current version: https://tbpl.mozilla.org/?tree=Try&rev=d10dfb5c8b1
(In reply to Florian Quèze [:florian] [:flo] from comment #20)

> I just pushed to try (Mac only for now) so that you can test the current
> version: https://tbpl.mozilla.org/?tree=Try&rev=d10dfb5c8b1

Oops, packaging mistake. Pushed again: https://tbpl.mozilla.org/?tree=Try&rev=db75de75c146
Attached patch Keyboard navigation (obsolete) — Splinter Review
This new patch applies on top of attachment 8512583 [details] [diff] [review] and attachment 8517561 [details] [diff] [review]; it implements keyboard navigation, mostly following Philipp's specification, and doing what I think makes sense for under-specified areas.

New try server build at https://tbpl.mozilla.org/?tree=Try&rev=2079fcf588dc
Flags: firefox-backlog+
Points: --- → 13
Flags: qe-verify-
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Attached patch Windows/Linux css changes (obsolete) — Splinter Review
CSS changes needed on Windows and Linux for attachment  8517561 [details] [diff] [review] and attachment 8518938 [details] [diff] [review].
Attached patch Flare v2.1 (obsolete) — Splinter Review
This combines the last 3 patches into one. Attachment 8512583 [details] [diff] still needs to be applied first.

Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=10305eb12208
Attachment #8517561 - Attachment is obsolete: true
Attachment #8518938 - Attachment is obsolete: true
Attachment #8521482 - Attachment is obsolete: true
Attached patch Patch for mozilla-beta (obsolete) — Splinter Review
Full patch, adapted for beta (there was only some minor bitrot). I tested this only on Mac, and surprisingly it worked at the first try! :)

I pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=6b6ae31e3049

Felipe, the patch is not ready for review yet, and tomorrow I expect to add a pref to turn on/off this feature, and to clean it up (eg. avoid reusing old css classes for different purposes, separate content/ and skin/ css, ...), but I don't expect major changes, so now may be a good time to start looking at the code so that we can discuss the parts that aren't clear (if any).
Attachment #8521789 - Flags: feedback?(felipc)
Attached patch Patch to pref off on non-en-US (obsolete) — Splinter Review
This applies above attachment 8521789 [details] [diff] [review].
Comment on attachment 8521789 [details] [diff] [review]
Patch for mozilla-beta

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

Only general comment is that it would be nice to merge a lot of what's going on in the platform-specific searchbar.css into the shared theme. But I know this is time dependent and things are split up because of the add-on work, so we can do that afterwards.

::: browser/base/content/browser.xul
@@ +139,5 @@
>  
>      <!-- for search and content formfill/pw manager -->
>      <panel type="autocomplete" id="PopupAutoComplete" noautofocus="true" hidden="true"/>
>  
> +    <panel type="autocomplete" id="PopupSearchAutoComplete" noautofocus="true" hidden="true"/>

please add a comment above here as all other panels have one

::: browser/base/content/urlbarBindings.xml
@@ +911,5 @@
> +      </xul:tree>
> +      <xul:hbox anonid="search-panel-one-offs-header"
> +                class="search-panel-header search-panel-current-input">
> +        <xul:label anonid="searchbar-input-before-label" value="Search for "/>
> +        <xul:label anonid="searchbar-input-value" flex="1" crop="end" class="search-panel-input-value"/>

input-value subconsciously triggers to me as the real source of the input element, not a copy of its value.

So maybe searchbar-ooheader-before, searchbar-ooheader-searchtext, searchbar-ooheader-after

I guess -label isn't necessary because we know which type of element it is.
And rename the inputElm var :)

@@ +926,5 @@
> +        let currentEngine = Services.search.currentEngine;
> +        let uri = currentEngine.iconURI;
> +        if (uri)
> +          uri = uri.spec;
> +        let PlacesUtils =

I /think/ PlacesUtils is already defined in the browser window scope by something else

@@ +952,5 @@
> +
> +        let list = document.getAnonymousElementByAttribute(this, "anonid",
> +                                                           "search-panel-one-offs")
> +        while (list.firstChild)
> +          list.firstChild.remove();

It would be nice to avoid reconstructing this on every onpopupshowing unless conditions change

::: toolkit/components/search/SearchSuggestionController.jsm
@@ +353,5 @@
>      }
>  
>      // Trim the number of results to the maximum requested (now that we've pruned dupes).
> +    results.remote =
> +      results.remote.slice(0, this.maxRemoteResults - results.local.length);

so essentially maxRemoteResults means now maxResults.. But that's not gonna be enforced unless there are remote results? (comment above says "This limit is only enforced if remote results are also returned"). So in theory if remote results are off, 11 local results can show up and still show a scrollbar, right?
Attachment #8521789 - Flags: feedback?(felipc) → review+
Attached patch Address feedback (obsolete) — Splinter Review
Attachment #8522428 - Flags: review?(felipc)
Attachment #8522428 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes from comment #28)

> Only general comment is that it would be nice to merge a lot of what's going
> on in the platform-specific searchbar.css into the shared theme. But I know
> this is time dependent and things are split up because of the add-on work,
> so we can do that afterwards.

I agree, let's cleanup the CSS in a follow-up.

> @@ +952,5 @@
> > +
> > +        let list = document.getAnonymousElementByAttribute(this, "anonid",
> > +                                                           "search-panel-one-offs")
> > +        while (list.firstChild)
> > +          list.firstChild.remove();
> 
> It would be nice to avoid reconstructing this on every onpopupshowing unless
> conditions change

Yes, but the conditions are non-trivial: any change to the list of hidden one off engines, any change to the default engine, any change to the searchbox size (width) would require a rebuild. Let's look at this later.

> ::: toolkit/components/search/SearchSuggestionController.jsm
> @@ +353,5 @@
> >      }
> >  
> >      // Trim the number of results to the maximum requested (now that we've pruned dupes).
> > +    results.remote =
> > +      results.remote.slice(0, this.maxRemoteResults - results.local.length);
> 
> so essentially maxRemoteResults means now maxResults.. But that's not gonna
> be enforced unless there are remote results? (comment above says "This limit
> is only enforced if remote results are also returned"). So in theory if
> remote results are off, 11 local results can show up and still show a
> scrollbar, right?

Yes it's a known edge case. I also know and edge case that could cause us to show slightly less than 10 results when 10+ were available. I'd like to clean this up in a dedicated bug.

Thanks for the review! :)
Attached patch Make test_searchSuggest.js pass (obsolete) — Splinter Review
Attachment #8522487 - Flags: review?(felipc)
Attachment #8522487 - Flags: review?(felipc) → review+
Attached patch Make the tests pass (obsolete) — Splinter Review
Attachment #8522487 - Attachment is obsolete: true
Attachment #8523071 - Flags: review?(felipc)
Attachment #8523071 - Flags: review?(felipc) → review+
Attached patch Plug leak (obsolete) — Splinter Review
This patch fixes the "leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xul]" errors I had on debug bc1 on my latest try push.

It also fixes the "JavaScript Error: 'this.updateDisplay is not a function'" JS error I was seeing locally when the searchbox is moved to the hamburger panel.

Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=80c8af553477
Attachment #8523830 - Flags: review?(felipc)
Attached patch Plug leak (obsolete) — Splinter Review
Ooops, copy/paste mistake in the patch. I canceled the try server builds and pushed again: https://tbpl.mozilla.org/?tree=Try&rev=af1d2c734b54
Attachment #8523830 - Attachment is obsolete: true
Attachment #8523830 - Flags: review?(felipc)
Attachment #8523832 - Flags: review?(felipc)
Attachment #8523832 - Flags: review?(felipc) → review+
Attached patch Fix bc1 on XP opt (obsolete) — Splinter Review
Fix the intermittent (but frequent) orange on XP bc1 opt: https://tbpl.mozilla.org/?tree=Try&rev=e8f82ae25121
Attachment #8523950 - Flags: review?(felipc)
Attachment #8523950 - Flags: review?(felipc) → review+
Tweak the wordings on the Search preference per UX request.
Attachment #8524120 - Flags: review?(felipc)
Attachment #8524120 - Flags: review?(felipc) → review+
Attached patch Additional fixes (obsolete) — Splinter Review
Fix a few bugs that have been identified while testing the previous try builds:
- using "paste" in the context menu of the searchbox didn't make the go button appear (switching the event handler from "keyup" to "input" fixes this)
- while testing this patch, I've noticed that dropping some text to the searchbox correctly searches for the text immediately, but doesn't make the go button appear; also fixed by this patch.
- The command+up/down (and command+mouse wheel) keyboard shortcuts of the old UI could cause users to change their default search engine with the new UI without noticing. The recommendation I got when I mentioned this to UX was that it's a perfect opportunity to get rid of these shortcuts.
Attachment #8524166 - Flags: review?(felipc)
Attachment #8524166 - Flags: review?(felipc) → review+
This also includes attachment 8524166 [details] [diff] [review], but I'll let Felipe review it as a separate patch before I mark it obsolete and carry forward the review here (or update this patch to address his comments).

Try: https://tbpl.mozilla.org/?tree=Try&rev=ed47421037eb
Attachment #8521789 - Attachment is obsolete: true
Attachment #8522236 - Attachment is obsolete: true
Attachment #8522428 - Attachment is obsolete: true
Attachment #8523071 - Attachment is obsolete: true
Attachment #8523832 - Attachment is obsolete: true
Attachment #8523950 - Attachment is obsolete: true
Attachment #8524120 - Attachment is obsolete: true
Attachment #8524166 - Attachment is obsolete: true
Comment on attachment 8524175 [details] [diff] [review]
Combined beta patch

All the patches that were combined into this one have r=felipe.
Attachment #8524175 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=283a70bc3be
Attachment #8512583 - Attachment is obsolete: true
Attachment #8521598 - Attachment is obsolete: true
Attachment #8524206 - Flags: review+
Forgot to hg add the icons for the previous patch...
Attachment #8525642 - Attachment is obsolete: true
Now tested on Windows/Linux too.
Attachment #8525644 - Attachment is obsolete: true
Blocks: 1101122
Blocks: 1101999
Blocks: 1101654
Depends on: 1102024
Depends on: 1102032
Depends on: 1102038
Depends on: 1102039
Depends on: 1102045
Depends on: 1102050
Depends on: 1102051
Depends on: 1102055
Depends on: 1102056
Depends on: 1102057
Depends on: 1102058
I think this is the last large code-patch. I still need to improve a few details in the CSS for Windows/Linux.
Attachment #8525657 - Attachment is obsolete: true
Attachment #8526160 - Flags: review?(felipc)
Depends on: 1102398
Attachment #8526160 - Flags: review?(felipc) → review+
Attached patch Polish CSS for Windows/Linux (obsolete) — Splinter Review
My linux build is still not finished so I haven't been able to test the opensearch stuff there, but I tested the other parts of this patch on an old build I had around and I'm confident :).
Attachment #8526322 - Flags: review?(felipc)
Attached patch Polish CSS for Windows/Linux (obsolete) — Splinter Review
Actually tested this time. The only difference is I needed to add the menuitem-with-favicon class to my menuitems for the icons to be displayed on Linux in the search pane.
Attachment #8526322 - Attachment is obsolete: true
Attachment #8526322 - Flags: review?(felipc)
Attachment #8526329 - Flags: review?(felipc)
Attachment #8526329 - Flags: review?(felipc) → review+
Why does this land on beta only?
(In reply to Mike Hommey [:glandium] from comment #49)
> Why does this land on beta only?

I expect it to land on aurora and central (without hardcoded strings) next week. Given how little time we have until beta becomes release, we focused on beta for now.
Is it the expected UX that keyboard navigation only uses up and down keys or should I file a bug? The UX also feels awkward when search suggestions are disabled.

(In reply to Wes Kocher (:KWierso) from comment #51)
> Backed out of beta in
> https://hg.mozilla.org/releases/mozilla-beta/rev/9ae0161389d3 for bc1 errors:

(which, btw, were appearing on try)
(In reply to Mike Hommey [:glandium] from comment #52)
> Is it the expected UX that keyboard navigation only uses up and down keys or
> should I file a bug?

It's the expected UX that the text field stays focused at all time and the left/right arrows are reserved for navigation within what has been typed in the text field.

Keyboard navigation uses both up/down arrows and the tab key.

We are not satisfied with it yet and it will be improved next week; however feel free to file bugs if there are specific behaviors you think should be improved. Better file a dupe than miss something important :).

> The UX also feels awkward when search suggestions are
> disabled.

Why? Please tell us more in a separate bug (cc :phlsa for UX and me for the code).
 
> (In reply to Wes Kocher (:KWierso) from comment #51)
> > Backed out of beta in
> > https://hg.mozilla.org/releases/mozilla-beta/rev/9ae0161389d3 for bc1 errors:
> 
> (which, btw, were appearing on try)

Which try are you referring to? This was a trivial CSS change, I didn't push it to try.
Flags: needinfo?(florian)
Same patch without the -moz-border-colors: none; lines; I verified that things work without them, I must have been confused yesterday evening.
Attachment #8526609 - Flags: review+
Attachment #8526329 - Attachment is obsolete: true
Depends on: 1102821
Can someone please explain why this patch landed on the beta branch without any approval from drivers? Also without any baking on Nightly, and Aurora first, nearly at the end of the 34.0 beta cycle?
Depends on: 1102903
Depends on: 1102912
[Tracking Requested - why for this release]:

whimboo: I think there was no "baking" because this is related to (or needed for) for the new search UI that Kamil is working on testing. Most of us only found out about his yesterday when the announcement went public about the deal with Yahoo. But yeah it probably still should have been nominated for tracking so I'm doing that now.
Depends on: 1102918
[Tracking Requested - why for this release]:
Depends on: 1102933
Depends on: 1102942
I'm changing the bugs found overnight by the  Softvision team  to block fx34 (bug 1101642) instead of 1088660.  They couldn't see 1101642 until a couple of hours ago.  Sorry for the bug spam.
No longer depends on: 1102961
No longer depends on: 1102948
No longer depends on: 1102821
No longer depends on: 1102903
No longer depends on: 1102909
No longer depends on: 1102911
No longer depends on: 1102912
No longer depends on: 1102919
No longer depends on: 1102942
Depends on: 1102919
Gavin asked me to switch these back to 1088660 for now. So there will be more bugmail.
Depends on: 1102912
Depends on: 1102911
Depends on: 1102909
Depends on: 1102903
Depends on: 1102821
Depends on: 1102948
Depends on: 1102961
Depends on: 1103119
Depends on: 1103326
OS: Mac OS X → All
Hardware: x86 → All
I found the following issues with the implementation that just landed in the beta channel. As I'm not used to Bugzilla I'm posting them here. Please feel free to file bug reports if you agree with the issues.

Missing features:

- Can't remove search engines

- Can't change the order of the alternative search engines

- Can't add keywords

- Can't modify keywords

Breaks the UX:

- Can't open search results of an alternative search engine in a new tab (both CTRL click and mousewheel click will open it in the same tab)

- TAB selects the next search engine instead of the next search result (unexpected behavior, e.g. the address bar and Google both select the next result)

- Also if you TAB through the alternative search engines, the default search engine has no highlight

- Some search engines have the same icon (e.g. multiple versions of Wikipedia) - the name of the search engine should be displayed (mouse over is not enough)

- Can't see which search engine is currently the default without opening the dropdown or hovering over the search field

Minor UI related issues:

- Hovering with the mouse over the search field displays the default search engine; hovering over the icons (magnifying glass and arrow) just displays "Search"

- The text size is rather small, I don't think such small text is used anywhere else

- "Add $searchengine" has a weird border

- The magnifying glass has a different color than every other icon in the address and search bar (Windows 8)

- Hovering over the magnifying glass icon just changes it to a darker gray, every other icon in the address and search bar has some kind of glow

General:

- If you focus the empty search field and press DOWN the search history appears, but above the alternative search engines the text says "Search for  with:"

Overall I would argue this feature doesn't belong into the beta channel yet, and certainly not the release channel.
I found some more, sorry for the second post.

- Unnecessary dividers are shown if the user has not enough alternative search engines configured to fill a row

- Can't navigate the alternative search engines with LEFT and RIGHT, only TAB, TOP and DOWN. This is rather unexpected, because the are lined up horizontally.

- Can't use TAB to auto complete the search with the first search result anymore
Flags: needinfo?(w8fsq+b7cozw6gfo8p8)
(In reply to Greenix from comment #61)
> I found the following issues with the implementation that just landed in the
> beta channel. 

https://bugzilla.mozilla.org/show_bug.cgi?id=891258#c65

"Unless there's a special reason for doing so, the expectation is that patches will ride the usual trains. That gives things times to bake and be tested before hitting non-Nightly audiences."

This isn't just landing on beta, it's landing real close to the end of beta ...
The icon for the search engine should not be changed to the magnifying glass search icon until the bar underneath the search bar shows what search engine should be used. This is because the user needs to immediately see what search engine they will be using as they direct their eyes to the search bar.

Right now, the default search engine isn't even shown at all unless there is text in the search bar, even when clicking the icon.
The functionality of having the drop down list of search engines needs to be returned. Unlabeled icons are difficult to use, and also a pain to use in the grid format for a person with many search engines.

I like your idea of using the tab button to switch search engines. But some people use the tab button to scroll through the search suggestions. I don't know how many people do this, but this way, it works the same as the address bar with both tab and arrow buttons to scroll down suggestions.I don't see why you had to use special icons for this though. So complicated.
Your idea of searching immediately on click when you click the search engine is a terrible idea because in the past, that was not the functionality when clicking on the drop down. And the user may not want to search with the keyword they are using on that search engine.

Also, if you didn't know, by changing the default search engine, you also change the right click "Search 'example engine' for 'keyword' ". The way it is now, you are forced to go into "Change Search Settings" just so you can switch this.



I could probably say more, but I feel burned out after analyzing this so much. I think what you should do is to ONLY have the new search icons bar that you made, show up when the user starts typing in or clicks on the whitespace search bar (with or without pre-typed text). Whatever you do, make sure that clicking on the icon works the same way as it always has, the default search engine icon will always be shown at least once to the user at all times, and the normal functionality when clicking on the icon in order to change the default search engine within two steps is still available.
Another issue:

There is no visual difference between past searches and search suggestions anymore.
(In reply to Greenix from comment #65)
> Another issue:
> 
> There is no visual difference between past searches and search suggestions
> anymore.

That's scheduled to be fixed in a later release:

Add icons to history-based suggestions in search bar dropdown
https://bugzilla.mozilla.org/show_bug.cgi?id=1101996
This change breaks the compatibility of Second Search (https://addons.mozilla.org/en-US/firefox/addon/second-search/).

Why such a large change has landed really close to final release? This should be backed out immediately.
Depends on: 1103931
Depends on: 1104142
Depends on: 1104221
> - Can't remove search engines

Bug # 1102513

> - Can't change the order of the alternative search engines

Bug # 1102909

> - TAB selects the next search engine instead of the next search result
> (unexpected behavior, e.g. the address bar and Google both select the next
> result)

Bug # 1102948

> - Can't see which search engine is currently the default without opening the
> dropdown or hovering over the search field

Bug # 1102922

> - The text size is rather small, I don't think such small text is used
> anywhere else

Bug # 1102919

> This change breaks the compatibility of Second Search
> (https://addons.mozilla.org/en-US/firefox/addon/second-search/).

Bug # 1103326
Depends on: 1103455
Depends on: 1104315
Depends on: 1104325
QA Contact: petruta.rasa
Iteration: 36.3 → 37.1
Depends on: 1104738
Depends on: 1104748
Depends on: 1104846
Depends on: 1104971
Depends on: 1105259
Depends on: 1105264
Depends on: 1105272
No longer depends on: 1105264
Depends on: 1105274
Depends on: 1105287
Attachment #8524175 - Flags: approval-mozilla-beta?
Attachment #8524175 - Flags: approval-mozilla-aurora?
Attachment #8526160 - Flags: approval-mozilla-beta?
Attachment #8526160 - Flags: approval-mozilla-aurora?
Attachment #8526609 - Flags: approval-mozilla-beta?
Attachment #8526609 - Flags: approval-mozilla-aurora?
(In reply to Greenix from comment #61)

> - Can't add keywords
> 
> - Can't modify keywords

No depending bug for those ? Looks pretty annoying for "advanced users" that heavily use this...
Depends on: 1105329
No longer depends on: 1105272
I pushed the set of flare patches on try above fx-team yesterday and had some intermittent bc1 test failures: https://tbpl.mozilla.org/?tree=Try&rev=a18c5dc570fc

With the attached patch, try is green: https://tbpl.mozilla.org/?tree=Try&rev=d3e93e01d551

Felipe r+'ed over email.


I think the SearchSuggestionController.jsm change is needed because "A promise chain failed to handle a rejection" causes
mochitests to fail on 36 but not 34.

Searching a bit more shows my guess is correct "Bug 1016387 - Uncaught
async Promise errors should cause mochi tests to fail" landed on 35.


The other test fix I made looks a lot like it's the same thing
as bug 1104654 so it would be needed on beta as well. I'm just not sure we should land test fixes on beta at this point, given that it's already been
merged to release.
Attachment #8529717 - Flags: review+
Depends on: 1105750
Depends on: 1105746
Can anyone point me to a discussion where it was decided to land this on beta directly, and skip central altogether? The amount of follow-up issues shows very clearly, that this change is *not* ready for the public yet, and that a longer testing and baking phase on Nightly would have been needed.

(In reply to Florian Quèze [:florian] [:flo] from comment #50)
> I expect it to land on aurora and central (without hardcoded strings) next
> week. Given how little time we have until beta becomes release, we focused
> on beta for now.

Givent that little time, it’s even more ridiculous that this skipped Nightly completely and landed on beta directly. I always thought that the release system (central -> aurora -> beta -> final) was there to make sure that only polished things will be pushed out to the general public. Skipping two major steps now, and also landing it this short before the final release on beta seems really careless.
Comment on attachment 8529717 [details] [diff] [review]
Fix tests on fx-team

After reviewing with flo-retina, this patch is required to avoid test bustage when the rest of this work lands on 35. Aurora+
Attachment #8529717 - Flags: approval-mozilla-aurora+
Depends on: 1105794
Comment on attachment 8524175 [details] [diff] [review]
Combined beta patch

I had previously given flo-retina verbal approval for all branches on this bug. Cleaning up the approvals now.
Attachment #8524175 - Flags: approval-mozilla-beta?
Attachment #8524175 - Flags: approval-mozilla-beta+
Attachment #8524175 - Flags: approval-mozilla-aurora?
Attachment #8524175 - Flags: approval-mozilla-aurora+
Attachment #8526160 - Flags: approval-mozilla-beta?
Attachment #8526160 - Flags: approval-mozilla-beta+
Attachment #8526160 - Flags: approval-mozilla-aurora?
Attachment #8526160 - Flags: approval-mozilla-aurora+
Attachment #8526609 - Flags: approval-mozilla-beta?
Attachment #8526609 - Flags: approval-mozilla-beta+
Attachment #8526609 - Flags: approval-mozilla-aurora?
Attachment #8526609 - Flags: approval-mozilla-aurora+
Where are my keyboard shortcuts?
What if my icons are all same and I can't change that?
Blocks: 1106205
Depends on: 1106240
Depends on: 1106241
This also breaks being able to switch search engines for the context menu search (select text, 'Search <provider" for "<text>"'). In previous Fx versions, changing this is reasonably easy using the search provider dropdown, but it's no longer possible without heading into the preferences.

While this change seems to be great for improving the one-off search flow, IMO it completely breaks it for uses outside of that. If you have multiple search engines that you use regularly, say MDN and Google, it's now practically impossible to use them.
Depends on: 1106333
Depends on: 1106331
Depends on: 1106559
No longer depends on: 1106333
Depends on: 1106569
Flags: needinfo?(w8fsq+b7cozw6gfo8p8)
Depends on: 1106476
Depends on: 1106873
Depends on: 1106876
Depends on: 1106924
Depends on: 1106926
Depends on: 1107135
Depends on: 1107178
Depends on: 1107266
Depends on: 1107278
Depends on: 1107449
Depends on: 1107493
Depends on: 1107471
No longer depends on: 1107471
No longer depends on: 1107494
Depends on: 1107506
Depends on: 1107695
Is there a bug covering a visual pass to this feature ? As much as I like it I don't think it's really nice visually (one example : the dropdown arrow appearing on hover is really too close to the search lens) The work done in bug 1047354 could also be taken into account concerning the search field (mockup : http://people.mozilla.org/~mmaslaney/Search/Autocomplete-dropdown.png ).
(In reply to Guillaume C. [:ge3k0s] from comment #79)
> Is there a bug covering a visual pass to this feature ?

No, please file bugs blocking this one for things that you think should be polished.

> one example : the dropdown arrow
> appearing on hover is really too close to the search lens

This specific icon was from :shorlander, please CC him if you file a bug.
(In reply to Florian Quèze [:florian] [:flo] from comment #80)
> (In reply to Guillaume C. [:ge3k0s] from comment #79)
> > Is there a bug covering a visual pass to this feature ?
> 
> No, please file bugs blocking this one for things that you think should be
> polished.

I was mainly asking and maybe UX folks have already discussed that, so I'm needinfo'ing my comment for answer from the concerned people.

> > one example : the dropdown arrow
> > appearing on hover is really too close to the search lens
> 
> This specific icon was from :shorlander, please CC him if you file a bug.

I wasn't aware of that. Waiting on the needinfo too.
Flags: needinfo?(philipp)
Flags: needinfo?(mmaslaney)
Depends on: 1108488
Depends on: 1108646
Depends on: 1108648
Depends on: 1108273
Depends on: 1108490
Two more bugs to add as dependencies: bug 1107137 and bug 1107292 (one is probably a duplicate).
Depends on: 1107137, 1107292
Depends on: 1108594
Depends on: 1109249
Depends on: 1108230
No longer depends on: 1102934
Alias: fx34-searchui
No longer depends on: 1102918
Depends on: 1109636
Just tried on 34.0.5 on a fresh profile. Then tried to switch back, which doesn't restore the behavior even in new tabs of the same session. Will raise a separate bug on it.

(In reply to Ryan McCue from comment #78)
> This also breaks being able to switch search engines for the context menu
> search (select text, 'Search <provider" for "<text>"'). In previous Fx
> versions, changing this is reasonably easy using the search provider
> dropdown, but it's no longer possible without heading into the preferences.
> 
> While this change seems to be great for improving the one-off search flow,
> IMO it completely breaks it for uses outside of that. If you have multiple
> search engines that you use regularly, say MDN and Google, it's now
> practically impossible to use them.
I also have 2 problems

1) duplication of icons (I use google and google verbatim, google images and google images HD, DuckDuckGo and DuckDuckGo (no filter))
2) wide search box - I put the search box on the main menu so it stretches very far to the right - icons can be very far right as a result making the oneoff click even more difficult 

(2) is probably a classic edge case, so not this important
(1) is a bit of a usability problem, can we allow showing / modifying labels?

Also personally I find a plain dropdown selection easier and faster than reaching for an icon, YMMV.
(In reply to Guillaume C. [:ge3k0s] from comment #81)
> (In reply to Florian Quèze [:florian] [:flo] from comment #80)
> > (In reply to Guillaume C. [:ge3k0s] from comment #79)
> > > Is there a bug covering a visual pass to this feature ?
> > 
> > No, please file bugs blocking this one for things that you think should be
> > polished.
> 
> I was mainly asking and maybe UX folks have already discussed that, so I'm
> needinfo'ing my comment for answer from the concerned people.

Shorlander did a visual design pass on this. The implementation doesn't match it 100%, but it's very close. If you find issues, the best way is to file separate bugs that reference this one :)
Flags: needinfo?(philipp)
Flags: needinfo?(mmaslaney)
Depends on: 1109839
Depends on: 1107544
Depends on: 1109851
Depends on: 1108254
Depends on: 1110820
Not entirely Mozilla's problem, but I'm concerned that this strange new behaviour got pushed out to Ubuntu LTS as part of the trusty security repository just over a month after it's development began, apparently fast tracked by skipping due process. At least it appears that it's been done transparently and it's had some eyes on it.

What worries me the most is when I go in and uncheck all the available options, the new ugly buggy behaviour is still there (in version 34). Couldn't this have just been an extension for people that want this feature? I like nice clean simple default or at least provide us with an option to disable such bloat.
After looking at your screenshots I realised that my Quick Search looks nothing like it was intended to so I've just submitted bug 1111868 for that. Along with the other bugs I've found this new quick search has been very disruptive, even after going in to about:config and toggling browser.search.showOneOffButtons I find that the most irritating bugs are still in effect (bug 1012370 and bug 1107292). So I guess what I'd actually like to see here is an option in the settings menu to revert to a well tested, mature Quick Search bar.

I can see what you've attempted could be useful to people, I just think time would have been better spent testing and debugging instead of putting some fancy first-use tour in to advertise it. So far it's been far more disruptive to me that any possible usefulness I might get from it may have made up for.
Depends on: 1111868
Depends on: 1111943
Depends on: 1111947
No longer depends on: 1107544
Depends on: 1113550
Depends on: 1113556
Depends on: 1107503
Depends on: 1113731
Depends on: 1113747
Depends on: 1106599
Depends on: 1107509
Depends on: 1107580
Depends on: 1103321
No longer depends on: 1108080
Depends on: 1111305
Depends on: 1119233
Depends on: 1119250
Depends on: 1119273
Depends on: 1119895
Depends on: 1119987
Depends on: 1121535
Depends on: 1123001
Depends on: 1123442
Depends on: 1129587
Depends on: 1129597
No longer depends on: 1129587
No longer depends on: 1129597
Depends on: 1130480
Depends on: 1131685
Depends on: 1138176
Depends on: 1143638
Depends on: 1160860
Depends on: 1197480
Depends on: 1204241
Depends on: 1204242
Depends on: 1137332
Depends on: 1234134
Depends on: 1167556
Depends on: 361923
Depends on: 1230961
Depends on: 1251977
You need to log in before you can comment on or make changes to this bug.