Closed
Bug 1068860
Opened 10 years ago
Closed 10 years ago
[CBS] Cell Broadcast dialogues are empty
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 fixed)
People
(Reporter: mschwart, Assigned: gerard-majax)
References
Details
(Keywords: regression, Whiteboard: [caf priority: p2][CR 725896][systemsfe])
Attachments
(1 file)
46 bytes,
text/x-github-pull-request
|
zbraniecki
:
review+
bajaj
:
approval-gaia-v2.1+
|
Details | Review |
[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.
Blocks: CAF-v2.1-FC-metabug
Updated•10 years ago
|
Keywords: regression
Comment 2•10 years ago
|
||
(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?
Flags: needinfo?(mschwart)
Flags: needinfo?(mhenretty)
Comment 3•10 years ago
|
||
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
Flags: needinfo?(mhenretty)
Reporter | ||
Comment 4•10 years ago
|
||
Sorry I didn't intend to use any defined terms. Thanks Michael for chiming in.
Flags: needinfo?(mschwart)
Comment 5•10 years ago
|
||
Maybe a branch check first to find the regressed bug?
Flags: needinfo?(timdream)
Keywords: qawanted
Comment 6•10 years ago
|
||
(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.
Keywords: qawanted
Comment 8•10 years ago
|
||
Alex, can you take a look?
Flags: needinfo?(lissyx+mozillians)
Whiteboard: [systemsfe]
Updated•10 years ago
|
Whiteboard: [systemsfe] → [CR 725896][systemsfe]
Assignee | ||
Comment 10•10 years ago
|
||
Can you provide a screenshot and more informations on how to reproduce ?
Flags: needinfo?(lissyx+mozillians) → needinfo?(mschwart)
Assignee | ||
Comment 11•10 years ago
|
||
I confirm Michael's fear, it looks like it is because of bug 1029944
Depends on: 1029944
Assignee | ||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8492128 [details] [review] Gaia PR Gandalf, would you please review this?
Attachment #8492128 -
Flags: review?(gandalf)
Assignee | ||
Comment 18•10 years ago
|
||
Gaia-Try is green: https://tbpl.mozilla.org/?rev=5c0e83534802324020e7225e85ccfce8e4a08bc0&tree=Gaia-Try
Comment 19•10 years ago
|
||
Comment on attachment 8492128 [details] [review] Gaia PR (In reply to Yi-Fan Liao [:yifan][:yliao] from comment #14) > alissy@mozilla.com 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 :)
Attachment #8492128 -
Flags: review?(gandalf) → review-
Assignee | ||
Comment 20•10 years ago
|
||
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.
Attachment #8492128 -
Flags: review- → review?(gandalf)
Comment 21•10 years ago
|
||
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.
Attachment #8492128 -
Flags: review?(gandalf) → review+
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8492128 [details] [review] Gaia PR Nits addressed.
Attachment #8492128 -
Flags: review+ → review?(gandalf)
Comment 23•10 years ago
|
||
Comment on attachment 8492128 [details] [review] Gaia PR One more nit I described in PR and r+
Attachment #8492128 -
Flags: review?(gandalf) → review+
Assignee | ||
Comment 24•10 years ago
|
||
Latest nits addressed, I'll merge when try is green :)
Flags: needinfo?(mschwart) → needinfo?(gandalf)
Assignee | ||
Comment 26•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/3802009e1ab6c3ddfc3eb15522e3140a96b33336
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•10 years ago
|
||
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
Attachment #8492128 -
Flags: approval-gaia-v2.1?
Updated•10 years ago
|
Target Milestone: --- → 2.1 S5 (26sep)
Updated•10 years ago
|
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
Comment 28•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #8492128 -
Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Comment 29•10 years ago
|
||
v2.1: https://github.com/mozilla-b2g/gaia/commit/c58656b9cf3a60049aa48c4a379bf0e7b1e37953
Assignee | ||
Comment 30•10 years ago
|
||
(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.
Flags: needinfo?(anshulj)
Comment 31•10 years ago
|
||
Alexandre, may be my build was a few days old. Anyways, doesn't matter since it is uplifted.
Flags: needinfo?(anshulj)
Updated•10 years ago
|
Whiteboard: [CR 725896][systemsfe] → [caf priority: p2][CR 725896][systemsfe]
You need to log in
before you can comment on or make changes to this bug.
Description
•