Closed Bug 1101654 Opened 10 years ago Closed 9 years ago

first-use tour for new one-off search UI

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 + verified
firefox35 + verified
firefox36 + verified

People

(Reporter: Gavin, Assigned: mossop)

References

Details

Attachments

(6 files, 1 obsolete file)

Attached image mock-up
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.
Assignee: nobody → dtownsend+bugmail
Group: mozilla-employee-confidential
Depends on: fx34-searchui
Attached image screenshot
Current look
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8525707 - Flags: review?(felipc)
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?
Attachment #8525707 - Flags: review?(felipc) → review+
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.
Attached patch patch rev 2Splinter Review
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.
Attachment #8525707 - Attachment is obsolete: true
Attachment #8526373 - Flags: review?(felipc)
Attachment #8526373 - Flags: review?(dolske)
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
Attachment #8526373 - Flags: review?(felipc) → review+
Landed on beta with hardcoded strings and one margin tweak from UX: https://hg.mozilla.org/releases/mozilla-beta/rev/98c4c4a55139
Attachment #8526373 - Flags: review?(dolske)
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
Attached patch fixup patchSplinter Review
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.
Attachment #8526954 - Flags: review?(felipc)
Attachment #8526954 - Flags: review?(felipc) → review+
Attached patch trunk patchSplinter Review
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.
Attachment #8527057 - Flags: review+
https://hg.mozilla.org/releases/mozilla-beta/rev/51c693c1ca3f disables the test everywhere as it turns out to be intermittent :(
Flags: qe-verify+
QA Contact: catalin.varga
Depends on: 1105286
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 .
No longer depends on: 1105286
https://hg.mozilla.org/mozilla-central/rev/edb694f470b8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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).
Depends on: 1105286
Verified as fixed using:

FF 35
Build id: 20141218174327
OS: Mac Os X 10.9.5, Ubuntu 14.04 x64, Win 7 x64
Depends on: 1124694
Depends on: 1127922
Depends on: 1130480
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.