1025633 - Word suggestion strings appear in the word suggestion space

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect)

Description

[Alexandre LISSY :gerard-majax]

Since a couple of days, I noticed that on master build with multilocate, there is a « Suggestions de mots » string that appears, barely visible, in the word suggestion area of the keyboard.

This reproduces on my multilocale builds for Nexus S, not on my non-multilocale builds for Flame. I also have the bug reproduced on my Keon (multilocale builds from Geeksphone).

This also reproduces if I switch the locale (and the string is localized).

STR:
 0. Open Messages app
 1. Tap new message
 2. Tap in the input field

Expected:
 The word suggestion area is clean

Actual:
 The string "Suggestions de mots" is occupying space in the word suggestion area, hiding half of the suggested string in height. The resulting word suggestions are barely usable.

Comment 1

[Alexandre LISSY :gerard-majax]

Attachment: Proper suggestion area on Flame, non multilocale build

Comment 2

[Alexandre LISSY :gerard-majax]

Attachment: Broken suggestion area on Keon, multilocale build

Comment 3

[Alexandre LISSY :gerard-majax]

Attachment: Broken suggestion area on Nexus S, multilocale build

Comment 4

[Alexandre LISSY :gerard-majax]

FYI, this string is only in the gaia-l10n repo:

> alex@portable-alex:~/codaz/B2G/gaia$ git grep 'Suggestions de mots'
> alex@portable-alex:~/codaz/B2G/gaia$ 
> alex@portable-alex:~/codaz/B2G/gaia/locales/fr$ git grep 'Suggestions de mots'
> apps/keyboard/keyboard.properties:wordSuggestions=Suggestions de mots

Of course, all repos are uptodate.

Comment 5

[Alexandre LISSY :gerard-majax]

Both word suggestion and dismiss string are added by https://github.com/mozilla-b2g/gaia/blob/dfc4703bb81d1fa4f2087a1a6124b47a80a5d1de/apps/keyboard/js/render.js#L891

Comment 6

[Alexandre LISSY :gerard-majax]

This has been modified by bug 1013207.

Comment 7

[Alexandre LISSY :gerard-majax]

So after checking the bug who introduced this, I noticed it modified the l10n:
> diff --git a/apps/keyboard/locales/keyboard.en-US.properties b/apps/keyboard/locales/keyboard.en-US.properties
> index f1937b6..3c579d2 100644
> --- a/apps/keyboard/locales/keyboard.en-US.properties
> +++ b/apps/keyboard/locales/keyboard.en-US.properties
> @@ -12,26 +12,26 @@ wordSuggestion=Word suggestion
>  # LOCALIZATION NOTE (wordSuggestions): The name of the section above the
>  # keyboard where the word suggestions appear. This is read out to screen reader
>  # users when they put a finger in that area.
> -wordSuggestions=Word suggestions
> +wordSuggestions.ariaLabel=Word suggestions
>  # LOCALIZATION NOTE (dismiss): The "x" button that closes the word suggestions.
> -dismiss=Dismiss
> +dismiss.ariaLabel=Dismiss

So, théo/delphine, seems you have work to do :)

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ouch, I didn't realized I will introduce this kind of side effect.

Should I instead update the l10n IDs (e.g. dismiss -> dismiss2.ariaLebel)?

Comment 9

[Jason Smith [:jsmith]]

QA Wanted to check 2.0.

Comment 10

[Théo Chevalier [:tchevalier]]

2.0 is not affected. So it's safe to update the string entity.

Gaia      0f254c92bc44d614ae56a855f18a895a7e4703ad
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/301e62e44f18
BuildID   20140616160201
Version   32.0a2
ro.build.version.incremental=eng.theo.20140613.174651
ro.build.date=vendredi 13 juin 2014, 17:47:02 (UTC-0700)

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #8)
> Ouch, I didn't realized I will introduce this kind of side effect.
> 
> Should I instead update the l10n IDs (e.g. dismiss -> dismiss2.ariaLebel)?

I don't think so. We just need to update translations and the effect will disappear. We will not push to production any locale that doesn't have the right strings.

Theo, am I right?

Comment 12

[Théo Chevalier [:tchevalier]]

(In reply to Zibi Braniecki [:gandalf] from comment #11)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #8)
> > Ouch, I didn't realized I will introduce this kind of side effect.
> > 
> > Should I instead update the l10n IDs (e.g. dismiss -> dismiss2.ariaLebel)?
> 
> I don't think so. We just need to update translations and the effect will
> disappear. We will not push to production any locale that doesn't have the
> right strings.
> 
> Theo, am I right?

I'm not sure to understand the root cause of the issue. Is it happening because locales still have "dismiss=Dismiss" in their repo?
If yes, we can't remove it manually for all locales. Especially because we don't have any 2.1 repo yet.

If dismiss.ariaLebel becomes dismiss2.ariaLebel, I guess it should stop using translations from "dismiss"?

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Théo Chevalier [:tchevalier] from comment #12)
> I'm not sure to understand the root cause of the issue. Is it happening
> because locales still have "dismiss=Dismiss" in their repo?
> If yes, we can't remove it manually for all locales. Especially because we
> don't have any 2.1 repo yet.

Yes, it seems to be the cause.

> If dismiss.ariaLebel becomes dismiss2.ariaLebel, I guess it should stop
> using translations from "dismiss"?

Well, yeah, but that seems like an unnecessary inflation of string ID changes. We are capable of catching this without having to fall back to inflating string ID versions.

Comment 14

[Théo Chevalier [:tchevalier]]

(In reply to Zibi Braniecki [:gandalf] from comment #13)
> (In reply to Théo Chevalier [:tchevalier] from comment #12)
> > I'm not sure to understand the root cause of the issue. Is it happening
> > because locales still have "dismiss=Dismiss" in their repo?
> > If yes, we can't remove it manually for all locales. Especially because we
> > don't have any 2.1 repo yet.
> 
> Yes, it seems to be the cause.
> 
> > If dismiss.ariaLebel becomes dismiss2.ariaLebel, I guess it should stop
> > using translations from "dismiss"?
> 
> Well, yeah, but that seems like an unnecessary inflation of string ID
> changes. We are capable of catching this without having to fall back to
> inflating string ID versions.

oh, if you can tell the App to ignore this string it's way better, yes.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Théo Chevalier [:tchevalier] from comment #14)
> oh, if you can tell the App to ignore this string it's way better, yes.

I can't. But we can make sure we transition all locales away from string's value to string's attribute `ariaLabel`, right?

Comment 16

[Francesco Lodolo [:flod]]

(In reply to Zibi Braniecki [:gandalf] from comment #11)
> I don't think so. We just need to update translations and the effect will
> disappear. We will not push to production any locale that doesn't have the
> right strings.

That's not necessarily true. A build is green if it has all the needed strings, it doesn't count "extra strings" (obsolete). And most important, this create issues on testing while the locale is not yet complete (it's going to take a while before we branch for 2.0 on gaia-l10n).

I think the solution is just to assign a new data-l10n-id to the element.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

oh, ok. Whatever you think is necessary. I was hoping to save us from having to update the string ID.

Comment 18

[Axel Hecht [:Pike]]

Zibi, never assume that localizations are correct, that's not going to work. All bugs in localizations are OK and need to be dealt with in the expected manner by the code that's calling in to them.

Comment 19

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#20728

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8442656 [details] [review]
mozilla-b2g:master PR#20728

Update the l10n IDs

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8442656 [details] [review]
mozilla-b2g:master PR#20728

ok, r=me if this is what is needed to get this fixed.
Thanks.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8442656 [details] [review]
mozilla-b2g:master PR#20728

Let me make sure if this is indeed the conclusion of the discussion above.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8442656 [details] [review]
mozilla-b2g:master PR#20728

yeah. Until we can figure out how to make our toolchain smarter we need to inflate the ids.

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/99cfbe40f8f821a39ff50333e74dc922aa429626

Comment 25

[bhavana bajaj [:bajaj]]

Switching the 2.1?->2.1+, on these fixed bugs as these are regression.

Nothing to land here, its just flag-cleanup of 2.1? list. Please Ni me if there is confusion/disagreement.

Comment 26

[SandKing]

Hi Josh,
This issue has been verified successfully on Flame2.1
See video: verify_1025633.MP4

Flame2.1 build:
Gaia-Rev        8ae086c39011bc8842b2a19bb5267906fa22345a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebbd5c65c3c1
Build-ID        20141124094013
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141124.130744
FW-Date         Mon Nov 24 13:07:55 EST 2014
Bootloader      L1TC00011880

Comment 27

[SandKing]

Attachment: Verify video: verify_1025633.MP4.mp4

Comment 28

[Annabelle]

From comment 26&27,so change the Status to "verified".