We know that many users don't know/use the search field in the UI or the awesomebar. Providing a search field on the new tab page would make their search experience more fluid. I know that there are several projects around new tab happening right now, but this looks like such a quick and easy win to me, that we should do it right away, even when the layout of the page will be changed again in the future. This is a good starting point: https://wiki.mozilla.org/File:Centered_search_actually_default_32423.png

Having effectively three search bars seems like over-cluttering the UI, although it does look nice in the screenshot

This works. I pretty much copied the architecture and much of the code from about:home's search box. It uses event- and message-passing to be e10s-safe. It pretty much looks like the mockup in comment 0 although it's not perfect, and it works just like about:home's search box, even changing the search engine based on the selected engine in the main search bar. It needs a test. I filed bug 975786 to make FHR recognize its searches. Boriss, there's an old about:home mockup in comment 0 you posted, but I wanted to check whether you had anything else to add. Feel free to try this patch or just give more general comments. (I wish about:home and about:newtab were combined like that mockup shows...)

(In reply to Drew Willcoxon :adw from comment #2) > Created attachment 8380300 [details] [diff] [review] > working WIP > > This works. I pretty much copied the architecture and much of the code from > about:home's search box. It uses event- and message-passing to be > e10s-safe. It pretty much looks like the mockup in comment 0 although it's > not perfect, and it works just like about:home's search box, even changing > the search engine based on the selected engine in the main search bar. It > needs a test. I filed bug 975786 to make FHR recognize its searches. > > Boriss, there's an old about:home mockup in comment 0 you posted, but I > wanted to check whether you had anything else to add. Feel free to try this > patch or just give more general comments. > > (I wish about:home and about:newtab were combined like that mockup shows...) Adding a mockup. We don't need to allow changing the search engine in v1. adw, Firefox intern vt made a patch for the modular search bar, which may be usable here, in Bug 966107.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #3) > adw, Firefox intern vt made a patch for the modular search bar, which may be > usable here, in Bug 966107. That implementation doesn't match your mockup. Should we change it to match the mockup? Should all in-content search bars look the same?

(In reply to Drew Willcoxon :adw from comment #4) > (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #3) > > adw, Firefox intern vt made a patch for the modular search bar, which may be > > usable here, in Bug 966107. > > That implementation doesn't match your mockup. Should we change it to match > the mockup? Should all in-content search bars look the same? Yes, we are going to need to change this field a bit. The exact styling is dependent on the design spec I'm working now for tiles, which isn't quite done. I'll upload a spec here when that's finished & sync with adw.

Carry over to Iteration it-30c-29a-28b.3

Hey folks, This looks really cool! Well before you are ready to move forward to Nightly, we should chat about the business issues around adding a new Search Access Point. Can whoever owns this project reach out to me on email and I can explain the items we'll need to resolve before this can launch? Thanks, Joanne

The attached mockup shows the intended design of the Search Field in New Tab. Some notes: - Search bar should be aligned to left and rightmost tiles, even when resized. While changes are being made to Tile size and ratio, this should be maintained - 32px gutter should be maintained at top of search bar and left of search bar always. - 32px gutter should disappear from right side of window only when window is too small to maintain 32px gutter and one complete tile - Design reflects changes to New Tab taking place in bug 975208 and bug 975199

(In reply to Joanne Nagel from comment #7) > This looks really cool! Well before you are ready to move forward to > Nightly, we should chat about the business issues around adding a new Search > Access Point. Can whoever owns this project reach out to me on email and I > can explain the items we'll need to resolve before this can launch? Thanks, Taking this on. bug 980606 is a blocker for required affiliate codes. Please don't land this, even in Nightly, until we have those codes from our partners. Thanks!

(In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #9) > (In reply to Joanne Nagel from comment #7) > > This looks really cool! Well before you are ready to move forward to > > Nightly, we should chat about the business issues around adding a new Search > > Access Point. Can whoever owns this project reach out to me on email and I > > can explain the items we'll need to resolve before this can launch? Thanks, > > Taking this on. bug 980606 is a blocker for required affiliate codes. > Please don't land this, even in Nightly, until we have those codes from our > partners. Thanks! Adw, is it possible to work on the design of this before we have the affiliate codes, so we'll be ready to deploy quickly when we do?

Absolutely. If "affiliate codes" mean what I think they mean -- small strings we plug into the search URLs we generate -- then we can add those to the code at the last minute. Not having them shouldn't even block implementation.

(In reply to Drew Willcoxon :adw from comment #11) > Absolutely. If "affiliate codes" mean what I think they mean -- small > strings we plug into the search URLs we generate -- then we can add those to > the code at the last minute. Not having them shouldn't even block > implementation. Fantastic. In that case, I'll stay tuned for questions/comments on attachment 8386518 [details] to see if anything's unclear, too time-consuming to implement, if graphics are needed, etc. Blocking bug 980606 still has open non-eng questions before it can be resolved.

I did some work in bug 966107 to make a modular search bar that could be used on any in-content page such as new tab, home page or the new network error pages I've been working on. For the sake of consistency I think all three locations we use a search bar should look and behave the same. This should extend as far as sharing the same code. adw, how do you think we should go about tackling this problem so we can keep our code DRY and use our time effectively?

(In reply to Valentin Tsatskin [:vt] from comment #13) > For the sake of consistency I think all > three locations we use a search bar should look and behave the same. This > should extend as far as sharing the same code. Absolutely, in the basic graphic level. However, small parameters such as the length and height of the search bar, if flexible, could allow us to adapt the search to different designs and still look visually cohesive to users.

[qa+] this. I think I minus'd it previously thinking this was a design work bug

Talked on IRC: Search engine images are needed, and also a spec showing how engines without images should be displayed. The attached shows how search engines with and without images should display.

Attaching placeholder .png images for search engines. For the final, I'm assuming an svg or otherwise scalable image might be needed.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #17) > Created attachment 8393213 [details] > Design Spec: Changing Search Engine, with and without available images > > Talked on IRC: Search engine images are needed, and also a spec showing how > engines without images should be displayed. > > The attached shows how search engines with and without images should display. We might have at least a favicon for other engines if you want to use those. Also it would be good if those images were all the same size so we could offer a size that could be specified by other sites and it would show up there. You could probably even do min/max height and widths which would be acceptable and used. i.e. opensearch spec allows for others to add something this: <Image width="65" height="26" type="image/png">http://example.com/search-image.png</Image> I'm assuming here that we use this same system for specifying our built in opensearch engines.

Following on Bryan's comment, I'm the process of analyzing some qualitative tests of how users use search in Firefox that were conducted last week with 30 participants online. I'll share the complete results shortly, but since this bug is in discussion I want to provide one preliminary observation. The participants (especially those who primarily used the searchbox for searches) were not always aware they could change the search engine in the searchbox. The image helped identify users to the primary search engine selected. However, even with the image (and the adjacent nib), some users did not know that they could click on the image to have a dropdown selection among search engines. I suggest providing a stronger and clearer affordance for selecting among search engine options in the dropdown on the search field to the new tab page.

(In reply to Bill Selman from comment #20) > Following on Bryan's comment, I'm the process of analyzing some qualitative > tests of how users use search in Firefox that were conducted last week with > 30 participants online. I'll share the complete results shortly, but since > this bug is in discussion I want to provide one preliminary observation. Bill, thanks for the insight. I'm sure it is correct, but at this time it's ok that users not know they can change the search engine. I suspect few will, and in fact it's marked as a "stretch" goal now. The current design does not allow switching search engines, and doing so now is towards a move of unifying our search widgets and giving users in all places the choice they should expect. For now, tis is a bit of an "easter egg," and I suspect most people will only see it via mouseover. (In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #19) > i.e. opensearch spec allows for others to add something this: > <Image width="65" height="26" > type="image/png">http://example.com/search-image.png</Image> > > I'm assuming here that we use this same system for specifying our built in > opensearch engines. Good point, and I just measured - 65x26 is exactly what the design is, so let's specify that as the target size for now.

Incomplete WIP. Try builds will be here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-a5949760ae95

Add 'max-height: 20px;' to #newtab-search-form to prevent this chunkiness.

Try builds will be here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-bbd8ed65690e Boriss, I think this matches your design except for the colors, which I took a stab at by eye and with OS X's built-in eye dropper. Could you give me feedback on it? Also, do we have higher-res logos? There's one thing not covered by the mockup, which is the undo box that appears when you click a tile's X. I positioned it 32px above the search bar.

I did some exploratory testing on the try build under Win 7 64-bit and I noticed the following issues on "Manage Search Engines" option: - the position of search engines is modified only on Search field from toolbar - "Get more search engines" doesn't open the expected link I will continue testing tomorrow.

Thanks, Petruta, very helpful. (In reply to Petruta Rasa [QA] [:petruta] from comment #25) > - "Get more search engines" doesn't open the expected link I should fix this one before landing. > - the position of search engines is modified only on Search field from > toolbar But in a pinch I think it would be OK to push this one to a follow-up bug. It shouldn't be hard to fix, though.

(In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #9) > Taking this on. bug 980606 is a blocker for required affiliate codes. > Please don't land this, even in Nightly, until we have those codes from our > partners. Thanks! Partners have been informed and we have the go ahead. We don't have the codes yet but should be getting them within a week or so. bug 980606 doesn't need to block this from landing in Nightly. That said, we shouldn't uplift beyond Nightly until we get the codes landed.

(In reply to Drew Willcoxon :adw from comment #24) > Created attachment 8397579 [details] [diff] [review] > WIP 3 > > Try builds will be here: > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/ > dwillcoxon@mozilla.com-bbd8ed65690e > > Boriss, I think this matches your design except for the colors, which I took > a stab at by eye and with OS X's built-in eye dropper. Could you give me > feedback on it? Also, do we have higher-res logos? Very good question. Let me check with graphics/business and see if we can answer that. If using the branding logos these companies have available is ok with them, that may work for now. If you have suggestions on the ideal file format/size, that would be helpful too. > There's one thing not covered by the mockup, which is the undo box that > appears when you click a tile's X. I positioned it 32px above the search > bar. A few things: - We need a mouseover state on switching search providers. Let's go with #e9e9e9, as that's the same grey as moused over items in Australis' customization menu - "Manage Search Engines" should be vertically aligned in the search engine panel. In this build it's displaying too far up Great work, adw. This is really starting to look excellent.

Felipe, since you reviewed the about:home e10s changes, would you mind giving feedback on this patch? I modeled it after about:home, making it e10s-safe and also not assuming that about:newtab is privileged. (But now that I actually went and looked up where about:home's message passing code comes from (bug 899222), I see that Bill says, "Our goal for most about: pages is to load them non-remotely." So maybe this work was unnecessary... :-/) This patch has a lot of CSS and XUL changes, but I'm only asking for feedback on the message-passing architecture, which is in these files: browser/base/content/content.js browser/base/content/newtab/page.js browser/base/content/newtab/search.js browser/components/nsBrowserGlue.js browser/modules/ContentSearch.jsm content.js mediates communication between about:newtab content in {page,search}.js and chrome in ContentSearch.jsm.

Making one small adjustment to the design, per some very helpful feedback from Shorlander and Gavin: instead of showing wordmarks on the Search Engine dropdown, let's show favicons of those engines instead. One a search engine is selected, its wordmark would appear at the top of the search bar. This approach gives a few benefits: - Better consistency with current search bar, in which favicons are shown - Less language redundancy, since images for search engines would often be wordmarks - Maintaining ability to quickly scan the list for either visual cue or text name

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #28) > Great work, adw. This is really starting to look excellent. Thanks! Try builds with the new design will be here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-ec6ebad060e8

Comment on attachment 8400326 [details] [diff] [review] WIP 4 Review of attachment 8400326 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content.js @@ +195,5 @@ > + init: function (chromeGlobal) { > + chromeGlobal.addEventListener("ContentSearchClient", this, true, true); > + }, > + > + handleEvent: function (event) { you'll need to check if these events really came from about:newtab ::: browser/base/content/newtab/page.js @@ +139,5 @@ > + > + //XXXadw content.js is only for browser content windows. the newtab > + // preloader uses a browser in the hidden window, so that browser > + // doesn't get content.js, which is why this is necessary. > + gSearch.registerWindow(); I thought ttaubert had fixed this by loading these listeners in the preloader too, using the getDelayedFrameScripts() function. Can you double check?

Thanks, Felipe. I'm not sure how to ask for review on this. There's the ContentSearch stuff that I asked Felipe for feedback on that I want all in-content search bars to use, plus its tests. And then there's all the newtab XUL, CSS, and JS changes that use ContentSearch, plus its tests. (Writing those tests now.)

Attached are search engine images in three formats: a. Original. How I found it online in its natural habitat. b. 65x26 pixels. How engines would appear on New Tab for all non-retina-OSX displays. c. 130x52 pixels. How engines would appear on retina-OSX displays

do we have the OK from the legal team to use the icons? IIRC we had to ask before using the google logo in about:home, and it took some time before got the approval. It may be a good idea to add these to our searchplugins definitions if we start using them more often... OR just fallback to a design that just uses the searchbar icons.

(In reply to Marco Bonardo [:mak] from comment #35) > do we have the OK from the legal team to use the icons? IIRC we had to ask > before using the google logo in about:home, and it took some time before got > the approval. > It may be a good idea to add these to our searchplugins definitions if we > start using them more often... OR just fallback to a design that just uses > the searchbar icons. Joanne is checking with the individual search providers to verify that we can use these images, but in the meantime please go ahead and use them for Nightlies. The legal team is aware we're doing so and has given the go-ahead.

This allows content to use the search service in a non-privileged and e10s-safe way. It's basically the same patch I asked Felipe for feedback on, but only the ContentSearch part, and with a test.

This updates newtab to use the previous patch. I'm not sure where you draw the line between content CSS and theme CSS.

This updates the Google and Bing tests to do a search on newtab. Note that right now the expected URLs are just the "base" URLs since we don't have codes for newtab.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #36) > Joanne is checking with the individual search providers to verify that we > can use these images, but in the meantime please go ahead and use them for > Nightlies. The legal team is aware we're doing so and has given the > go-ahead. Thanks for working on those, Boriss. I think it's a little tricky including these images only on Nightly due to the way the patches work. They'd live in the search plugin XML, which would have to somehow be preprocessed, which I don't think we're set up to do, but I could be wrong. The patches I posted don't use them, but we can easily drop them in later and they'll just start working, either in this bug or a new one.

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-b38d65ba619e https://tbpl.mozilla.org/?tree=Try&rev=b38d65ba619e

Adw, I've been looking at the new try-builds (under Windows, Mac OSX 10.9.2 and Ubuntu 32-bit) and observed that now there is no button before the search text field, so now the search engine can be changed only from the toolbar search. Is this intended? Also, on Windows builds (7 64-bit, 8 32-bit), the "Search" button is no longer blue and close to the text field (as in design). This is not the case for Mac and Ubuntu builds. Please see the Win 7 screenshot: http://i.imgur.com/BM85jup.jpg

(In reply to Petruta Rasa [QA] [:petruta] from comment #42) > Adw, I've been looking at the new try-builds (under Windows, Mac OSX 10.9.2 > and Ubuntu 32-bit) and observed that now there is no button before the > search text field, so now the search engine can be changed only from the > toolbar search. Is this intended? Yes, the latest build doesn't include any temporary/development logos, so the panel can't be easily manually tested. You can add a search plugin to your profile that has a 65x26 image (or 130x52 if your screen is 2x DPI), and it will be picked up. > Also, on Windows builds (7 64-bit, 8 32-bit), the "Search" button is no > longer blue and close to the text field (as in design). This is not the case > for Mac and Ubuntu builds. Thanks, that's a bug.

Actually that may not be a bug. When the search field is not focused, it shouldn't have a blue border, and the button shouldn't be blue. Was the field unfocused when you took that picture? The field and button should look exactly the same as the ones on about:home, including when they're not focused. (In reply to Drew Willcoxon :adw from comment #38) > I'm not sure where you draw the line between content CSS and theme CSS. I had forgotten that about:home just puts everything in one content CSS file, so maybe we should do that, too.

Comment on attachment 8403104 [details] [diff] [review] Google, Bing tests patch >diff --git a/browser/components/search/test/browser_bing_behavior.js b/browser/components/search/test/browser_bing_behavior.js >+ // Let gSearch respond to the event before continuing. >+ setTimeout(() => doSearch(win.document)); You should just use "executeSoon" (defined globally in browser-chrome tests, same comment applies to the other test). If we want to unblock this on bug 980606 we should spin off adding the right parameters to another (tracked) bug.

(In reply to Drew Willcoxon :adw from comment #41) > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/ > dwillcoxon@mozilla.com-b38d65ba619e > > https://tbpl.mozilla.org/?tree=Try&rev=b38d65ba619e This build without images is fine for v1, particularly before we hear back from Joanne & the search engine companies. One problem I'm running into, though, is that the Back button doesn't seem to work after a search query or click on a Tile. Perhaps this needs to be another bug, but Back should be enabled on *all* actions removing the user from New Tab.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #46) > One problem I'm running into, though, is that the Back button doesn't seem > to work after a search query or click on a Tile. Perhaps this needs to be > another bug, but Back should be enabled on *all* actions removing the user > from New Tab. Bug 724239 created that behavior. If you file a bug to revert it, I'm totally down for fixing it (... if it's prioritized. :-/).

Comment on attachment 8403102 [details] [diff] [review] ContentSearch Gavin points out that init() needs to be called on Services.search before using it. It's async, but it shouldn't be too hard to update the patch because everything there is async.

https://tbpl.mozilla.org/?tree=Try&rev=0c5420d9e2b9 Something's wrong with browser_newtab_drag_drop_ext.js on Win 7. Need to figure that out.

(In reply to Drew Willcoxon :adw from comment #44) > When the search field is not focused, it > shouldn't have a blue border, and the button shouldn't be blue. Was the > field unfocused when you took that picture? The field and button should > look exactly the same as the ones on about:home, including when they're not > focused. Yes, the field was unfocused, but it still doesn't look like about:home (or other OSs) when focused/unfocused: the text field margins and the Search button (at hover) have a light blue color and there is 1px between the text field and the button.

Update on partner approvals: Amazon - waiting for logo approval Bing - waiting for logo approval, new form code = MOZTSB, pc = MOZI eBay - logo is approved Google - sending new approved logos, waiting for new channel id Yahoo - waiting for logo approval, waiting for new search tag

(In reply to Drew Willcoxon :adw from comment #47) > (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #46) > > One problem I'm running into, though, is that the Back button doesn't seem > > to work after a search query or click on a Tile. Perhaps this needs to be > > another bug, but Back should be enabled on *all* actions removing the user > > from New Tab. > > Bug 724239 created that behavior. If you file a bug to revert it, I'm > totally down for fixing it (... if it's prioritized. :-/). Looks from Comment 51 that getting image approvals is on track. I believe we can land this before we have all the approvals and make followup bugs to change images as needed. To be clear, if we can close this bug with the images but without all approvals, let's do it. If we can't close this bug without approvals, let's go back to text.

This makes the changes we talked about today. It removes about:blank from the whitelist. Instead, the test sends a message to the content script to add about:blank to its whitelist. This means that content.js unconditionally adds a message listener, in order to listen for this message. Previously it added a message listener only when a whitelisted page fired a RegisterWindow event. It also means that pages don't have to register themselves anymore. (And actually it wasn't pages that were registering, but browsers, since like you pointed out, listeners stick around when the page is changed.) It also adds documentation to ContentSearch.jsm. I'll post the interdiff from the previous patch next.

* Updates gSearch to work with the ContentSearch change I just posted. * Moves all the CSS to the content CSS, like about:home does. * Tightens up gSearch and removes inline event handlers. * Fixes the browser_newtab_focus.js failure. * The Win 7 test failures were due to the fact that the search bar reduces the height available to the tiles, and now that the number of tiles shown depends on the available height, when browser_newtab_drag_drop_ext.js added an iframe to the top of the page, it pushed out the bottom row of tiles. That led to a timeout when the test tried to drag a link to the bottom row. So this uses a new window instead of inserting an iframe into the page. I don't have a complete interdiff of this one unfortunately.

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-649f791f73de https://tbpl.mozilla.org/?tree=Try&rev=649f791f73de

(In reply to Drew Willcoxon :adw from comment #55) > * The Win 7 test failures were due to the fact that the search bar reduces > the height available to the tiles, and now that the number of tiles shown > depends on the available height, when browser_newtab_drag_drop_ext.js added > an iframe to the top of the page, it pushed out the bottom row of tiles. Actually it looks like the window is just too short, nothing necessarily to do with the iframe. So another try, going back to using an iframe, but this time making the window's height bigger if necessary: https://tbpl.mozilla.org/?tree=Try&rev=079ba0fd39ec

Try results look OK so far... so here's the updated newtab patch. It's the same as the previous except for head.js, which goes back to using an iframe for drag and drop and resizes the browser if necessary. browser_ContentSearch.js from the other patch had some errors logged to the console that were caused by the patch but unrelated to the test, so the test didn't fail, but I fixed the errors locally.

(In reply to Drew Willcoxon :adw from comment #56) > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/ > dwillcoxon@mozilla.com-649f791f73de > > https://tbpl.mozilla.org/?tree=Try&rev=649f791f73de I'm seeing 2 bugs in that build: the search bar is styled as though it were focused on load, but in fact the URL bar is focused. In fact, when I focused the new tab search bar and then unfocused it, it changed to the correct unfocused state. Second, I'm not seeing a search suggestion dropdown as on about:home (http://cl.ly/image/130F3z0r0G30)

Actually, there appears to be a regression: the back button doesn't work on new tab on these builds once the user clicks on something in new tab.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #60) > Actually, there appears to be a regression: the back button doesn't work on > new tab on these builds once the user clicks on something in new tab. I see this bug on normal nightlies too I think.

I've complained about this for about:home and I'll add this here in a different form. Please actually try the current google search page - google.com. You will see that as soon as you start typing, the search bar pops to the top of the screen so google can give you live samples of search results as you type. In addition, it gives type ahead guesses. All this is lost with the embedded search in about:home, and I assume with your about:newtab work. My previous idea in bug 993792 was to allow me to customize ctrl-k (or about:home) to get back to google.com. An alternative is to make the embedded search bar work the way google.com does - popping into the google.com live search when you start typing.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #60) > Actually, there appears to be a regression: the back button doesn't work on > new tab on these builds once the user clicks on something in new tab. Is that not what we already talked about in comment 47?

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #34) > Created attachment 8402228 [details] > Search engine icons: Original, 65x26 pixels, 130x52 pixels > > Attached are search engine images in three formats: > a. Original. How I found it online in its natural habitat. > b. 65x26 pixels. How engines would appear on New Tab for all non-retina-OSX > displays. > c. 130x52 pixels. How engines would appear on retina-OSX displays Just looking through these the Google 130x52 logo seems to be cut off on the left side. Otherwise it all looks good.

(In reply to Drew Willcoxon :adw from comment #63) > (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #60) > > Actually, there appears to be a regression: the back button doesn't work on > > new tab on these builds once the user clicks on something in new tab. > > Is that not what we already talked about in comment 47? Yep, I think this is the same thing. And I'll add that I'm all for making that change as well.

There's a persistent intermittent failure in browser_newtab_sponsored_icon_click.js on Linux debug with these patches for some reason. https://tbpl.mozilla.org/?tree=Try&rev=fb85968b86b4

For the browser_newtab_sponsored_icon_click.js test runs that timeout, they make it past the check for sponsoredPanel.state == closed but then seems to wait forever for popupshown. Some reason the event isn't triggering or is happening at the wrong time? I do see it runs immediately after browser_newtab_search.js where one of the later checks are "Search panel should be open" Is it possible the search panel stays open and prevents the sponsored panel from opening?

(In reply to Joanne Nagel from comment #51) More update on partner approvals: > Amazon - waiting for logo approval > Bing - waiting for logo approval, new form code = MOZTSB, pc = MOZI Have approval + logo guidelines, just need to update to the gray logo. > eBay - logo is approved > Google - sending new approved logos, waiting for new channel id Joanne will get us approved logos on Monday. > Yahoo - waiting for logo approval, waiting for new search tag

Color adjustment to Bing logo after talking with their branding folks

(In reply to Ed Lee :Mardak from comment #67) > Is it possible the search panel stays open and prevents the sponsored panel > from opening? I tried waiting until popuphidden when closing the search panel and until domwindowclosed when closing the engine management window, but there were still failures, the same failure where the sponsored test is left waiting for popupshown: https://tbpl.mozilla.org/?tree=Try&rev=369cf131fa7a All the failures have the same screenshot, which shows one, blank tab in the (foreground) window. But there should be two tabs, with the second one selected and showing about:newtab since the sponsored test opens about:newtab. I don't get how the sponsored test is even running and passing its first is() if about:newtab doesn't appear to be open. http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/c21d1b88dd3121f9931525c8cab71a529560dbe8e8c53526665c1766b3606cf438292f11a9a135f99ad01c7196fee289b457884f3898be1b63076086d8f81d7b In previous try runs, screenshots sometime looked like the above but also sometimes right, even with a focus ring drawn around the sponsored button: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/29aaed0edfd651b8a9d2fc265e426119d18e44b0dfcd0b414923d5eebaef03d1071a5aa6c691ba40408617bd9b01ab30778c46d3101c811001523535d3e49598

Amazon replacement logo (they wanted a different version of the logo)

For Petruta, a test search plugin file with a logo.

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #59) > I'm seeing 2 bugs in that build: the search bar is styled as though it were > focused on load, but in fact the URL bar is focused. I fixed this so that the search text field really does have focus on load, but I have a question about the design. The design spec says that the search engine should be focused on load. I think you mean the search text field specifically, right? If so, are you sure? The current behavior of focusing the location bar after a opening a new tab is probably ingrained in a lot of people, me included. Cmd+T to open a new tab, start typing to look up a page in the awesomebar. I do that constantly.

To add to Drew's point, this to me is also an accessibility issue. Focus will have changed locations from where it traditionally is, leaving blind and partially sighted users having to re-find their way around Firefox.

Comment on attachment 8405714 [details] [diff] [review] newtab patch v2.1 Review of attachment 8405714 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay :/ r=me with comments addressed. More thoughts: 1) The search engines in your try build didn't have any logos so I couldn't get the panel to show up - I thus just called .openPopup() myself. Should we have a :hover state for the currently hovered .newtab-search-panel-engine? I don't see any indication in the try build but that could also be due to how I opened the panel. 2) Unlike the search box right to the location bar, the input field doesn't support hitting Esc to clear the text field. I think it would be great to support that. 3) I totally agree that focusing the search field is the wrong thing to do when opening the new tab page. ::: browser/base/content/newtab/grid.js @@ +211,5 @@ > + > + // Resize the search bar. > + let width = parseFloat(window.getComputedStyle(this._node).width); > + let visibleCols = Math.floor(width / this._cellWidth); > + gSearch.setWidth(visibleCols * this._cellWidth - this._cellMargin); This is really not ideal but we probably have to live with that for now. Can we at least maybe query #newtab-grid's width and use that instead of doing all those calculations here? ::: browser/base/content/newtab/newTab.css @@ +250,5 @@ > + -moz-box-pack: center; > +} > + > +#newtab-search-container[page-disabled] { > + opacity: 0; Please add |pointer-events: none| here. ::: browser/base/content/newtab/search.js @@ +9,5 @@ > + currentEngineName: null, > + > + init: function () { > + window.addEventListener("ContentSearchService", this); > + this._send("GetState"); We could call this.registerWindow() here. @@ +13,5 @@ > + this._send("GetState"); > + }, > + > + registerWindow: function () { > + this._send("GetState"); registerWindow() is not a great name, we don't really register any window here, all we do is retrieve state and create/initialize the whole panel. @@ +18,5 @@ > + }, > + > + showPanel: function () { > + let panel = this.$("panel"); > + let logo = this.$("logo"); Could we define them all in init? for (let suff of ["panel", "logo", ...]) { this[elem] = document.getElementById("newtab-search-" + suff); } @@ +54,5 @@ > + handleEvent: function (event) { > + switch (event.type) { > + case "ContentSearchService": > + this["on" + event.detail.type](event.detail.data); > + break; We can remove the switch statement if we only handle a single event here. @@ +94,5 @@ > + > + // Empty the panel except for the Manage Engines row. > + while (panel.childNodes.length > 1) { > + panel.firstElementChild.remove(); > + } Wouldn't this break once we have a single non-element child in there? @@ +110,5 @@ > + box.setAttribute("engine", engine.name); > + > + box.addEventListener("click", () => { > + this._send("SetCurrentEngine", engine.name); > + panel.hidePopup(); panel isn't defined here. Needs to be this.$("panel"). ::: browser/base/content/test/newtab/browser_newtab_search.js @@ +42,5 @@ > + addSearchEngine(ENGINE_NO_LOGO); > + waitForSearchEvent("State"); > + info("Waiting for no-logo engine to be added..."); > + yield null; > + yield null; This is a little too magic (I wish Task.jsm had existed when I wrote this test suite :/). Can you please combine those two in a helper function and make it look like: yield waitForSearchEngine(ENGINE_LOGO); ... and then wait for the right search engine to be added and a search event behind the scenes? @@ +60,5 @@ > + panel.removeEventListener("popupshown", onShown); > + TestRunner.next(); > + }); > + info("Waiting for search panel to open..."); > + yield EventUtils.synthesizeMouseAtCenter(logoImg(), {}, getContentWindow()); When using synthesizeMouse*() we should probably call waitForFocus(..., getContentWindow()) before to ensure we don't introduce another intermittent orange. @@ +106,5 @@ > + is(subj.opener, gWindow, > + "Search engine manager opener should be the chrome browser " + > + "window containing the newtab page"); > + subj.close(); > + setTimeout(() => TestRunner.next(), 0); executeSoon(TestRunner.next); @@ +113,5 @@ > + } > + }); > + EventUtils.synthesizeMouseAtCenter(manageBox, {}, getContentWindow()); > + info("Waiting for the search manager window to open..."); > + yield null; Maybe: yield waitForEngineManager(); ... and move the watcher into its own helper function? @@ +137,5 @@ > +} > + > +function checkCurrentEngine(basename) { > + let engine = Services.search.currentEngine; > + ok(engine.name.indexOf(basename) >= 0, "Sanity check: current engine"); ok(engine.name.contains(basename), "Sanity check: current engine"); @@ +143,5 @@ > + // gSearch.currentEngineName > + is(gSearch().currentEngineName, engine.name, "currentEngineName"); > + > + // search bar logo > + let logoSize = [65 * window.devicePixelRatio, 26 * window.devicePixelRatio]; Can you please define 65 and 26 as constants? I don't know what they are but I guess that we have 65x26 pixel search engine logos? @@ +167,5 @@ > + } > + } > +} > + > +function waitForSearchEvent(type, callback) { function waitForSearchEvent(type, callback = TestRunner.next) { @@ +175,5 @@ > + info("Got search event " + event.detail.type); > + if (event.detail.type == type) { > + win.removeEventListener(SERVICE_EVENT_NAME, onEvent); > + // Let gSearch respond to the event before continuing. > + setTimeout(callback || (() => TestRunner.next()), 0); executeSoon(callback);

(In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #68) > (In reply to Joanne Nagel from comment #51) > Update on partner approvals: Approved & Ready: Amazon - (with new logo) Bing - (with gray logo) new form code = MOZTSB, pc = MOZI eBay - Approved: Google - have logos, waiting for new channel id Boriss, how is the Google logo work coming? Joanne, checking in on the channel id progress > Yahoo - waiting for logo approval, waiting for new search tag Joanne, how are we doing with Yahoo for logo and channel id?

Here's my update: Google Channel ID - pending, just need before we move into GA Yahoo - pending, I have had multiple conversations (and shared Boriss' mockups) and there still seems to be some confusion on their side with another project. Hoping to resolve this by next week at the latest.

(In reply to Tim Taubert [:ttaubert] from comment #78) > Should we have a :hover state for the currently hovered > .newtab-search-panel-engine? I don't see any indication in the > try build but that could also be due to how I opened the panel. There's no panel-engine :hover state so you weren't missing anything. Boriss's design doesn't call for one, and she hasn't commented on its absence when she's used some try builds here. If we decide we do want it, it'd be easy to add in a follow-up. > 2) Unlike the search box right to the location bar, the input field doesn't > support hitting Esc to clear the text field. I think it would be great to > support that. I didn't know that -- and actually when I hit Esc in the main search bar, it doesn't do anything. :-| Maybe I'm misunderstanding what you mean? Anyway, this project is already over budget, so I'd rather not add new features now. Good for a follow-up. Other comments addressed (locally) except: > ::: browser/base/content/newtab/grid.js > @@ +211,5 @@ > > + > > + // Resize the search bar. > > + let width = parseFloat(window.getComputedStyle(this._node).width); > > + let visibleCols = Math.floor(width / this._cellWidth); > > + gSearch.setWidth(visibleCols * this._cellWidth - this._cellMargin); > > This is really not ideal but we probably have to live with that for now. Can > we at least maybe query #newtab-grid's width and use that instead of doing > all those calculations here? The quoted code does query #newtab-grid's width. Are you talking about the other code that's above that? Or the getComputedStyle()?

The code is from comment 79. The test part of this patch builds on the test patch already posted. I'm trying to get my ducks in a row for landing. I'm not planning on waiting on the other codes to land, but since this one's ready let's include it.

Whoops, meant to include browser_bing.js, too.

These are the two logos we have so far. I'm planning on landing the ContentSearch, newtab, and search component test patches plus bug 975786 once try results finish in 30-60 minutes, assuming they're OK. Those are the four main patches. The other patches so far are the new Bing parameter and this logo patch. I want to make sure the main patches stick before landing any others. We can land parameters and logos piecemeal as they're ready.

Can we only package the 2x images and scale them down in the non-hidpi cases? As bug 986676 points out it's kind of sucky to have images in the XML like this, particularly larger+duplicated ones. (I wouldn't treat this as a blocker to your landing - we can followup to improve things.)

ContentSearch: https://hg.mozilla.org/integration/fx-team/rev/eeafc69ebfb1 newtab: https://hg.mozilla.org/integration/fx-team/rev/a36dd9f25739 Search component tests: https://hg.mozilla.org/integration/fx-team/rev/532886a149ab Most recent try run: https://tbpl.mozilla.org/?tree=Try&rev=8324896978fe Slightly older try run (Linux only): https://tbpl.mozilla.org/?tree=Try&rev=7ebf3367b90b I guess I'll mark this as leave open until all the codes and logos land, but if Gavin or anybody else thinks we should handle those in different bugs, that's fine with me. (In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #88) > Can we only package the 2x images and scale them down in the non-hidpi > cases? Probably... I noticed that's how the Google logo in about:home works. I thought that the search providers might want total control over both images rather than relying on Firefox automatically resizing a high-DPI version, but maybe I'm overthinking it.

https://hg.mozilla.org/mozilla-central/rev/eeafc69ebfb1 https://hg.mozilla.org/mozilla-central/rev/a36dd9f25739 https://hg.mozilla.org/mozilla-central/rev/532886a149ab

New Bing parameter: https://hg.mozilla.org/integration/fx-team/rev/8e8876136974 Bing and Amazon logos: https://hg.mozilla.org/integration/fx-team/rev/ef18d9fd18a1

https://hg.mozilla.org/mozilla-central/rev/8e8876136974 https://hg.mozilla.org/mozilla-central/rev/ef18d9fd18a1

Is this en-US only? Because we're fine on l10n with Bing and Google (centralized), not so much with Amazon.

Large part of content/newtab/newTab.css should be in /skin/ as it is theme specific, and full-theme may want to provide their own styling (without having to override/undo the default theme styling).

alfredkayser, anything in particular you believe should be moved? I believe the changes here were following the pattern of about:home where everything is under /content as opposed to /skin.

Missed one check that I should have added to browser_bing.js in https://hg.mozilla.org/integration/fx-team/rev/8e8876136974 comment 91 (thanks to mconnor for spotting it): https://hg.mozilla.org/integration/fx-team/rev/e68ffdbbed26

(In reply to Drew Willcoxon :adw from comment #89) > I guess I'll mark this as leave open until all the codes and logos land, but > if Gavin or anybody else thinks we should handle those in different bugs, > that's fine with me. Yeah, let's split that off. This bug is long enough!

Here is the Yahoo-approved logo, I am still working on confirming whether or not they want a separate search tag for this.

Let's close this bug since the main implementation work is done. Bug 980606 can track the remaining search provider codes, and I filed bug 1006203 to track the remaining provider logos.

This is Firefox 31 actually. The only thing that didn't make it for 31 and landed for 32 is the small test addition in https://hg.mozilla.org/integration/fx-team/rev/e68ffdbbed26 in comment 96. My fault for leaving this open, should have closed it after the main parts landed.

This missed the 31 train and landed on 32 instead, unlike everything else. It's a small but important test addition we should land on 31. [Approval Request Comment] Bug caused by (feature/regressing bug #): described above User impact if declined: If the thing this is testing breaks, then we're using the wrong Bing search partner code on 31, which is bad. Testing completed (on m-c, etc.): It's part of an automated test that's been running on m-c and fx-team for several days. Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none

That is indeed a great new feature for 31. I added it to the release notes for 31. Wording: "Add the search field to the new tab page"

I continued testing this feature across platforms. I'm not marking it yet as verified as it is treated like a feature and will be tested all current cycle. Also I still have to cover some scenarios and log potential issues.

Comment on attachment 8418164 [details] [diff] [review] Small browser_bing.js follow-up from comment 96 https://hg.mozilla.org/releases/mozilla-aurora/rev/aa6f29455c25

When the new tab page was designed, there was a lot of user research regarding why nine tiles was the best number. However this work has effectively gone against all of that. Would it be possible to add a pref to display the redundant search box and restore old behaviour?

(In reply to Paul [pwd] from comment #106) > When the new tab page was designed, there was a lot of user research > regarding why nine tiles was the best number. However this work has > effectively gone against all of that. What do you mean? There are still 9 tiles by default.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #107) > (In reply to Paul [pwd] from comment #106) > > When the new tab page was designed, there was a lot of user research > > regarding why nine tiles was the best number. However this work has > > effectively gone against all of that. > > What do you mean? There are still 9 tiles by default. Hah, if only. I have six on this laptop (15") and 3 on my netbook.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #107) > (In reply to Paul [pwd] from comment #106) > > When the new tab page was designed, there was a lot of user research > > regarding why nine tiles was the best number. However this work has > > effectively gone against all of that. > > What do you mean? There are still 9 tiles by default. The change to use directory tiles altered this behavior, where we now show only as many tiles as will fit without reducing their size below some fixed dimensions. The addition of the search bar made this worse, as there is now less room for the tiles and they are more likely to disappear. I think Paul brings up a good point, in that people have gotten used to their 9 tiles, and now that we are not showing all 9, they are feeling a loss of functionality. Paul, can you file a new bug about this so we can discuss the issue and possible solutions there?

Verified as fixed using latest Aurora 31.0a2 20140519004014 under Win 7 64-bit, Ubuntu 32-bit and Mac OSX 10.7.5.

There should be an option to remove or turn off the search bar. Until then the newtab page is complete broken and I will never use it again. it may be enough excuse to simply remove firefox forever.

(In reply to shinobizx@yahoo.com from comment #111) > There should be an option to remove or turn off the search bar. Until then > the newtab page is complete broken and I will never use it again. it may be > enough excuse to simply remove firefox forever. Can you mention what's broken here ? If it's about not having 3 rows and columns, this change had nothing to do with it. Directory Tiles were responsible for this.

(In reply to Tim Nguyen [:ntim] from comment #112) > (In reply to shinobizx@yahoo.com from comment #111) > > There should be an option to remove or turn off the search bar. Until then > > the newtab page is complete broken and I will never use it again. it may be > > enough excuse to simply remove firefox forever. > > Can you mention what's broken here ? If it's about not having 3 rows and > columns, this change had nothing to do with it. Directory Tiles were > responsible for this. Yes, I can explain it. There was no search field in the new tab page. (preferred behavior) Now there is a search field in the new tab page. (not my preferred behavior) The correct solution is to provide an option in about:config to toggle the search bar from the new tab page when it is not useful or desired by the user. anything else?

(In reply to Tim Nguyen [:ntim] from comment #112) > (In reply to shinobizx@yahoo.com from comment #111) > > There should be an option to remove or turn off the search bar. Until then > > the newtab page is complete broken and I will never use it again. it may be > > enough excuse to simply remove firefox forever. > > Can you mention what's broken here ? If it's about not having 3 rows and > columns, this change had nothing to do with it. Directory Tiles were > responsible for this. That's disingenuous given that removing the search bar restores the third row of tabs,

Politely asking for a way (a pref in about:config for example) to turn of the search bar is more effective. Even more effective is to create a new bug for this, and link that to this bug. Also one can use Stylish to remove (hide) the search bar if you want to.

(In reply to Alfred Kayser from comment #115) > Also one can use Stylish to remove (hide) the search bar if you want to. FYI, stylish can't do it as the new tab page doesn't have a URL, otherwise it'd indeed be as simple as @namespace url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul); @-moz-document url("about:newtab") { #newtab-search-container { display: none !important; } }

(In reply to Alfred Kayser from comment #115) > Politely asking for a way (a pref in about:config for example) to turn of > the search bar is more effective. Even more effective is to create a new bug > for this, and link that to this bug. > > Also one can use Stylish to remove (hide) the search bar if you want to. as requested, see https://bugzilla.mozilla.org/show_bug.cgi?id=1026274 Thanks

(In reply to Paul [pwd] from comment #116) > (In reply to Alfred Kayser from comment #115) > > Also one can use Stylish to remove (hide) the search bar if you want to. > > > FYI, stylish can't do it as the new tab page doesn't have a URL, otherwise > it'd indeed be as simple as > @namespace > url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul); > > @-moz-document url("about:newtab") { > #newtab-search-container { > display: none !important; > } > } The code you provided works, it's just that the xul namespace needs to be removed (the page is coded in xhtml)

(In reply to Tim Nguyen [:ntim] from comment #118) > (In reply to Paul [pwd] from comment #116) > > (In reply to Alfred Kayser from comment #115) > > > Also one can use Stylish to remove (hide) the search bar if you want to. > > > > > > FYI, stylish can't do it as the new tab page doesn't have a URL, otherwise > > it'd indeed be as simple as > > @namespace > > url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul); > > > > @-moz-document url("about:newtab") { > > #newtab-search-container { > > display: none !important; > > } > > } > > The code you provided works, it's just that the xul namespace needs to be > removed (the page is coded in xhtml) Thank you kindly, I'm now back to making the most of the new tab page and restoring parity between the workflow on Android and desktop thanks to the following style changes: /*@namespace url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);*/ @-moz-document url("about:newtab") { #newtab-search-container, #newtab-margin-bottom, #newtab-margin-top, #newtab-margin-undo-container { display: none !important; } #newtab-vertical-margin { margin-top: 2.5em !important; } }