948958 - [Messages][MMS] MMS does not display correctly when it has a subject but no content.

Status: VERIFIED FIXED

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

Description

[Loli (:lolimartinezcr)]

Attachment: 2013-12-11-16-12-29.png

tested in:
1.3
12/11
Gecko 3b2d739
Gaia cbd2921

Steps:
1. Tap messages aplication.
2. Tap  in new message icon.
3. Tap in top-right icon and select "Add subject" option.
4. Write phone number, introduce only spaces in subject and body is empty.
5. Press "Send" button
(See attached)

Actual result:
"There is a problem with the attached file and it cannot be downloaded." (see attached)

Comment 1

[Loli (:lolimartinezcr)]

Attachment: 2013-12-11-16-12-40.png

Comment 2

[Julien Wajsberg [:julienw]]

So probably Gecko does not like empty subject. Or maybe we're trimming it?

Ayman, should we accept sending a subject with only spaces? Or should we handle this just like an empty subject (and it would mean send the message as a SMS if it contains only text)?

Comment 3

[Fernando Campo (:fcampo)]

Only operation done to subject is exchanging multiple spaces and breaklines into single space 
[dom.subject.value.replace(/\n\s*/g, ' ')]

Comment 4

[Julien Wajsberg [:julienw]]

I tested it myself, and the issue is happening also with any subject.

The issue is that the message is converted to MMS as soon as there is a subject, but ThreadUI.buildMessageDOM assumes that if the message is a MMS it must have attachments, and that if it has no attachment it's an error.

See [1] and [2].

The issue is happening in the same way when _receiving_ a MMS with a subject and no attachment. And what's worse is that it happens in 1.1 and 1.2 as well!

Therefore I'm gonna ask koi+ on this.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L1397-L1398
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L1428

Comment 5

[Julien Wajsberg [:julienw]]

Correction: it works fine both in 1.1, 1.2 and 1.3 as soon as there is a body content (text or attachment).

So it's maybe not enough a blocker to warrant koi+, but we can fix it for 1.3+ for sure.

Comment 6

[Jason Smith [:jsmith]]

Does this reproduce on 1.1?

Triage Notes:

Probably not worth blocking on. It doesn't feel critical enough to block 1.2 at this point.

Comment 7

[J Zimbrick]

As far as I can tell, there does not appear to be a way to add a subject line to SMS/MMS when using the latest 1.1 or 1.2 Mozilla builds on a Buri device.

I have the option in 1.1 to select attachments from Video, Wallpaper, Music, Gallery, and Camera, but see nothing concerning a subject line. In 1.2 only Video, Music, Gallery, and Camera are shown.

Sending an SMS in 1.1 and 1.2 that is filled only with spaces simply sends an empty message to the receiving phone, no error message is displayed.

Is there a way to add a subject line to an SMS message in the 1.1 and 1.2 branches that I'm unaware of?

Environmental Variables:
Device: Buri v1.1 Mozilla RIL
BuildID: 20131213041208
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Gecko: bdac595a4e46
Version: 18.0
Base Image: V1.2_20131115

Device: Buri v1.2 Mozilla RIL
BuildID: 20131213004002
Gaia: 1aca7c4860e39b1a9969807d335dcf9f070ea9b3
Gecko: a2b69b561d9b
Version: 26.0
Base Image: V1.2_20131115

Comment 8

[Julien Wajsberg [:julienw]]

You can't send a message with a subject but you can receive one.

I actually tried it myself on my 1.1 device when I wrote comment 4 so the only qawanted thing is with 1.2.

Also, I'd like to know what's the behavior on Android, but I don't know if that's the goal of qawanted.

Thanks!

Comment 9

[J Zimbrick]

Thanks for the clarification, Julien.

Just tested this on the latest Buri 1.2 Mozilla build.

When receiving a message with a subject line but no body text from a another device running the latest 1.4 Buri Mozilla build, an error is displayed stating: "There is a problem with the attached file and it cannot be downloaded."

Environmental Variables:
Device: Buri v1.2 Mozilla RIL
BuildID: 20131216004002
Gaia: fcf1c2fe020c29da4755621cbffdc1a333a43be9
Gecko: 129ad3c335a5
Version: 26.0
Base Image: V1.2_20131115

I'll leave the qawanted tag as I do not have an Android phone available.

Comment 10

[J Zimbrick]

Checked with two testers here that did have personal Android devices. There didn't seem to be any option for them to send an SMS with a subject from their devices, but I did send an SMS with a Subject and a blank body from a Buri device running the latest 1.4 Buri Mozilla build.

On the first Android (Galaxy S3 running 4.1 Jellybean), the message was received without error, displaying the subject line text and stating that the body was empty. 

On the second Android (LGG 2 running 4.2.2 Jellybean), the message with a subject line but no body simply wasn't received at all. Sending an SMS with text was received fine and sending an MMS with picture attachment was received fine.

And, though not asked for, I checked how this works on iPhone 5 running iOS version 7.0.3. The device isn't able to send a message that only has a subject and no body, the send button does not function until body text is entered. When receiving a message with a subject but no body text, the message is received without error and shows the subject text in bold and a blank body.

Hopefully this was the sort of information you were looking for, removing the qawanted tag for now.

Comment 11

[Julien Wajsberg [:julienw]]

Yes, this is very comprehensive, thanks a lot!

Ayman, how should we display a MMS that has a subject but no content?
And please also answer for older versions that don't show a subject too, as we might need to do a patch for this as well. ;)

Comment 12

[Stephany Wilkes]

I cannot tell from the two attached screenshots, but just to be clear: the user can still see the body of the message if there is a message body, yes? Just not the attachment? The context in which the error message screenshot (attached to this bug) appears is not clear. Thanks!

Comment 13

[Julien Wajsberg [:julienw]]

The error message appears as soon as there is a subject but no content at all (it means: no attachment, no text).

As soon as there is some content, there is no error.

Comment 14

[Maria Angeles Oteo (:oteo)]

(In reply to Stephany Wilkes from comment #12)
> I cannot tell from the two attached screenshots, but just to be clear: the
> user can still see the body of the message if there is a message body, yes?
> Just not the attachment? 
As Julien has said the failure is happening when sending a MMS without body content
STR:

1. Tap messages aplication.
2. Tap  in new message icon.
3. Tap in top-right icon and select "Add subject" option.
4. Write phone number, introduce any subject and body is empty.
5. Press "Send" button

Actual result:
It appears the subject of the message and instead of a empty body the message "There is a problem with the attached file and it cannot be downloaded." see screenshot

Comment 15

[Maria Angeles Oteo (:oteo)]

Attachment: 2013-12-18-12-20-32.png

Comment 16

[John Hammink]

Bumping to 1.3 in triage.   It's possible that the impact of landing a patch this late is bigger than the benefit of fixing this feature now.

Comment 17

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

triage: 1.3+. broken new feature implementation

Comment 18

[Steve Chung [:steveck]]

I can take a look first, but I need a clear answer for solving the problem. For 1.3, how about we just showing a single empty line in the content field with subjuct in the message bubble? For the older version, we don't handle the subject. Maybe we can show the subject in the content field if there is only subject but no message content in this case?

Comment 19

[Julien Wajsberg [:julienw]]

I think we need a message like "Empty message" inside the bubble, but written with a different font style than usual messages.

We don't need to care about older versions now, since koi+ was rejected ;) This is quite an edge case anyway.

Comment 20

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> I think we need a message like "Empty message" inside the bubble, but
> written with a different font style than usual messages.
> 
> We don't need to care about older versions now, since koi+ was rejected ;)
> This is quite an edge case anyway.

on yes you're right, since it's 1.3 blocking issue now, we don't have to take care about the old version case.

Comment 21

[Steve Chung [:steveck]]

By the way, I'm ok with the "Empty message" inside the bubble. We just need UX input for some confirmation.

Hi Ayman, if we have "Empty message" inside the bubble, do we still need the message in attachment 8345909 [details] for no attachment error case? If we both have 'Empty message' and no attachment error cases, do we need to apply different font style for both case?

Comment 22

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Borja, I commit this patch based on Julien's previous comment, since he might busy on other tasks, could you please take a look first? Thanks.

Comment 23

[Julien Wajsberg [:julienw]]

FYI I just pinged Ayman by email so that he can have a quick look here.

Thanks to both of you :)

Comment 24

[Borja Salguero [:borjasalguero]]

Attachment: Spinner.png

Testing the code I've just realized that the spinner is not hidden when sending this type of MMS with Subject but empty body and no attachments. Steve, could you take a look? Thanks!

Comment 25

[Julien Wajsberg [:julienw]]

Could this be bug 957084 ?

Comment 26

[Steve Chung [:steveck]]

(In reply to Borja Salguero [:borjasalguero] from comment #24)
> Created attachment 8361568 [details]
> Spinner.png
> 
> Testing the code I've just realized that the spinner is not hidden when
> sending this type of MMS with Subject but empty body and no attachments.
> Steve, could you take a look? Thanks!

Might be the problem in bug 957084 like Julien said, but I can send the subject only message on my device.
If we are actually sending the empty message(it's still a valid mms), so showing the spinner while sending is quite normal, isn't it? :)

Comment 27

[Steve Chung [:steveck]]

Attachment: Screen Shot 2014-01-06 at 17.46.41.png

Hi Ayman, the below 2 message bubble is the result after applying the patch. The content shows 'empty message' is the valid mms message with subject only. It's Julien's opinion that showing 'empty message' wording inside the bubble with differnt font type and color. The bottom one is a mms message without subject and attachment, which should be a rare case and hardly reproducible. We will display a warning message with error icon in the bubble. It would be great if you have better solution for displaying these 2 different cases, thanks.

Comment 28

[ayman maat :maat]

referencing comment 27 passing ni? to Jose as this is more of a visual design question.

Comment 29

[José Vittone]

Passing ni to Ayman again since he has more info about the original spec.

Comment 30

[Steve Chung [:steveck]]

Hi Ayman, I think this would be more likely an UX issue first, we need a clear answer for empty message(subject only) MMS about how we display the message in the bubble:
1) My idea is we can simply showing the bubble with empty content. The message content in bubble should have min height for sending cursor.
2) Julien's idea is showing the 'empty message' in the message content with different style. Jose can provide some feedback if we choose this solution.
3) Any other way to display the empty message?

Comment 31

[José Vittone]

(In reply to Steve Chung [:steveck] from comment #30)
Hi Steve,
> 1) My idea is we can simply showing the bubble with empty content. The
> message content in bubble should have min height for sending cursor.
Seems to be enough to say that something is empty, it's explained via its shape. I understand Julien's idea on being clear, but finally the solution gets so complicated. 
So, from the UX PoV your first idea is just fine.

Comment 32

[Julien Wajsberg [:julienw]]

Sorry José, I don't understand why it is so "complicated" to show a message with a different style?

Comment 33

[José Vittone]

(In reply to Julien Wajsberg [:julienw] from comment #32)
> Sorry José, I don't understand why it is so "complicated" to show a message
> with a different style?

Hi Julien, I would say that:

- It's not about the complexity of showing a message with a different style, is about how to communicate an empty message. 

- Having a placeholder text like that will take the focus off the actual user input, in this case the subject. We should always highlight the content rather than the chrome.

- An empty message explains what's on it. Adding an explanatory text is redundant and also wierd because that space is reserved to be filled with other person words.

- This kind of message could work on the preview of the message, on the messages list, before entering the thread. Like Apple's email does, they don't modify the message itself.

- The time stamp is going to be moved inside the bubble as we defined here https://bugzilla.mozilla.org/show_bug.cgi?id=871432  will also balance that emptiness.

Comment 34

[Julien Wajsberg [:julienw]]

Oki, makes sense, thanks!

Comment 35

[Steve Chung [:steveck]]

Hi Borja,
I update the patch based on  José's comment in comment 33. Please review the patch for the final result, thanks!

Comment 36

[Jason Smith [:jsmith]]

We punted this from 1.2. Why is this necessary for 1.3? Renoming for more discussion.

Comment 37

[Julien Wajsberg [:julienw]]

I think we didn't block from 1.2 mainly because the bug was found too late.

I don't think this is too late for 1.3 especially if we don't add new strings.

Comment 38

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #37)
> I think we didn't block from 1.2 mainly because the bug was found too late.
> 
> I don't think this is too late for 1.3 especially if we don't add new
> strings.

Well right, but that would mean that we should be pushing for approval here, not blocking.

Comment 39

[Preeti Raghunath(:Preeti)]

Already in 1.2, would like to wait till 1.4 for a fix.

Unblocking for 1.3 now.

Comment 40

[ayman maat :maat]

don't understand why bugzilla tells me i have a ni? for this bug when the bug tells me I don't.
Great to see this in V1.3

Comment 41

[Julien Wajsberg [:julienw]]

> Great to see this in V1.3

Won't be with the current flags, but I think we can request an approval to Fabrice as the code looks fairly safe. I haven't tested it myself so maybe Borja can do the request if he tested it and checked that it worked ?

Comment 42

[Borja Salguero [:borjasalguero]]

This behaviour is improved with the visual refresh, so I would like to check tomorrow with UX if we can move this to 1.4 for getting the whole solution. I'll come back tomorrow with more info about this! Thanks Julien for raising this.

Comment 43

[Julien Wajsberg [:julienw]]

But if we can get this patch into 1.3 then I think we should, instead of waiting for doing something better in 1.4.

Comment 44

[Borja Salguero [:borjasalguero]]

Attachment: Subject but empty body mms

Jose, this is the result when the patch is applied. Could you take a look? We would like to know if we should push for adding this to 1.3, or wait the final solution coming with the refresh in 1.4. Thanks!

Comment 45

[José Vittone]

Comment on attachment 8369910 [details]
Subject but empty body mms

I think the result is just fine. I'd would include it in 1.3 because thread's visual refresh for 1.4 is reduced to color changes. Good job guys!

Comment 46

[Borja Salguero [:borjasalguero]]

https://github.com/steveck-chung/gaia/commit/00c5ae4f29f48fab128ca85e888315ea361266d4
https://github.com/mozilla-b2g/gaia/commit/014e6c39c69fdc51c08079c9bb3b827664387dcd
R+. Merged in master.

Comment 47

[Borja Salguero [:borjasalguero]]

Comment on attachment 8361485 [details] [review]
Link to github

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 #): When sending a MMS with subject but without body the UI was not working as expected.
[User impact] if declined: Wrong UI
[Testing completed]: Working fine in my tests
[Risk to taking this patch] (and alternatives if risky): This patch is not risky due to is surgical, and only modify one scenario for avoiding the wrong UI rendering when sending a MMS with Subject but without Body. Safe to land.
[String changes made]: none

Comment 48

[Rick Waldron [:rwaldron]]

Borja or Steve, 

Can either of you explain why the `noAttachment` case was lumped into the "message without subject" case? I've read through the history in this bug and I don't see where Ayman signed off on rolling these into one. 

Relevant comments: https://github.com/mozilla-b2g/gaia/commit/014e6c39c69fdc51c08079c9bb3b827664387dcd#diff-1

Comment 49

[Steve Chung [:steveck]]

(In reply to Rick Waldron [:rwaldron] from comment #48)
> Borja or Steve, 
> 
> Can either of you explain why the `noAttachment` case was lumped into the
> "message without subject" case? I've read through the history in this bug
> and I don't see where Ayman signed off on rolling these into one. 
> 

Hi Rick,
Before v1.3, we didn't implement subject feature for mms(neither edit nor display it). If the mms has no attachments(media and text), we treat it as an error case and display error message inside the message bubble with error style. But mms without attachment is validate if the message have subject. Once subject feature landed in v1.3, we should also consider the subject for handling different cases as well. So the flow will become:

 Before v1.3: If mms has no attachments -> display error message inside the bubble with error style

 After v1.3: a) mms has no attachments and no subject -> display error message inside the bubble with error style
             b) mms has no attachments but subject exist -> display empty message.

So we didn't lumped into "message without subject" case. It divide into 2 different cases based on the subject actually. This bug described that we treat (b) as an error case because of outdated logic for handling no attachment case. We need ux input here for (b) and José provided his viewpoint in comment 31.

Comment 50

[Noemí Freire (:noemi)]

Hi John,

Fabrice gave the approval-gaia-v1.3+, could you please help us with the uplift to v1.3 branch?. Thanks!

Comment 51

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 014e6c39c69fdc51c08079c9bb3b827664387dcd to:
v1.3: 31a2c3539e8cf0982d310f521809e63c7303d9f8

Comment 52

[Loli (:lolimartinezcr)]

Tested 02/06/2014 and working
1.3
Gecko 1fe8aca
Gaia fb35a67

Tested 02/06/2014 and working
master
Gecko fdde61f
Gaia 1aa8377