1100954 - Activities menu has no text after forwarding a SMS

Status: VERIFIED FIXED

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

Description

[Florin Strugariu [:Bebe]]

Attachment: 2014-11-18-16-18-52.png

The activities menu has no text after forwarding a sms message.

PREREQUISITES:

There is at least one message sent in the message app

STR:

1 Open Messaging app
2 Open the sent message
3 Long press on it 
4 Tap on Forward message option 
5 Introduce a phone number in the 'To' field
6 Tap on Send key 
7 Long press the sent message again 

Actual:
7. The Activities menu has no text in the buttons

Expected:
7. Menu items have text on the buttons.


Gaia-Rev        ae3a84acaab80a5b35d5542d63e68462273c8a1b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2d0a51ef828d
Build-ID        20141117160200
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  40
FW-Date         Tue Oct 21 15:59:42 CST 2014
Bootloader      L1TC10011880

Comment 1

[Pi Wei Cheng [:piwei] (inactive)]

I assume the qawanted tag is requesting for a branch check.

This issue occurs on Flame 2.2.

Observed behavior: Activities menu (Message options) displays without text after it had previously been used for forwarding. The issue persists until the app is killed in card view, and will re-occur again after forwarding.

Device: Flame 2.2 Master
BuildID: 20141118082629
Gaia: 4aee256937afe9db2520752650685ba61ce6097d
Gecko: 084441e904d1
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

------------

This issue does NOT occur on Flame 2.1 and Flame 2.0.

Observed behavior: Activities menu displays correctly after forwarding an SMS.

Device: Flame 2.1
BuildID: 20141117201226
Gaia: 1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko: 95fbd7635152
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.0
BuildID: 20141118044626
Gaia: 1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gecko: bde95439014c
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Joshua Mitchell (Inactive)]

Nomming for 2.2: regression, broken functionality, poor UX

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141106003822
Gaia: 2f077d05105f227839dbecb22cb5324f1321b934
Gecko: a4b93cd87739
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141106010822
Gaia: 0f93196939e02a559ed23ca4ade6301d3bc9a7de
Gecko: d0177443f822
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Gaia Last Working Gecko - issue DOES repro
Gaia: 0f93196939e02a559ed23ca4ade6301d3bc9a7de
Gecko: a4b93cd87739

First Broken Gecko Last Working Gaia - issue does NOT repro
Gaia: 2f077d05105f227839dbecb22cb5324f1321b934
Gecko: d0177443f822

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/2f077d05105f227839dbecb22cb5324f1321b934...0f93196939e02a559ed23ca4ade6301d3bc9a7de

Caused by Bug 1080828.

Comment 4

[Oleg Zasypkin [:azasypkin]]

Will take a look

Comment 5

[Oleg Zasypkin [:azasypkin]]

Okay, I see problem, will provide patch shortly

Comment 6

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Hey Julien,

In the patch I've tried to unify setting header content in one method as it's used by Report, Participant, Thread and Composer panels.

Could you please take a look?

Thanks!

Comment 7

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression and breaks function.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8525264 [details] [review]
GitHub pull request URL

r=me with nits and possibly another test
thanks!

Comment 9

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Comment on attachment 8525264 [details] [review]
> GitHub pull request URL
> 
> r=me with nits and possibly another test
> thanks!

Nits addressed + added simple integration test that covers underlying cause of the issue. Thanks!

Master: https://github.com/mozilla-b2g/gaia/commit/375cb5650609b147543006677a342565f2869213

Comment 10

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

Oleg, for the future, for consistency reasons, it would be nice to build L10n API's like setContentHeader this way: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs

So, basically if you pass HTML it should be {html: 'string'} and just 'string' should be L10nId.

Not sure if it's worth changing here. If not, I'm just evangelizing :)

Comment 11

[Oleg Zasypkin [:azasypkin]]

(In reply to Zibi Braniecki [:gandalf] from comment #10)
> Oleg, for the future, for consistency reasons, it would be nice to build
> L10n API's like setContentHeader this way:
> https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/
> localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs
> 
> So, basically if you pass HTML it should be {html: 'string'} and just
> 'string' should be L10nId.
> 
> Not sure if it's worth changing here. If not, I'm just evangelizing :)

Hey Zibi, good to know that we have recommended approach for such things! We'll definitely use the same method signature for similar methods.

The only thing that bothers me in recommended API example is the case when it should support both "{html: string}" and "l10nId": if we first call method with "rich" HTML string and then with l10n args - setAttributes will fail and l10n won't process other strings (as we see in this bug - l10n fails to set l10n attributes for header, but some other l10n strings from unrelated elements were affected too).

Is there any recommended way to deal with the case above? In "setContentHeader" we use "if(el.firstElementChild) { el.textContent = ''; }" before setting l10n attributes.

P.S. We actually like that l10n fails for unrelated strings - it's easier to notice the bug :)

Comment 12

[sarsenyev]

Verified the issue is fixed on 2.2 Flame

The text with menu and options are displayed when long tapping the sent forwarded message

Flame 2.2
Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141124040203
Gaia: c5bad6d78c5fe168e3bb894fc5cb70902c9b19b1
Gecko: 8c02f3280d0c
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 13

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

(In reply to Oleg Zasypkin [:azasypkin] from comment #11)
> The only thing that bothers me in recommended API example is the case when
> it should support both "{html: string}" and "l10nId": if we first call
> method with "rich" HTML string and then with l10n args - setAttributes will
> fail and l10n won't process other strings (as we see in this bug - l10n
> fails to set l10n attributes for header, but some other l10n strings from
> unrelated elements were affected too).
> 
> Is there any recommended way to deal with the case above? In
> "setContentHeader" we use "if(el.firstElementChild) { el.textContent = '';
> }" before setting l10n attributes.

Yeah, that's a good point. We do not have any clean solution right now because we have no clean way to store the "original DOM Fragment" for retranslation purposes (so that you apply translation of top of source DOM, vs previous translation).

For that reason, for now, we don't want to do anything on our own hence we don't clean the node after you remove l10n-id.

We're working with the platform team to move the l10n-id piece into the platform as WebAPI and we're thinking of some sort of ShadowL10n DOM, which would allow us to make things cleaner and removing translation or retranslation would work better.

For now, your solution is the right one. I'll document it in the best practices :)

Comment 14

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

For the record. The bug for automatically cleaning up is bug 1083837

Comment 15

[Oleg Zasypkin [:azasypkin]]

(In reply to Zibi Braniecki [:gandalf] from comment #14)
> For the record. The bug for automatically cleaning up is bug 1083837

Cool! This is what we need :)