995605 - [Sora][Message][MMS]Can't add the text in content when forward a media MMS.

Status: RESOLVED FIXED

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

Description

[sync-1]

Mozilla build ID:20140323004002
 
 DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 ->MS send/received a MMS,the MMS content only have media files;
 ->Long tap the MMS,select forward the MMS;
 ->Tap the content pane to edit,the keyboard can't display,it means can't add the text in content.(ko)
 ->Back to edit other message interface,can't use the virtual keyboard to input text in content.(ko)
 
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[Julien Wajsberg [:julienw]]

Forwarding is new to 1.3 so it can't be a regression from a previous version.

That said, this is in my opinion serious because after this bug happen we can't focus the composer anymore, even in another thread or when starting a new message !

Comment 2

[Jason Smith [:jsmith]]

Can we reproduce this on the Moz side?

Comment 3

[Maria Angeles Oteo (:oteo)]

Not able to reproduce this bug in latest 1.3 version
hamachi v1.3
B-130
Build Identifier-20140412061954
Gecko-e8ec2e6
Gaia-d98c34b

Comment 4

[Jason Smith [:jsmith]]

Backlog then if it's not reproducible.

Comment 5

[Julien Wajsberg [:julienw]]

FYI I could reproduce very easily but I have a slightly older build, I'll try again with a newer build.

Comment 6

[Julien Wajsberg [:julienw]]

Maria, I can reproduce very easily... Could you recheck on your side?

I'd like this to be at least a 1.4 blocker because it's really awkward.

Comment 7

[Maria Angeles Oteo (:oteo)]

You are right Julien, sorry for the inconvenience, it seems I didn't follow the STR correctly...

I am reproducing the bug in latest v1.3 version
hamachi v1.3
B-141
Gecko-1949f0e
Gaia-fb2b8e2

and agree that it's an awful error and IMHO we should fix it in v1.3

So nominating to v1.3 because once the error happens it breaks totally the SMS application as you can not use the keyboard in the composer anymore.

I had to kill the SMS application and launch it again to make the keyboard work and be able to send a SMS.

Comment 8

[Julien Wajsberg [:julienw]]

I think it would be easy to fix: just ensure there is a text placeholder after forwarding (I think Compose.fromMessage is the place).

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

triage: blocking 1.4+ but please ask for approval to 1.3 when the patch is ready. thanks

Comment 10

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #9)
> triage: blocking 1.4+ but please ask for approval to 1.3 when the patch is
> ready. thanks

We don't grant approvals for 1.3 anymore. They have be cert blockers to be taken at this point.

Vance - Can you find out if this is a cert blocker for TCL?

Comment 11

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Yes this is a cert blocker for TCL

Comment 12

[Julien Wajsberg [:julienw]]

taking

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: github PR

The issue is that in compose.js, "pointer-events: none" is added after it's
    removed, in this case.
    
    This patch ensures that SMIL.parse always calls the callback in an asynchronous way,
    so that the caller does not have any surprise.

 apps/sms/js/desktop-only/mobilemessage.js | 18 ++++++++++++++++++
 apps/sms/js/smil.js                       |  2 +-
 apps/sms/test/unit/smil_test.js           | 25 +++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

Comment 14

[Julien Wajsberg [:julienw]]

This is a good lesson: I always say it's a good practice to ensure that calling a callback should always be done either in an asynchronous or synchronous way, but here is a very concrete bug coming from the face we didn't respect the good practice.

Note that Promises are always asynchronous by design, so it's great to use them :) Maybe we should eventually use a Promise here too, but since it's a 1.3+ bug, I'd rather be conservative and safe.

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8412554 [details] [review]
github PR

Only some nits for the comment part, thanks for the quick fix!

Comment 16

[Julien Wajsberg [:julienw]]

Thanks for the quick review !

master: c6f4657a4ec2d9fc4fff7b7b8f96f91ef3bd13c2

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/6e95a110e87e5f02dbb4af5cc266e4f2c1428ab0

Please request approval-gaia-v1.3 on this patch when it is ready for 1.3 uplift.

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8412554 [details] [review]
github PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 976999
[User impact] if declined: after forwarding an image-only MMS, the user can't interact with the composer, even in other panels, unless he forwards another MMS or restart the application.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v1.3: https://github.com/mozilla-b2g/gaia/commit/a8f5ca74ce896569c601c6849d534ff9fa7ce8cb