Bug 1088660 (fx34-searchui)

Improve the search bar UI to support one-off searches

RESOLVED FIXED in Firefox 34

Status

()

Firefox
Search
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 19 bugs)

Trunk
Firefox 36
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox34+ fixed, firefox35+ fixed, firefox36+ fixed)

Details

Attachments

(9 attachments, 28 obsolete attachments)

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
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8511036 [details] [diff] [review]
Patch (Mac only)

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.
(Assignee)

Comment 3

4 years ago
Created attachment 8511107 [details] [diff] [review]
Patch (Mac only) v2

- 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
(Assignee)

Comment 4

4 years ago
Created attachment 8511110 [details]
Screenshot on Mac
Attachment #8511042 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
Created attachment 8511158 [details] [diff] [review]
Patch (Mac only) v3

Fixed icon sizing on retina screens.
Attachment #8511107 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 8511165 [details]
Add-on, Mac only

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.
(Assignee)

Comment 9

4 years ago
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
(Assignee)

Comment 10

4 years ago
Created attachment 8512017 [details] [diff] [review]
CSS for Windows
(Assignee)

Comment 11

4 years ago
Created attachment 8512018 [details]
Screenshot on Windows
(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?
(Assignee)

Comment 14

4 years ago
Created attachment 8512285 [details] [diff] [review]
Linux CSS

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).
(Assignee)

Comment 15

4 years ago
Created attachment 8512288 [details]
Screenshot on Linux
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
Created attachment 8512538 [details]
Screenshot on Windows without rounded corners

(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.
(Assignee)

Comment 18

4 years ago
Created attachment 8512583 [details] [diff] [review]
Patch v4

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
(Assignee)

Updated

4 years ago
Attachment #8511110 - Attachment description: Screenshot of attachment 8511107 → Screenshot on Mac
(Assignee)

Comment 19

4 years ago
Created attachment 8512627 [details]
Add-on, Windows/Mac/Linux

I tested this on current nightlies on Windows 7/Mac 10.9/Ubuntu 13.10.
Attachment #8511165 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Created attachment 8517561 [details] [diff] [review]
Flare v2

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
(Assignee)

Comment 21

4 years ago
(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
(Assignee)

Comment 22

4 years ago
Created attachment 8518938 [details] [diff] [review]
Keyboard navigation

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+

Updated

4 years ago
Points: --- → 13
Flags: qe-verify-

Updated

4 years ago
Status: NEW → ASSIGNED
Iteration: --- → 36.3
(Assignee)

Comment 23

4 years ago
Created attachment 8521482 [details] [diff] [review]
Windows/Linux css changes

CSS changes needed on Windows and Linux for attachment  8517561 [details] [diff] [review] and attachment 8518938 [details] [diff] [review].
(Assignee)

Comment 24

4 years ago
Created attachment 8521598 [details] [diff] [review]
Flare v2.1

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
(Assignee)

Comment 25

4 years ago
Created attachment 8521789 [details] [diff] [review]
Patch for mozilla-beta

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)
(Assignee)

Comment 26

4 years ago
Created attachment 8522236 [details] [diff] [review]
Patch to pref off on non-en-US

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+
Attachment #8522236 - Flags: review+
(Assignee)

Comment 29

4 years ago
Created attachment 8522428 [details] [diff] [review]
Address feedback
Attachment #8522428 - Flags: review?(felipc)
Attachment #8522428 - Flags: review?(felipc) → review+
(Assignee)

Comment 30

4 years ago
(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! :)
(Assignee)

Comment 31

4 years ago
Created attachment 8522487 [details] [diff] [review]
Make test_searchSuggest.js pass
Attachment #8522487 - Flags: review?(felipc)
Attachment #8522487 - Flags: review?(felipc) → review+
(Assignee)

Comment 32

4 years ago
Created attachment 8523071 [details] [diff] [review]
Make the tests pass
Attachment #8522487 - Attachment is obsolete: true
Attachment #8523071 - Flags: review?(felipc)
Attachment #8523071 - Flags: review?(felipc) → review+
(Assignee)

Comment 33

4 years ago
Created attachment 8523830 [details] [diff] [review]
Plug leak

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)
(Assignee)

Comment 34

4 years ago
Created attachment 8523832 [details] [diff] [review]
Plug leak

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+
(Assignee)

Comment 35

4 years ago
Created attachment 8523950 [details] [diff] [review]
Fix bc1 on XP opt

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+
(Assignee)

Comment 36

4 years ago
Created attachment 8524120 [details] [diff] [review]
Tweak wordings on search preferences pane

Tweak the wordings on the Search preference per UX request.
Attachment #8524120 - Flags: review?(felipc)
Attachment #8524120 - Flags: review?(felipc) → review+
(Assignee)

Comment 37

4 years ago
Created attachment 8524166 [details] [diff] [review]
Additional fixes

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+
(Assignee)

Comment 38

4 years ago
Created attachment 8524175 [details] [diff] [review]
Combined beta patch

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
(Assignee)

Updated

4 years ago
Attachment #8524166 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
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+
(Assignee)

Comment 40

4 years ago
Created attachment 8524206 [details] [diff] [review]
Combined patch for fx-team

https://tbpl.mozilla.org/?tree=Try&rev=283a70bc3be
Attachment #8512583 - Attachment is obsolete: true
Attachment #8521598 - Attachment is obsolete: true
Attachment #8524206 - Flags: review+
(Assignee)

Comment 41

4 years ago
Created attachment 8525642 [details] [diff] [review]
Make the 'search settings' more discoverable
(Assignee)

Comment 42

4 years ago
Created attachment 8525644 [details] [diff] [review]
Make the 'search settings' more discoverable

Forgot to hg add the icons for the previous patch...
Attachment #8525642 - Attachment is obsolete: true
(Assignee)

Comment 43

4 years ago
Created attachment 8525657 [details] [diff] [review]
Make the 'search settings' more discoverable

Now tested on Windows/Linux too.
Attachment #8525644 - Attachment is obsolete: true
Blocks: 1101122

Updated

4 years ago
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
(Assignee)

Comment 44

4 years ago
Created attachment 8526160 [details] [diff] [review]
Make the 'search settings' more discoverable and show opensearch items

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+
(Assignee)

Comment 46

4 years ago
Created attachment 8526322 [details] [diff] [review]
Polish CSS for Windows/Linux

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)
(Assignee)

Comment 47

4 years ago
Created attachment 8526329 [details] [diff] [review]
Polish CSS for Windows/Linux

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?
(Assignee)

Comment 50

4 years ago
(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)
(Assignee)

Comment 53

4 years ago
(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)
(Assignee)

Comment 54

4 years ago
Created attachment 8526609 [details] [diff] [review]
Polish CSS for Windows/Linux - fixed

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+
(Assignee)

Updated

4 years ago
Attachment #8526329 - Attachment is obsolete: true

Updated

4 years ago
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: 1102911
Depends on: 1102912
Depends on: 1102919
[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]:
tracking-firefox34: --- → ?
status-firefox34: --- → affected
Depends on: 1102934
Depends on: 1102933
Depends on: 1102942
Depends on: 1102948
status-firefox34: affected → fixed
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox34: ? → +
tracking-firefox35: --- → +
tracking-firefox36: --- → +
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
Gavin asked me to switch these back to 1088660 for now. So there will be more bugmail.
Depends on: 1103119

Updated

4 years ago
Depends on: 1103326

Updated

4 years ago
OS: Mac OS X → All
Hardware: x86 → All

Comment 61

4 years ago
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.

Comment 62

4 years ago
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 ...

Comment 64

4 years ago
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.

Comment 65

4 years ago
Another issue:

There is no visual difference between past searches and search suggestions anymore.

Comment 66

3 years ago
(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

Comment 67

3 years ago
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

Updated

3 years ago
Depends on: 1104142
(Assignee)

Updated

3 years ago
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
(Assignee)

Updated

3 years ago
Depends on: 1103455
Depends on: 1104315
Depends on: 1104325
QA Contact: petruta.rasa

Updated

3 years ago
Iteration: 36.3 → 37.1
(Assignee)

Updated

3 years ago
Depends on: 1104846

Updated

3 years ago
Depends on: 1104971
(Assignee)

Updated

3 years ago
No longer depends on: 1105264
Depends on: 1105278
(Assignee)

Updated

3 years ago
Attachment #8524175 - Flags: approval-mozilla-beta?
Attachment #8524175 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8526160 - Flags: approval-mozilla-beta?
Attachment #8526160 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
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...
No longer depends on: 1105272
(Assignee)

Comment 70

3 years ago
Created attachment 8529717 [details] [diff] [review]
Fix tests on fx-team

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+

Updated

3 years ago
Depends on: 1105750
Depends on: 1105746

Comment 72

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/495a9f216f11
https://hg.mozilla.org/mozilla-central/rev/7c408a498d6c
https://hg.mozilla.org/mozilla-central/rev/6bcf7110d616
https://hg.mozilla.org/mozilla-central/rev/69c30325e867
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox36: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
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+

Comment 77

3 years ago
Where are my keyboard shortcuts?
What if my icons are all same and I can't change that?

Updated

3 years ago
Blocks: 1106205

Updated

3 years ago
Depends on: 1106240

Updated

3 years ago
Depends on: 1106241

Comment 78

3 years ago
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.

Updated

3 years ago
Depends on: 1106333

Updated

3 years ago
Depends on: 1106331
Depends on: 1106559

Updated

3 years ago
No longer depends on: 1106333

Updated

3 years ago
Depends on: 1106569

Updated

3 years ago
Flags: needinfo?(w8fsq+b7cozw6gfo8p8)

Updated

3 years ago
Depends on: 1106476

Updated

3 years ago
Depends on: 1106873

Updated

3 years ago
Depends on: 1106876
(Assignee)

Updated

3 years ago
Depends on: 1106924
(Assignee)

Updated

3 years ago
Depends on: 1106926

Updated

3 years ago
Depends on: 1107135
Depends on: 1107178

Updated

3 years ago
Depends on: 1107266

Updated

3 years ago
Depends on: 1107278

Updated

3 years ago
Depends on: 1107449
(Assignee)

Updated

3 years ago
No longer depends on: 1107471
No longer depends on: 1107494

Updated

3 years ago
Depends on: 1107506

Comment 79

3 years ago
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 ).
(Assignee)

Comment 80

3 years ago
(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.

Comment 81

3 years ago
(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)

Updated

3 years ago
Duplicate of this bug: 365227
(Assignee)

Updated

3 years ago
Depends on: 1108488

Updated

3 years ago
Depends on: 1108646

Updated

3 years ago
Depends on: 1108648

Updated

3 years ago
Depends on: 1108273

Updated

3 years ago
Depends on: 1108490

Comment 83

3 years ago
Two more bugs to add as dependencies: bug 1107137 and bug 1107292 (one is probably a duplicate).

Updated

3 years ago
Depends on: 1107137, 1107292

Updated

3 years ago
Depends on: 1108594

Updated

3 years ago
Depends on: 1109249

Updated

3 years ago
Depends on: 1108230
No longer depends on: 1102934
Alias: fx34-searchui
No longer depends on: 1102918

Updated

3 years ago
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)

Updated

3 years ago
Flags: needinfo?(mmaslaney)
Depends on: 1109839

Updated

3 years ago
Depends on: 1107544

Updated

3 years ago
Depends on: 1109851

Updated

3 years ago
Depends on: 1108254

Updated

3 years ago
Depends on: 1110820

Comment 86

3 years ago
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.

Comment 87

3 years ago
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.

Updated

3 years ago
Depends on: 1111868

Updated

3 years ago
Depends on: 1111943

Updated

3 years ago
Depends on: 1111947
(Assignee)

Updated

3 years ago
No longer depends on: 1107544

Updated

3 years ago
Depends on: 1107503

Updated

3 years ago
Depends on: 1113731

Updated

3 years ago
Depends on: 1113747

Updated

3 years ago
Depends on: 1106599

Updated

3 years ago
Depends on: 1107509

Updated

3 years ago
Depends on: 1107580
(Assignee)

Updated

3 years ago
Depends on: 1103321
No longer depends on: 1108080

Updated

3 years ago
Depends on: 1111305

Updated

3 years ago
Depends on: 1119233
(Assignee)

Updated

3 years ago
Depends on: 1119250
(Assignee)

Updated

3 years ago
Depends on: 1119273

Updated

3 years ago
Depends on: 1119895

Updated

3 years ago
Depends on: 1119987
(Assignee)

Updated

3 years ago
Depends on: 1121535

Updated

3 years ago
Depends on: 1123001

Updated

3 years ago
Depends on: 1123442

Updated

3 years ago
Depends on: 1129587

Updated

3 years ago
Depends on: 1129597

Updated

3 years ago
No longer depends on: 1129587

Updated

3 years ago
No longer depends on: 1129597

Updated

3 years ago
Depends on: 1130480
Depends on: 1131685

Updated

3 years ago
Depends on: 1138176

Updated

3 years ago
Depends on: 1143638

Updated

3 years ago
Depends on: 1160860

Updated

3 years ago
Depends on: 1197480

Updated

3 years ago
Depends on: 1204241

Updated

3 years ago
Depends on: 1204242
(Assignee)

Updated

3 years ago
Depends on: 1137332
(Assignee)

Updated

3 years ago
Duplicate of this bug: 889113
(Assignee)

Updated

3 years ago
Duplicate of this bug: 646277
(Assignee)

Updated

3 years ago
Duplicate of this bug: 607855

Updated

2 years ago
Depends on: 1234134

Updated

2 years ago
Depends on: 1167556

Updated

2 years ago
Depends on: 361923

Updated

2 years ago
Depends on: 1230961

Updated

2 years ago
Depends on: 1251977
You need to log in before you can comment on or make changes to this bug.