1068860 - [CBS] Cell Broadcast dialogues are empty

Status: RESOLVED FIXED

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

Description

[Michael Schwartz [:m4]]

[Blocking Requested - why for this release]:

Steps to reproduce:

# Inject cell broadcast message

Expected: Dialogue showing the broadcast message
Observed: Dialogue is shown with an empty title and empty body

Quick debugging shows that the http://mxr.mozilla.org/gaia/source/apps/system/js/carrier_info_notifier.js#28 is receiving the correct title and body but the prompt isn't displaying it correctly.  Seems like a regression due to the recent system notification changes.

Comment 1

[Gregor Wagner [:gwagner]]

Tim, can you take a look here?

Comment 2

[Gregor Wagner [:gwagner]]

(In reply to Michael Schwartz [:m4] from comment #0)
> Seems like a regression due to the recent system
> notification changes.

m4: Can you be more specific? here
Mike: are you aware of any changes from our side?

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

There have been quite a few changes in the notifications ui recently. But I am confused as to why those changes would cause this issue. From comment 0, it seems like the phone is unlocked when injecting the cell broadcast message, and that the modal dialog does display but just with empty title and body. If that is the case, this code path does not use the notification code [1].

My guess is that this is a regression in modal dialog rather than notifications. Perhaps [2]?

1.) https://github.com/mozilla-b2g/gaia/blob/d37950eb09e28aa18d0e01df9ff90574bd4337e0/apps/system/js/carrier_info_notifier.js#L37

2.) https://github.com/mozilla-b2g/gaia/commit/f65332a3c3bd036bf20f3ab1d383518810ce1eb7

Comment 4

[Michael Schwartz [:m4]]

Sorry I didn't intend to use any defined terms.  Thanks Michael for chiming in.

Comment 5

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

Maybe a branch check first to find the regressed bug?

Comment 6

[Jason Smith [:jsmith]]

(In reply to Tim Guan-tin Chien [:timdream] (OOO ~Sep 19) (MoCo-TPE) (please ni?) from comment #5)
> Maybe a branch check first to find the regressed bug?

Can't - cell broadcast is a feature we can't test on the Moz QA side, since we don't have the tools to be able to do that.

Comment 7

[Gregor Wagner [:gwagner]]

Regression

Comment 8

[Gregor Wagner [:gwagner]]

Alex, can you take a look?

Comment 9

[:gerard-majax]

Sure.

Comment 10

[:gerard-majax]

Can you provide a screenshot and more informations on how to reproduce ?

Comment 11

[:gerard-majax]

I confirm Michael's fear, it looks like it is because of bug 1029944

Comment 13

[:gerard-majax]

The message we need to display is sent from the network and depending on the type of the payload:
 - http://hg.mozilla.org/mozilla-central/annotate/c8e325eee9e1/dom/system/gonk/ril_worker.js#l10190
 - http://hg.mozilla.org/mozilla-central/annotate/c8e325eee9e1/dom/system/gonk/ril_worker.js#l10245

Comment 14

[yifan [:yifan][:yliao]]

see https://bugzilla.mozilla.org/show_bug.cgi?id=1058795#c14

Comment 15

[:gerard-majax]

(In reply to Yi-Fan Liao [:yifan][:yliao] from comment #14)
> see https://bugzilla.mozilla.org/show_bug.cgi?id=1058795#c14

That does the trick.

Comment 16

[:gerard-majax]

Attachment: Gaia PR

Comment 17

[:gerard-majax]

Comment on attachment 8492128 [details] [review]
Gaia PR

Gandalf, would you please review this?

Comment 18

[:gerard-majax]

Gaia-Try is green: https://tbpl.mozilla.org/?rev=5c0e83534802324020e7225e85ccfce8e4a08bc0&tree=Gaia-Try

Comment 19

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

Comment on attachment 8492128 [details] [review]
Gaia PR

(In reply to Yi-Fan Liao [:yifan][:yliao] from comment #14)
> [e-mail] mentioned the fix would require changes of all use of
> ModalDialog.
> 
> How about using navigator.mozL10n.get to check if the message is a l10n id?
> 
> if (navigator.mozL10n.get(message)) {
>   navigator.mozL10n.setAttributes(node, message);
> } else {
>   node.removeAttribute('data-l10n-id');
>   // may need to escape message here
>   node.textContent = message;
> }

No, we cannot rely on returned value from get being falsy. `get` is overall a method we're planning to deprecate and the overall behavior of l10n strings should be deterministic and user friendly.

It means that first of all we want to know which strings are localizable without having to try, and second, when we do attempt to localize, l10n library will try a fallback chain of locales (es-MX -> es -> en-US) which will load three additional locales in this case.

I'll be happy to help update the API and it's use cases to handle the dual nature of the dialog.

Feel free to assign this bug to me :)

Comment 20

[:gerard-majax]

Comment on attachment 8492128 [details] [review]
Gaia PR

Fair enough. Let's try this way. From what I tested locally, it fixes the issue, and all unit tests I could run making use of this were green.

Comment 21

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

Comment on attachment 8492128 [details] [review]
Gaia PR

That looks good!

I have two suggestions that may help us standardize the approach to APIs that may need to take a raw string, but that's up to you if you want to update it in this patch.

Comment 22

[:gerard-majax]

Comment on attachment 8492128 [details] [review]
Gaia PR

Nits addressed.

Comment 23

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

Comment on attachment 8492128 [details] [review]
Gaia PR

One more nit I described in PR and r+

Comment 24

[:gerard-majax]

Latest nits addressed, I'll merge when try is green :)

Comment 25

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

Lgtm!

Comment 26

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/3802009e1ab6c3ddfc3eb15522e3140a96b33336

Comment 27

[:gerard-majax]

Comment on attachment 8492128 [details] [review]
Gaia PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): regression introduced by bug 1029944
[User impact] if declined: empty modal dialog on network-generated user interaction
[Testing completed]: tested locally, since it depends on CDMA network-generated messages
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 28

[Anshul]

Alexandre, your patch doesn't apply cleanly on 2.1. Please provide the patch for 2.1 so I can land it internally for testing purposes while we are waiting for approval-gaia-v2.1

Comment 29

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/c58656b9cf3a60049aa48c4a379bf0e7b1e37953

Comment 30

[:gerard-majax]

(In reply to Anshul from comment #28)
> Alexandre, your patch doesn't apply cleanly on 2.1. Please provide the patch
> for 2.1 so I can land it internally for testing purposes while we are
> waiting for approval-gaia-v2.1

Anshul, I'm not sure what your problem is, Ryan could uplift to v2.1 branch without any issue.

Comment 31

[Anshul]

Alexandre, may be my build was a few days old. Anyways, doesn't matter since it is uplifted.