1053641 - [b2g][2.0] "call forwarding status" page string format has problem.

Status: VERIFIED FIXED

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

Description

[Peipei Cheng (needinfo if you need my action)]

Attachment: 2014-08-13-09-21-30.png

1. Tap on the "Dialer" icon to open the dialer app.

2. Tap on the "dial pad" icon that is located at the bottom right corner of the screen.

3. Type in ""*#21#"", tap "Dial" icon and check all the strings on the "Call Forwarding" screen.

--> You will see the format of this page is wrong. It seems the page does not recongnize '\n' as a new line. Please see attached screenshot.

Comment 1

[Peipei Cheng (needinfo if you need my action)]

V1.4 does not have this problem.

Comment 2

[Jason Smith [:jsmith]]

Peipei - Do you think this is a blocking issue for 2.0 from your perspective?

Note - we'll usually block on non-minor regressions, so if you feel this is non-minor, then feel free to nominate the bug to block.

Comment 3

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi all,

The code related to callforwarding in mmi.js and mmi_ui.js didn't change much.
I guess this is because l10n change its behavior between v1.4 and v2.0. 
In v1.4 it treats '\n' as a new line but in v2.0 and later, it treats '\n' as plain '\n' literal.

I scanned through all locale files and found out only dialer and system use '\n' in their locale files.
(Check keyword 'low-disk-space' In system app)
So I guess this behavior change breaks system app too.

Comment 4

[Peipei Cheng (needinfo if you need my action)]

I searched for all locale strings that ended by '\n', and only found 10 strings [1]. 
9 of these strings are on "Call Forwarding" screen as attached screenshot. And the other one is 'low-disk-space' in system app as mentioned by dwi2.

So the user impact should be low. This should not be a blocker for v2.0.


[1] localized strings
----------------------------------------------------------------
Asynchronous: {{ async }}\n	
Data: {{ data }}\n	
FAX: {{ fax }}\n	
Packet: {{ packet }}\n	
PAD: {{ pad }}\n	
SMS: {{ sms }}\n	
Call forwarding status\n	
Synchronous: {{ sync }}\n	
Voice: {{ voice }}\n	
Device space low.\n

Comment 5

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Although we could fix this quickly with a simple line like:
    msg.replace(/\\n/g, '\n');

However, I would rather fix it in a more proper way. Because:
1. l10n owner suggest that we should move away l10n.get and instead use l10n DOM attribute.
2. It is a little bit weird to put '\n' in locale files since it mix layout and localized text.
3. It is not a blocker and has low user impact.

I'd love to take this job if no one object to it. But I think I might fix it in next version other than 2.1.

Comment 7

[Gabriele Svelto [:gsvelto]]

Tzu-Lin, are are you actively working on this? If you're not do you mind if I take it, I should have the time to write a fix today.

Comment 8

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Gabriele,

I am currently working on something else so please take it.

Thanks a lot.

Comment 9

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Remove newlines from localized strings describing call waiting status

This patch removes the newlines from the localized strings and instead uses them in the code assembling the message. Note that the container used for this message has the CSS white-space: pre-line set so its |textContent| field will honor the newlines in the string as line breaks.

Comment 10

[Francesco Lodolo [:flod]]

Gabriele, sorry but you need new string IDs at this point to be sure people update their current translations.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings

Comment 11

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8488659 [details] [diff] [review]
[PATCH] Remove newlines from localized strings describing call waiting status

Francesco beat me to it.

He also pointed out that this would be late-l10n at this point, so I don't think we will be able to uplift this to 2.0, and I don't think it's worth pushing to uplift it to 2.1.

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Remove newlines from localized strings describing call forwarding status

I wasn't aware of the need to change the identifiers, thanks for pointing this out. The previous patch's comment was also wrong, this is about call forwarding, not call waiting.

Here's a revised patch with new identifiers for all strings. Since they were previously abbreviated to cf-* I've changed them all into call-forwarding-* for extra clarity.

I'm also fine with not uplifting this especially as I cannot seem to reproduce the issue in master, it was probably due to a change in the l10n code which was eating the '\n' characters but was later fixed. I'll give it a spin on 2.0 and 2.1 to see if it reproduces but I doubt for the latter.

Comment 13

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8488669 [details] [diff] [review]
[PATCH v2] Remove newlines from localized strings describing call forwarding status

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

Thanks, looks good now. Just one nit.

::: apps/communications/dialer/js/mmi.js
@@ +141,5 @@
> +        this._('call-forwarding-sms', {sms: sms || inactive }),
> +        this._('call-forwarding-sync', {sync: sync || inactive }),
> +        this._('call-forwarding-async', {async: async || inactive }),
> +        this._('call-forwarding-packet', {packet: packet || inactive }),
> +        this._('call-forwarding-pad', {pad: pad || inactive })

The spacing in each of these is off now.
```{pad: pad || inactive}```

Comment 14

[Francesco Lodolo [:flod]]

It's not really relevant, but for the future I'd completely ignore the non en-US .properties files (they should be gone soon, bug 1011519)

Comment 15

[Gabriele Svelto [:gsvelto]]

Thanks for the quick review!

(In reply to Doug Sherk (:drs) from comment #13)
> ::: apps/communications/dialer/js/mmi.js
> @@ +141,5 @@
> > +        this._('call-forwarding-sms', {sms: sms || inactive }),
> > +        this._('call-forwarding-sync', {sync: sync || inactive }),
> > +        this._('call-forwarding-async', {async: async || inactive }),
> > +        this._('call-forwarding-packet', {packet: packet || inactive }),
> > +        this._('call-forwarding-pad', {pad: pad || inactive })
> 
> The spacing in each of these is off now.
> ```{pad: pad || inactive}```

OK, I'll adjust before landing.

(In reply to Francesco Lodolo [:flod] from comment #14)
> It's not really relevant, but for the future I'd completely ignore the non
> en-US .properties files (they should be gone soon, bug 1011519)

OK, thanks for the tip.

Just an additional note regarding testing: I've just tested this on v2.0 and it does reproduce there. It doesn't on master so I'd say that v2.2 is unaffected. I'm now testing v2.1, I'll adjust the flags accordingly.

Comment 16

[Gabriele Svelto [:gsvelto]]

OK, this reproduces on v2.1 too. Somehow the visual issue with the newlines being visible doesn't happen on master though.

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Remove newlines from localized strings describing call forwarding status

Here's the PR for the try run, the nit is fixed too.

Comment 18

[Doug Sherk (:drs) (inactive)]

I noticed that the tests for this were disabled [1], but the bug that was needed to be fixed for them to be re-enabled has been resolved. [2] [3] Gabriele is going to look into re-enabling them. If it's not doable, then we'll just go ahead with this as-is.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/mmi_test.js#L763
[2] bug 884343
[3] bug 879680

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Remove newlines from localized strings describing call forwarding status

Now with unit-tests re-enabled; I've also updated the PR.

Comment 20

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8489089 [details] [diff] [review]
[PATCH v3] Remove newlines from localized strings describing call forwarding status

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

LGTM, thanks for the cleanup and re-enabling the tests.

Comment 21

[Gabriele Svelto [:gsvelto]]

Thanks for the review! I've got a broken integration test in my try run, I've re-triggered it and will wait until it turns green before landing.

Comment 22

[Doug Sherk (:drs) (inactive)]

This needs a demo. Please add one to https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S5#Demos
Thanks!

Comment 23

[Gabriele Svelto [:gsvelto]]

Try finally turned green: https://tbpl.mozilla.org/?rev=fc39bef7782955a141b2b964bf05503faf34ef37&tree=Gaia-Try

Pushed to gaia/master c451052868e71c932311dae95d99857c5b368f49

https://github.com/mozilla-b2g/gaia/commit/c451052868e71c932311dae95d99857c5b368f49

I've also added screenshots of the bug before and after the fix to the demo section in the sprint page.

Comment 26

[Gabriele Svelto [:gsvelto]]

[Blocking Requested - why for this release]: Transferring the nomination from bug 1093160

Comment 27

[Francesco Lodolo [:flod]]

Note: this PR has strings, 2.0 has been string frozen for months.

Comment 28

[Gabriele Svelto [:gsvelto]]

I could do a no-new-strings version of this patch if need be. It would be a hack (removing the '\n' after having retrieved the string) but it would do the job.

Comment 29

[Beatriz Rodríguez [:brg]]

We need it fixed in v2.0 as this is a regression.

Comment 30

[Wayne Chang [:wchang]]

Triage- the screenshot in comment 0 is pretty confusing and an end user may have trouble interpreting it. Given this may involve call costs (undesired forwarding or missed calls) - we should block on this.

Gabriele,

Would be great if you could do this no-new-strings patch you mentioned.

Comment 31

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8489089 [details] [diff] [review]
[PATCH v3] Remove newlines from localized strings describing call forwarding status

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug 793192
[User impact] if declined: The glitches in attachment 8472814 [details] will be visible
[Testing completed]: This has been on master for a while, the patch also ships with unit-tests to enforce consistent message responses
[Risk to taking this patch] (and alternatives if risky): Risks are minimal, only formatting of the call forwarding status was changed, since we're taking this for 2.0 I don't see why not taking it for 2.1 too
[String changes made]: In apps/communications/dialer/locales/dialer.en-US.properties strings beginning with cf- were replaced with identical ones beginning with call-forwarding- and without the trailing newline

Comment 32

[Francesco Lodolo [:flod]]

(In reply to Gabriele Svelto [:gsvelto] from comment #31)
> I don't see why not taking it for 2.1 too

Same reason of 2.0, 2.1 has been string frozen for a while. While hacky, the replacement approach should be used on both branches.

Comment 33

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH gaia/v2.0] Uplifted patch with new strings removed

Here's a cherry-picked patch w/o new strings. I've also removed the unit-tests we had re-enabled in the original patch as they relied on some significant changes to the sendMMI() stub that we landed in other patches in order to work correctly.

Asking for review again in case I might have missed something. This was tested on my device.

Comment 34

[Gabriele Svelto [:gsvelto]]

(In reply to Francesco Lodolo [:flod] from comment #32)
> Same reason of 2.0, 2.1 has been string frozen for a while. While hacky, the
> replacement approach should be used on both branches.

OK, I'll check if my no-strings patch applies to 2.1 too.

Comment 35

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH gaia/v2.1] Uplifted patch with new strings removed

Same technique as in attachment 8516594 [details] [diff] [review] of stripping the original strings but with a more straightforward uplift since the unit-tests could be preserved with minimal changes this time.

Comment 36

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8516594 [details] [diff] [review]
[PATCH gaia/v2.0] Uplifted patch with new strings removed

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

I'm arbitrarily setting r- for what I noticed below.

::: shared/test/unit/mocks/mock_navigator_moz_mobile_connections.js
@@ +72,5 @@
> +      ICC_SERVICE_CLASS_DATA_SYNC: (1 << 4),
> +      ICC_SERVICE_CLASS_DATA_ASYNC: (1 << 5),
> +      ICC_SERVICE_CLASS_PACKET: (1 << 6),
> +      ICC_SERVICE_CLASS_PAD: (1 << 7),
> +      ICC_SERVICE_CLASS_MAX: (1 << 7),

These don't seem to be needed.

Comment 37

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH gaia/v2.0 v2] Uplifted patch with new strings removed

Same as before minus the useless constant declarations.

Comment 38

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8516620 [details] [diff] [review]
[PATCH gaia/v2.1] Uplifted patch with new strings removed

If I've understood the landing procedures correctly we need approval for both 2.1 and 2.0 before landing even though this is 2.0+, so here's my requests.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): see comment 29 and comment 30
[User impact] if declined: The call forwarding status display is hard/impossible to read
[Testing completed]: This has been in master for a while where it works fine and it comes with unit-tests too
[Risk to taking this patch] (and alternatives if risky): Practically no risks are involved and the alternative is to keep the confusing '\n' symbols in the display
[String changes made]: None

Comment 39

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8516961 [details] [diff] [review]
[PATCH gaia/v2.0 v2] Uplifted patch with new strings removed

See comment 38.

Comment 40

[bhavana bajaj [:bajaj]]

Comment on attachment 8516620 [details] [diff] [review]
[PATCH gaia/v2.1] Uplifted patch with new strings removed

Lets verify this on branches once it lands.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Please post pull request links.

Comment 42

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST gaia/v2.0] Uplifted patch with new strings removed

Comment 43

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST gaia/v2.1] Uplifted patch with new strings removed

Here's both patches integrated in two pull requests.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Thanks :)

v2.1: https://github.com/mozilla-b2g/gaia/commit/6295f6acfe91c6ae659712747dd2b9c8f51d0339
v2.0: https://github.com/mozilla-b2g/gaia/commit/d3e4da377ee448f9c25f908159480e867dfb13f3

Comment 45

[Beatriz Rodríguez [:brg]]

Attachment: Screenshot Flame v2.1

Verified with Flame v2.1 Gecko-7fede3f.Gaia-6295f6a

Comment 46

[Beatriz Rodríguez [:brg]]

Attachment: Screenshot Flame v2.0

Tested with Flame v2.0 Gecko-05f9775.Gaia-d3e4da3. The result is not the same as v2.1 but I guess it will pass.

Comment 48

[Gabriele Svelto [:gsvelto]]

(In reply to Beatriz Rodríguez [:brg] from comment #46)
> Tested with Flame v2.0 Gecko-05f9775.Gaia-d3e4da3. The result is not the
> same as v2.1 but I guess it will pass.

That is really strange, I wonder how that's possible at all :-/ Where you using the tip of https://hg.mozilla.org/gaia-l10n/es/ for localization?

Comment 49

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.0, Flame 2.1, and Flame 2.2

Actual Results: /n's not visible on call forwarding status page. Line breaks are displayed as expected.

Device: Flame 2.0(319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141110000204
Gaia: d3e4da377ee448f9c25f908159480e867dfb13f3
Gecko: 7198906837e7
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141110001201
Gaia: 0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko: 97487a2d1ee6
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141110040206
Gaia: 5f8206bab97cdd7b547cc2c8953cadb2a80a7e11
Gecko: d380166816dd
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Comment 50

[Coler]

Verify passed, this issue can't be repro on Woodduck 2.0.
Attached: Verify_Woodduck_CallForwarding.mp4
Reproducing rate: 0/5
Gaia-Rev        ee5cf148b4c546beea9bfb799d2a3ee62074957d
Gecko-Rev       73601b71861cbc2f180c4d2653cec3e9fbb39db5
Build-ID        20141114050313
Version         32.0

Comment 51

[Coler]

Attachment: Verify_Woodduck_CallForwarding.mp4

Comment 52

[xiupinglong]

Dears,

The string id "low-device-storage" in system app also has "\n" when display "Device space low."

See in /apps/system/locales/system.en-US.properties
# Low device storage
low-device-storage=Device space low.\n

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Sounds like a question for the person who fixed the bug, not me...

Comment 54

[Gabriele Svelto [:gsvelto]]

(In reply to xiupinglong from comment #52)
> The string id "low-device-storage" in system app also has "\n" when display
> "Device space low."
> 
> See in /apps/system/locales/system.en-US.properties
> # Low device storage
> low-device-storage=Device space low.\n

Good catch, let's file another bug for fixing that.