918576 - [B2G][SMS] The text MMS does not appear above the send button if there is only one attachment

Status: RESOLVED FIXED

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

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: MMSlog.txt

Description:
The word "MMS" does not appear above the "send" button after adding an attachment when sending a message to only one recipient. It will appear if the user types text in the body or adds another recipient.

Repro Steps:
1)  Updated to Buri Build ID: 20130916040205
2)  Tap the "SMS" icon to open messaging.
3)  Tap the "Compose New Message" icon.
4)  Tap the "+" and select a contact to add to the "To" field.
5)  Tap the "Paperclip" icon on the bottom left corner of the screen.
6)  Tap on the "Wallpaper" button.
7)  Tap a "Wallpaper" to add it to the message.
8)  Look above the "Send" button and notice that it is blank.
9)  Enter text into the body field below the wallpaper. Look above the "Send" button.

Actual:
The text "MMS" is missing when sending a MMS to only one recipient or when the body of the message is blank.

Expected:
The text "MMS" appears above the "Send" button after adding an attachment.

Environmental Variables
Build ID: 20130916040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Platform Version: 26.0a1

Notes:
Repro frequency: (100%)
See attached: (screenshot, logcat)

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: MMSsendbutton.png

Comment 2

[Julien Wajsberg [:julienw]]

I see stranger things on an Unagi/master: the image is not displayed either :/

We need to block on this for sure.

Comment 3

[Julien Wajsberg [:julienw]]

(don't know why I asked info from Michael, removing it)

Comment 4

[Darren]

The regression window appears to be between 09/04 and 09/05:

Did occur-
Buri Build ID: 20130905040201
Gecko: http://hg.mozilla.org/mozilla-central/rev/77ed46bf4c1a
Gaia: ec885264d885d877d68980c0227058231f18ad09
Platform Version: 26.0a1


Did not occur -
Buri Build ID: 20130904040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/7ff96bd19c1c
Gaia: b6c5bf1d24230bfed4a8f680e625fa2175001f82
Platform Version: 26.0a1

The MMS icon either pops-up for a split second and disappears or is just not present.

Comment 5

[Julien Wajsberg [:julienw]]

Just like I expected, this happens since bug 902497, which looks consistent as it touched that code.

Taking it to figure this out.

Comment 6

[Julien Wajsberg [:julienw]]

The STR is simply to attach an image, it happens with any recipients number.

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: patch v1

* compose.js: trigger the input event after changing the type
* thread_ui.js: don't do anything in the asynchronous getSegmentInfoForText
  callback if the type is not SMS at this point
---
 apps/sms/js/compose.js               |    3 +--
 apps/sms/js/thread_ui.js             |    5 +++++
 apps/sms/test/unit/thread_ui_test.js |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

Github PR is at https://github.com/mozilla-b2g/gaia/pull/12372

Comment 8

[Corey Frang [:gnarf]]

Comment on attachment 808708 [details] [diff] [review]
patch v1

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

r+ with a few questions and nits

::: apps/sms/js/compose.js
@@ +78,5 @@
>      if (!hasFrames && state.type === 'mms') {
>        compose.type = 'sms';
>      }
>  
> +    trigger('input', new CustomEvent('input'));

Why did this move to happening after the compose type changed?  I don't think it matters much, but was curious why it moved.

::: apps/sms/js/thread_ui.js
@@ +670,5 @@
>      var kMaxConcatenatedMessages = 10;
>  
>      // Use backend api for precise sms segmentation information.
>      var smsInfoRequest = this._mozMobileMessage.getSegmentInfoForText(value);
>      smsInfoRequest.onsuccess = (function onSmsInfo(e) {

Totally unnecessary, but any chance you feel like doing the s/e/event/ while you're in this area?

::: apps/sms/test/unit/thread_ui_test.js
@@ +895,5 @@
>  
>        Compose.clear();
>        this.sinon.clock.tick(ThreadUI.UPDATE_DELAY);
>        segmentInfo.segments = 0;
> +      // we have 2 requests, then triggering twice

nit: s/then triggering/so we trigger/

@@ +945,5 @@
>  
>        Compose.clear();
>        this.sinon.clock.tick(ThreadUI.UPDATE_DELAY);
>        segmentInfo.segments = 0;
> +      // we have 2 requests, then triggering twice

same

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #8)
> ::: apps/sms/js/compose.js
> @@ +78,5 @@
> >      if (!hasFrames && state.type === 'mms') {
> >        compose.type = 'sms';
> >      }
> >  
> > +    trigger('input', new CustomEvent('input'));
> 
> Why did this move to happening after the compose type changed?  I don't
> think it matters much, but was curious why it moved.

The problem was because of two things:
* the asynchronous nature of getSegmentInfoForText
* the fact that the input event handler is doing things differently depending on the type ([1] and [2]).

I fixed both, even if for this specific case I could have done only one of them.

I admit I was lazy and didn't do a test for this one, I'll try to do one before merging.

[1] https://github.com/mozilla-b2g/gaia/blob/6e32fffdcf9c38f14d4dc2c7933c19faa35cfe8e/apps/sms/js/thread_ui.js#L343-L347
[2] https://github.com/mozilla-b2g/gaia/blob/6e32fffdcf9c38f14d4dc2c7933c19faa35cfe8e/apps/sms/js/thread_ui.js#L700


> 
> ::: apps/sms/js/thread_ui.js
> @@ +670,5 @@
> >      var kMaxConcatenatedMessages = 10;
> >  
> >      // Use backend api for precise sms segmentation information.
> >      var smsInfoRequest = this._mozMobileMessage.getSegmentInfoForText(value);
> >      smsInfoRequest.onsuccess = (function onSmsInfo(e) {
> 
> Totally unnecessary, but any chance you feel like doing the s/e/event/ while
> you're in this area?

I actually like "e" ;-) But I'll change it if it makes things easier for everyone.

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: patch v2

carrying r=gnarf

Follow-ups:
* added tests for Compose events
* s/e/event/ in updateSmsSegmentLimit
* comments change

Github PR is at https://github.com/mozilla-b2g/gaia/pull/12372

ready to be merged when the tree is open.

Comment 12

[Julien Wajsberg [:julienw]]

master: b6e6a532bf36e9194fa9301205ba8160168a0211

Comment 13

[James Lal [:lightsofapollo]]

Uplifted b6e6a532bf36e9194fa9301205ba8160168a0211 to:
v1.2: 6531497a175e4a9dfa4a50ec62e336f0c5622eea

Comment 14

[Lucas Aliaga]

Verified fixed in Latest 1.2 Buri. MMS displays on first attachment.

Environment:
Gaia   5ef3535021286ccab7af639897feaaf5955720a0
SourceStamp 75b0b968f3ed
BuildID 20131016004005
Version 26.0a2