We will likely want some form of in-product "tour" of the new UI in bug 1088660. We will want this to be triggered when you use the UI, so it probably won't be doable as a UITour but rather something custom for the search bar.

Current look

This is going to require some style and XUL changes once we get final mockups but the mechanics of showing the highlight panels for the first N times (currently 10) all work here. For now we're punting on keyboard accessibility.

Comment on attachment 8525707 [details] [diff] [review] patch Review of attachment 8525707 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +3306,5 @@ > + } > + > + // If the highlight has already been show the appropriate number of times > + // do nothing. > + let count = 11;//Services.prefs.getIntPref(this.countPref); debugging?

Comment on attachment 8525707 [details] [diff] [review] patch Review of attachment 8525707 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, general comment: One thing I don't see here is an adjusted message for users who already had Yahoo has their default, or who are in locales where the default search provider is _not_ changing. Seems like we should still have this point out the shiny new UI, but we should avoid making them think something changed when it didn't, and may even want to be quite explicit/reassuring to those who will not see a search engine change. ::: browser/app/profile/firefox.js @@ +425,5 @@ > #endif > > +// How many times to show the new search highlight > +pref("browser.search.highlightCount", 10); > + That's a lot. But IIRC the intent was to be a little noisy about this change, so I suppose that's accomplished. ::: browser/base/content/browser.xul @@ +256,5 @@ > + <hbox class="SearchHighlightContent"> > + <description class="SearchHighlightText" flex="1">&SearchHighlight1.text;</description> > + </hbox> > + <hbox class="SearchHighlightFooter" align="center"> > + <label class="text-link" href="https://www.mozilla.org/">&SearchHighlightLink;</label> That's not a very descriptive "Learn More" link. Is there (web)content being created for this feature? Followup bug? @@ +277,5 @@ > + <hbox class="SearchHighlightContent"> > + <description class="SearchHighlightText" flex="1">&SearchHighlight2.text;</description> > + </hbox> > + <hbox class="SearchHighlightFooter" align="center"> > + <label class="text-link" href="https://www.mozilla.org/">&SearchHighlightLink;</label> Ditto. ::: browser/locales/en-US/chrome/browser/searchbar.dtd @@ +6,5 @@ > <!ENTITY searchEndCap.label "Search"> > + > +<!ENTITY SearchHighlight1.title "New search provider"> > +<!ENTITY SearchHighlight1.text "Firefox now lets you search easily on your favourite sites."> > +<!ENTITY SearchHighlight1.page "1/2"> Nit: I found myself wondering why there was a "one half" floating in the window. Make this "1 of 2" and "2 of 2"? (Or, really, since this is only a 2-part thing, maybe the numbers just add more confusion than they actually fix. If it was a longer multi-step thing that would seem more important...) @@ +9,5 @@ > +<!ENTITY SearchHighlight1.text "Firefox now lets you search easily on your favourite sites."> > +<!ENTITY SearchHighlight1.page "1/2"> > + > +<!ENTITY SearchHighlight2.title "New search provider"> > +<!ENTITY SearchHighlight2.text "Firefox now lets you search easily on your favourite sites."> SearchHighlight1.* and SearchHighlight2.* are the same... I hope this just placeholder text awaiting final copy?

(In reply to Justin Dolske [:Dolske] from comment #5) > Comment on attachment 8525707 [details] [diff] [review] > patch > > Review of attachment 8525707 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hmm, general comment: One thing I don't see here is an adjusted message for > users who already had Yahoo has their default, or who are in locales where > the default search provider is _not_ changing. Seems like we should still > have this point out the shiny new UI, but we should avoid making them think > something changed when it didn't, and may even want to be quite > explicit/reassuring to those who will not see a search engine change. Hoping all your comments will be addressed today. Really the patch here is to get the functionality working, the actual content of the panels is being decided imminently.

This implements the new mockups from UX with actual text. The panel shows the first five times the user opens the search popup, if they click through them they never show again.

Comment on attachment 8526373 [details] [diff] [review] patch rev 2 Review of attachment 8526373 [details] [diff] [review]: ----------------------------------------------------------------- For beta just move the strings to the xul file to not cause l10n changes

Landed on beta with hardcoded strings and one margin tweak from UX: https://hg.mozilla.org/releases/mozilla-beta/rev/98c4c4a55139

Comment on attachment 8526373 [details] [diff] [review] patch rev 2 Review of attachment 8526373 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/jar.mn @@ +24,5 @@ > * skin/classic/browser/browser-lightweightTheme.css > skin/classic/browser/click-to-play-warning-stripes.png > * skin/classic/browser/content-contextmenu.svg > + skin/classic/browser/dots.png (../shared/dots.png) > + skin/classic/browser/dots@2px.png (../shared/dots@2px.png) Err, the double-sized HiDPI images should have a "@2x" suffix, not "@2px". Harmless, but could you fix that when landing on nightly/aurora?

This was backed out due to test failures so still needs to land on beta

Still running this through try but I think this will solve the problems. We have to keep the panels hidden until they are first shown. This also fixes problems on windows and linux where the button steals focus from the search box. A couple of changes from UX. We don't show when the search bar is in the panel and we don't show if there is no text in the search bar. There is also a test included. It doesn't work on linux for some reason but we were willing to land without tests so I've just disabled it there for now. I'm also disabling the panels in all test suites by default to make sure we don't conflict with other tests.

This is the trunk patch, it's based on an older patch from bug 1088660 so might need tweaking when that finally lands. This splits out the strings into a DTD.

https://hg.mozilla.org/releases/mozilla-beta/rev/51c693c1ca3f disables the test everywhere as it turns out to be intermittent :(

Verified as fixed using: FF 43.RC1 Build Id:20141125180439 OS: Win7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5 I found an issue on windows that seems to be related with the suggestion box bug 1105286 .

For 36: https://hg.mozilla.org/integration/fx-team/rev/edb694f470b8 Included the updated strings from bug 1104114

Has this been tested with much longer strings? Is enough to change the value of browser.search.highlightCount to test it?

I don't think we tested it with much longer strings but the popup should auto-adjust to the strings. Yeah, changing that pref is all you need to test it. Just set it to any value greater than 0 and start typing in the searchbar.

I managed to translate these strings before today's build, so far so good (my strings' lenght is almost double compared to English).

Verified as fixed using: FF 35 Build id: 20141218174327 OS: Mac Os X 10.9.5, Ubuntu 14.04 x64, Win 7 x64

Verified as fixed using the following environment: FF 36.0b9 Build Id: 20150212154903 OS: Win 8.1 x86, Ubuntu 12.04 x86, Mac Os X 10.9.5