950698 - Change the text for the tip for adding more search providers from Settings->Customize->Search settings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

Since "Bug 768667 - Use the action bar for text selection on ICS+" changed the way the user interacts with text fields, and the user can not long tap on the text field to add the search engine, we should change the text in Settings->Customize->Search settings to:

Expected: "TIP: Add any website to your list of search providers by tapping twice to open the action bar for text selection, and tap on the magnifying icon."

Actual: "TIP: Add any website to your list of search providers by long-pressing on its search field."

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Ian - What do we want to do with this?

Comment 2

[Ian Barlow (:ibarlow)]

I believe you still have to long press on the field to get into action mode. I would phrase it as:

TIP: Add any website to your list of search providers by long pressing on its search field, and then tapping the [add search engine icon] icon.

--

Mockup: http://cl.ly/image/310z1Y3l3R3d

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like this is worth tracking to see the wording correct on the first ship with this feature - will need to make sure this is run past l10n though since we are already in Aurora for FF28.

Comment 4

[Jeff Beatty [:gueroJeff]]

I think it's too late to make this change in 28. Some of our locales have already completed sign-offs for their work in 28 and may not catch this change to update it before the merge day.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Given that, we'll have to tolerate the mismatch for 28 and get this in 29.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Unless we have a breakdown of our Fennec users by locale and can at least confirm the possibility of updating this for our top locales -  that might be a good in between solution so that most of our users get the correct info.  However, it's a tooltip and not a prominent text so I'm not sure we need to panic about getting this into Aurora and breaking string freeze for it.

Comment 7

[Erin Lancaster [:elan]]

We missed merge for Fx29, Can we land this for Fx30, please?

Comment 8

[Aaron Train [:aaronmt]]

There's no assignee and no patch.

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Screenshot: New search string

This became much more complicated than I expected!

I think we may need another image as well, because the current long-tap one doesn't really match the text anymore.

(Need to split the patches, but they are forthcoming!)

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Update new search tip.

This became complicated because AFACT, there isn't a way to get to the view inside of the custom layout of a Preference without overriding the onBindView method of Preference, which is necessary to replace the search icon in the text. This patch is something that could be super-classed at some point, if it needs to be reused.

This annoying hack is another point in favor of ripping out some of the current Preferences code...

This will need aurora uplift and late-l10n.

I'll be on PTO next week, so if someone can shepherd this bug through landing and uplift, it would be much appreciated!

Comment 11

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [PTO until 2/18] [:liuche] from comment #9)
> Created attachment 8372723 [details]
> Screenshot: New search string
> 
> This became much more complicated than I expected!
> 
> I think we may need another image as well, because the current long-tap one
> doesn't really match the text anymore.
> 
> (Need to split the patches, but they are forthcoming!)

I think the image is fine, to be honest. The icon should be about 20% bigger though.

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 8372974 [details] [diff] [review]
Update new search tip.

Review of attachment 8372974 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +80,5 @@
>       advisory message on the customise search providers settings page explaining how to add new
> +     search providers.
> +     The "%I" in the string will be replaced by a small image of the icon described, and can be moved to wherever
> +     it is applicable. Please keep the spacing around the "%I" string. -->
> +<!ENTITY pref_search_hint "TIP: Add any website to your list of search providers by long-pressing on its search field and then tapping the &#37;I icon.">

&#37;I should be factored out into a 'formatI' constant here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/strings.xml.in#12

The reason is that localizers are used to variables being formatted as "&X;", so they may accidentally remove any trailing characters (the I in this case). There's no validation that checks that they used the variable correctly, and we find only after we start seeing crashes (full history in bug 775142).

::: mobile/android/base/preferences/ModifiableHintPreference.java
@@ +19,5 @@
> +import android.widget.TextView;
> +
> +class ModifiableHintPreference extends Preference {
> +    private static final String LOGTAG = "ModifiableHintPref";
> +    private Context mContext;

Nit: final

@@ +36,5 @@
> +        mContext = context;
> +    }
> +
> +    @Override
> +    protected void onBindView(View view) {

IIRC, onBindView is called many times when prefs are created. I'd override onCreateView instead.

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Part 0: Use formatted string for image

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Part 1: Update search tip

Called in onCreateView now.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Screenshot: New search string [larger]

Comment 16

[Chenxia Liu [:liuche]]

Attachment: Part 0: Use formatted string for image

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 8378733 [details] [diff] [review]
Part 1: Update search tip

Review of attachment 8378733 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/preferences/ModifiableHintPreference.java
@@ +45,5 @@
> +        return thisView;
> +    }
> +
> +    private void configurePreferenceView(View view) {
> +        Log.e("AOEU", "configurePreferenceView called");

Drop logging

@@ +50,5 @@
> +        TextView textView = (TextView) view.findViewById(RESID_TEXT_VIEW);
> +        String searchHint = textView.getText().toString();
> +
> +        // Use an ImageSpan to include the "add search" icon in the Tip.
> +        int imageSpanIndex = searchHint.indexOf("%I");

Might want to factor "%I" into a String constant...

@@ +61,5 @@
> +            ImageSpan searchIcon = new ImageSpan(drawable);
> +            final SpannableStringBuilder hintBuilder = new SpannableStringBuilder(searchHint);
> +
> +            // Insert the image.
> +            hintBuilder.setSpan(searchIcon, imageSpanIndex, imageSpanIndex + 2, Spanned.SPAN_INCLUSIVE_INCLUSIVE);

...and if you do, use length() here instead of a hardcoded 2.

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Part 0: Use formatted string for image v2

Lift "%I" into a constant. Carrying over r+.

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 0: Use formatted string for image v2

(qref)

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Part 1: Update search tip v2

Addressed review comments, carrying over r+.

Comment 21

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: Update search tip

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 768667 changed the way to add search engines
User impact if declined: Hint to add search engine won't match actual pathway 
Testing completed (on m-c, etc.): Local testing
Risk to taking this patch (and alternatives if risky): very low, string changes
String or IDL/UUID changes made by this patch: One string change, late-l10n

Comment 22

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/427496381a7d
https://hg.mozilla.org/integration/fx-team/rev/88e90c4e3780

Comment 23

[Sylvestre Ledru [:Sylvestre]]

I will wait for this patch to land in m-c before uplifting it

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/427496381a7d
https://hg.mozilla.org/mozilla-central/rev/88e90c4e3780

Comment 25

[u421692]

Verified as fixed on Nightly 30.0a1(2014-02-23).

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ed9ed99b763

Comment 27

[Chenxia Liu [:liuche]]

Is there anything I need to do for this as late-l10n?

Comment 28

[Jeff Beatty [:gueroJeff]]

(In reply to Chenxia Liu [:liuche] from comment #27)
> Is there anything I need to do for this as late-l10n?

What's the thinking for getting this into 29 instead of 30, as Erin suggested in comment 7? If this were within the first two weeks of the cycle, I wouldn't be so reluctant, but seeing as we're now halfway through, I'm more reluctant.

Comment 29

[Jeff Beatty [:gueroJeff]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #26)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/1ed9ed99b763

Can we back this out? I'd like to know the approval process here, cause I don't think any of the l10n drivers were asked about the impact for landing this at this point of the cycle.

Comment 30

[Chenxia Liu [:liuche]]

Backed out of aurora, and clearing the late-l10n keyword. Sorry for the confusion here, I thought late-l10n meant this was good to uplift, my mistake.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2180eeda96b

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Going out on a limb that means this is wontfix for 28/29.