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.

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.

- 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.

Fixed icon sizing on retina screens.

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

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

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

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.

Patch working on all 3 platforms, including all the changes I mentioned above.

I tested this on current nightlies on Windows 7/Mac 10.9/Ubuntu 13.10.

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

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

CSS changes needed on Windows and Linux for attachment 8517561 [details] [diff] [review] and attachment 8518938 [details] [diff] [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

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

This applies above attachment 8521789 [details] [diff] [review].

ftp://ftp.mozilla.org/pub/firefox/try-builds/florian@queze.net-6b6ae31e3049/ for those following along at home.

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?

(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! :)

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

Ooops, copy/paste mistake in the patch. I canceled the try server builds and pushed again: https://tbpl.mozilla.org/?tree=Try&rev=af1d2c734b54

Fix the intermittent (but frequent) orange on XP bc1 opt: https://tbpl.mozilla.org/?tree=Try&rev=e8f82ae25121

Tweak the wordings on the Search preference per UX request.

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.

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

Comment on attachment 8524175 [details] [diff] [review] Combined beta patch All the patches that were combined into this one have r=felipe.

Forgot to hg add the icons for the previous patch...

Now tested on Windows/Linux too.

I think this is the last large code-patch. I still need to improve a few details in the CSS for Windows/Linux.

https://hg.mozilla.org/releases/mozilla-beta/rev/9a8a27107a6f https://hg.mozilla.org/releases/mozilla-beta/rev/8622ff2cb47b

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 :).

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.

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.

Backed out of beta in https://hg.mozilla.org/releases/mozilla-beta/rev/9ae0161389d3 for bc1 errors: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=158181&repo=mozilla-beta

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.

Same patch without the -moz-border-colors: none; lines; I verified that things work without them, I must have been confused yesterday evening.

Relanded: https://hg.mozilla.org/releases/mozilla-beta/rev/c53b86f05fd2

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?

[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.

[Tracking Requested - why for this release]:

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.

Gavin asked me to switch these back to 1088660 for now. So there will be more bugmail.

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

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

> - 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

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

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.

https://hg.mozilla.org/integration/fx-team/rev/495a9f216f11 https://hg.mozilla.org/integration/fx-team/rev/7c408a498d6c https://hg.mozilla.org/integration/fx-team/rev/6bcf7110d616 https://hg.mozilla.org/integration/fx-team/rev/69c30325e867

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

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+

https://hg.mozilla.org/releases/mozilla-aurora/rev/9412ac46c023 https://hg.mozilla.org/releases/mozilla-aurora/rev/78772bd35440 https://hg.mozilla.org/releases/mozilla-aurora/rev/a87ab9d302eb https://hg.mozilla.org/releases/mozilla-aurora/rev/b97e451b0e67

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.

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

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.