Closed Bug 885680 Opened 7 years ago Closed 6 years ago

[Messages] Subject management when composing a MMS

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: borjasalguero, Assigned: fcampo)

References

Details

(Whiteboard: MMS_TEF, [u=commsapps-user c=messaging p=0] )

Attachments

(7 files, 7 obsolete files)

183.34 KB, application/pdf
Details
21.28 KB, image/png
Details
22.14 KB, image/png
Details
22.43 KB, image/png
Details
46.34 KB, image/png
Details
1.21 MB, application/pdf
Details
46 bytes, text/x-github-pull-request
borjasalguero
: review+
julienw
: feedback+
rwaldron
: feedback-
Details | Review
We have to show the subject in the MMS App when receiving a MMS.
blocking-b2g: --- → leo?
Depends on: 885679
Whiteboard: MMS_TEF
Whiteboard: MMS_TEF → MMS_TEF, TaipeiWW
Assignee: nobody → fbsc
(In reply to Borja Salguero [:borjasalguero] from comment #0)
> We have to show the subject in the MMS App when receiving a MMS.

Cert issue or implementation detail?
Bug 885679 suggests that this is likely a cert issue. Dietrich - can you confirm?
Flags: needinfo?(dietrich)
The Gecko has exposed the |subject| in the MMS dom message object. Sounds like a pure Gaia issue?

interface nsIDOMMozMmsMessage : nsISupports
{
  ...
  readonly attribute DOMString subject;
  ...
};

If you're hoping to display the subject (of the last message) in the thread list as well (bug 885679), then there's something to do with the Gecko end.
blocking-b2g: leo? → koi?
Whiteboard: MMS_TEF, TaipeiWW → MMS_TEF
Whiteboard: MMS_TEF → MMS_TEF, [u=commsapps-user c=messaging p=0]
Summary: [MMS] Show subject in the thread-list and header in MMS → [MMS] Subject management in MMS
this is picked up as a "nice to have" in the product backlog for v1.2.
(Copying notes from bug 894265)

Relevant notes:

- MMS Messages already include a "subject" property (currently unused)
- SMS Messages do not include a "subject" property (SMS doesn't have subject)
- MessageManager.sendMMS will need a refactor to accept a "subject" parameter (or an options object)
- MessageManager.sendMMS currently sends an explicit empty string as a "subject".

From a program perspective, I believe adding support for "subject" will be trivial. From UI/UX perspective, it's hard to say. I've put together a set of screen caps showing how iOS deals with (sending) messages that have a subject: https://www.dropbox.com/sh/8vsbel95k0i11re/DQGzPZ6Z2a
Flags: needinfo?(dietrich)
Duplicate of this bug: 840517
Duplicate of this bug: 894265
Hi Rick,
Thanks added to attach our detail pdf ;)

'n we're considering it's useful for users :
 - If message has "subject", Messenger automatically send MMS.
 - If message has NO "subject",  Messenger automatically send SMS.
 - If message's size is larger than 140 bytes, Messenger automatically send MMS or Concatenates SMS( it depends on mobile carrier )   
 - If message's size is smaller than 140 bytes, Messanger automatically send SMS.
I would go by:

1. if there is an attachment and/or subject choose MMS
2. if it does not have an attachment and/or subject choose SMS

I am not a fan of changing it based on message size as not many users will be able to understand why suddenly something changed from SMS to MMS (if case they type that big messages)
Hi Yusuke,
Here we have to wait until getting feedback from UX Team. As this is a 'nice to have' thing, UX team is focused in v1.2 blockers, but they will come here soon!
Im going to add a 'needinfo' to Ayman, who is the owner of this.
Flags: needinfo?(aymanmaat)
(In reply to Yusuke Yamamoto from comment #9)
> Hi Rick,
> Thanks added to attach our detail pdf ;)
> 
> 'n we're considering it's useful for users :
>  - If message has "subject", Messenger automatically send MMS.
>  - If message has NO "subject",  Messenger automatically send SMS.
>  - If message's size is larger than 140 bytes, Messenger automatically send
> MMS or Concatenates SMS( it depends on mobile carrier )   
>  - If message's size is smaller than 140 bytes, Messanger automatically send
> SMS.

There are rules already clearly defined rule in V1.1 for conversion from SMS to MMS and back again. You need to take those into account.

I will look at this from a UX perspective (when i have bandwith in between v1.2 priorities) as the proposed solution on comment 7 is both suboptimal in terms of layout and also discards key information that is a requirement to display... and therefore is not viable.
blocking-b2g: koi? → ---
(In reply to ayman maat :maat from comment #12)
> There are rules already clearly defined rule in V1.1 for conversion from SMS
> to MMS and back again. You need to take those into account.
Oh, sorry about that...
I'm going to read detail of V1.1 spec to contribute to you.

So, I think , SMS-MMS spec is below link.
https://mozilla.app.box.com/applications/1/864518456
Is it correct?
Duplicate of this bug: 905026
Because it was a little unpopular, about correspondence of MMS Subject, the contents of "Add subject field.pdf". 

I made UX specifications like iPhone and attached screen capture "Messages Subject(It is like the iPhone).pdf" when I implemented it by a trial.

Please confirm it and someone follow me.
Comment on attachment 805799 [details]
Messages Subject(It is like the iPhone).pdf

Ayman, this looks like what we discussed during the workweek. Maybe you can give some guidance ?
Attachment #805799 - Flags: feedback?(aymanmaat)
(In reply to a.kitamura from comment #15)
> Created attachment 805799 [details]
> Messages Subject(It is like the iPhone).pdf
> 
> Because it was a little unpopular, about correspondence of MMS Subject, the
> contents of "Add subject field.pdf". 
> 
> I made UX specifications like iPhone and attached screen capture "Messages
> Subject(It is like the iPhone).pdf" when I implemented it by a trial.
> 
> Please confirm it and someone follow me.

Hi. Let me be absolutely clear that, though your input and feedback is more than welcome, I am driving the UX for the messaging app and overseeing the UX for all the comms apps. Please send all suggestions to me personally over email. Posting specifications that have not been validated 1) duplicates work (We are currently producing the specification for the inclusion of Subject) 2) causes confusion for the developers as follow the wrong specifications. Both of these scenarios bring at the least inefficiency and at the worst risk to the performance and ultimately success of the project... so please be a team player and direct your suggestions to the person who is overseeing the UX for the area you wish to input into. Thanks.
Flags: needinfo?(aymanmaat)
Comment on attachment 805799 [details]
Messages Subject(It is like the iPhone).pdf

**note to developers** 

do not follow these screen. they are both very much incomplete and conflict with other solutions that are being composed and proposed through the formal design track. 

It would be lovely if they could be removed.

The actual solution will be proposed in the forthcoming days.
Attachment #805799 - Flags: feedback?(aymanmaat) → feedback-
(In reply to Julien Wajsberg [:julienw] from comment #16)
> Comment on attachment 805799 [details]
> Messages Subject(It is like the iPhone).pdf
> 
> Ayman, this looks like what we discussed during the workweek. Maybe you can
> give some guidance ?

we cannot have subject permanently visible for two reasons: 
1) as it eats more real-estate which we do not have when the device is in landscape mode. 
2) due to end user cost implications we need to make MMS mode a clear action to enter into.. typing in a field that is permanently displayed is not a clear enough action as it invites the user to accidentally enter MMS and therefore incur higher charges.
Comment on attachment 805799 [details]
Messages Subject(It is like the iPhone).pdf

Dear a.kitamura, I'm going to make this proposal obsolete.

Thanks again for your contribution, it's always hard to contribute to UX because it needs to be consistent in the whole app.

We understand the subject handling is a much wanted feature and as Ayman has said we're commited to make it work soon :)

Once UX is frozen by Ayman, and we have some Visual Design, you could contribute to the code if you like !
Attachment #805799 - Attachment is obsolete: true
blocking-b2g: --- → 1.3?
Attached file FFOS_Subject_V1.3_20120920_V1.0.pdf (obsolete) —
*RELEASE NOTE*

new wireframes
- none

updated wireframes
- none

deleted wireframes
- none

new flows
- Add and remove subject in a new message
- Send new message with subject
- Add and remove subject in a new message thread
- Send new message in a thread with subject

updated flows
- none

deleted flows
- none

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

If you need clarification on the relationship between the subject field and the message field in the interaction model ping me.
Attached file FFOS_Subject_V1.3_20120920_V1.0.pdf (obsolete) —
(oops, correct document attached this time)

*RELEASE NOTE*

new wireframes
- none

updated wireframes
- none

deleted wireframes
- none

new flows
- Add and remove subject in a new message
- Send new message with subject
- Add and remove subject in a new message thread
- Send new message in a thread with subject

updated flows
- none

deleted flows
- none

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

If you need clarification on the relationship between the subject field and the message field in the interaction model ping me.
Attachment #807786 - Attachment is obsolete: true
QA Contact: isabelrios
Ayman, one remark: the max message length is not absolute as you define in the wireframes, but it depends on the message encoding.

These wireframes are clear to me but this handle only the composing, we'll need UX for the display of subject too.

I think these 2 cases should be separated in 2 bugs though. I'll create a new one for handling how we display a received message with a subject.
Summary: [MMS] Subject management in MMS → [MMS] Subject management when composing a MMS
See Also: → 920164
Now this bug is for composing, the gecko change is for bug 920164 instead.
No longer depends on: 885679
I suppose we need a Visual Design now.
Flags: needinfo?(vpg)
Thanks Julien, but not yet as the WF are not final, after a final version is out i will provide Visual Design for this feature. Not clearing needinfo as i will get back to this bug.
Depends on: 923023
Depends on: 923024
Assignee: borja.bugzilla → fernando.campo
Flags: needinfo?(vpg)
*RELEASE NOTE*

new wireframes
- none

updated wireframes
- none

deleted wireframes
- none

new flows
- Subject : New message, send message
- Subject : Message thread, send message
- Subject : New message, add and remove subject
- Subject : Message thread, add and remove subject

updated flows
- none

deleted flows
- New message : Save as draft from within app
- New message : Save as draft from outside of app
Attachment #807795 - Attachment is obsolete: true
Publishing visuals and layout specs. Please note that the icons are place holders and definitive ones will be posted here once they are final.
Summary: [MMS] Subject management when composing a MMS → [Messages] Subject management when composing a MMS
**RELEASE NOTE**

new wireframes
- none

updated wireframes
- none

deleted wireframes
- none

new flows
- Subject : Add subject when message thread is extended

updated flows
Subject : New message, send message
- affordance of message input field and subject field enhanced

Subject : Message thread, send message
- affordance of message input field and subject field enhanced

Subject : New message, add and remove subject
- affordance of message input field and subject field enhanced
- annotation on screen 3 altered

Subject : Message thread, add and remove subject
- affordance of message input field and subject field enhanced
- annotation on screen 3 altered

deleted flows
- none
**RELEASE NOTE**

new wireframes
- none

updated wireframes
- none

deleted wireframes
- none

new flows
- none

updated flows

Subject : New message, add and remove subject
- mms mode supressed until content is added to subject field

Subject : Message thread, add and remove subject
- mms mode supressed until content is added to subject field

deleted flows
- none
Attachment #814056 - Attachment is obsolete: true
Attachment #814881 - Attachment is obsolete: true
Blocking as it is a feature request.
blocking per comment 35
blocking-b2g: 1.3? → 1.3+
Need info to Peter La to provide the final menu icon for the header.
Flags: needinfo?(pla)
Hi Fernando.

rwaldron wanted me to find 1.3 feature bugs to work on and I was wondering if I could take this to work on.

Thanks!
Evelyn
Flags: needinfo?(fer.campo.garcia+bugzilla)
Hi,

Attached is the icon for the menu @1x and 1.5x sizes.  Until things get reworked in 1.3 and onwards, e've decided that it is best to stick to a consistent metaphor in this case rather than create a new one.
Flags: needinfo?(pla)
Comment on attachment 820533 [details]
Menu Action Icons @1x and 1.5x Resolutions

Apologies.  In my haste, I mistook this for a 1.2 item, and thus this icon is not what we intend.  We are in the process of updating the menu icon for 1.3.  Will post it as soon as we have a final design (soon).
Attachment #820533 - Attachment is obsolete: true
Hi Evelyn, 

thanks a lot for the offering, but there's some work already going on around this bug and a PR on the waiting (couple of blockers to be solved), so if it's ok with you, I'd rather to keep it :)

In case you can't find any other 1.3 suitable, let me know (better to free it than block it)
Flags: needinfo?(fer.campo.garcia+bugzilla)
(In reply to Fernando Campo (:fcampo) from comment #41)
> Hi Evelyn, 
> 
> thanks a lot for the offering, but there's some work already going on around
> this bug and a PR on the waiting (couple of blockers to be solved), so if
> it's ok with you, I'd rather to keep it :)
> 
> In case you can't find any other 1.3 suitable, let me know (better to free
> it than block it)

Sounds good, Fernando!
Will do.
(In reply to Fernando Campo (:fcampo) from comment #41)
> Hi Evelyn, 
> 
> thanks a lot for the offering, but there's some work already going on around
> this bug and a PR on the waiting (couple of blockers to be solved), so if
> it's ok with you, I'd rather to keep it :)
> 
> In case you can't find any other 1.3 suitable, let me know (better to free
> it than block it)

The reason I asked Evelyn to request this is because she's working on the Subject display as well. The only PR I see is the one for creating the header button menu—is there something else that I've missed?
(In reply to Rick Waldron [:rwaldron] from comment #43)
> The reason I asked Evelyn to request this is because she's working on the
> Subject display as well. The only PR I see is the one for creating the
> header button menu—is there something else that I've missed?

No, nothing missing, I never launched the PR, as I don't see the point of it until bug 923024 is solved, as that's the way we have to show/hide subject during compose.
Target Milestone: --- → 1.3 Sprint 4 - 11/8
**Release Note**

new wireframes
- none

updated wireframes
- none

deleted wireframes
- none

new flows
- Forward Message : Message from thread
- Delivery / Read report : Setting reports from within phone settings
- Delivery / Read report : Setting reports from message app inbox
- Delivery / Read report : Setting reports from within new message
- Delivery / Read report : Setting reports from within message thread
- Delivery / Read report : View report from thread
- Delivery / Read report : View delivery report from notification
- Delivery / Read report : View read report from notification
- Message Module Interactions : Long press and MMS
- Message Module Interactions : Tap on email address and MMS
- Message Module Interactions : Tap on url and MMS
- Message Module Interactions : Tap on phone number and MMS
- Anonymous messages : Thread

updated flows
- none

deleted flows
- none

pages relevant to this bug: 5-10 however no updates have been made in this wireframe pack, just attaching the latest wireframe pack here
Attachment #814944 - Attachment is obsolete: true
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 5 - 11/22
Hey Fernando, everything's moving smoothly ? :)
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Attached file pre-patch v1
Sending first version with some comments on github
Attachment #8336834 - Flags: feedback?(gnarf37)
Attachment #8336834 - Flags: feedback?(felash)
Comment on attachment 8336834 [details] [review]
pre-patch v1

I'm pretty sure we should leave compose.js mostly alone.  This is pretty invasive for adding a subject.  The subject should probably also not be a [contenteditable], but an <input maxlength=40>
Attachment #8336834 - Flags: feedback?(gnarf37) → feedback-
Comment on attachment 8336834 [details] [review]
pre-patch v1

Changed subject to textarea (we need multiline), and addressed some other comments. 
As gnarf is on PTO, could you take a look to the updated PR Rick?

I also would like to discuss about modifying Compose. IMO it should include Subject related stuff, as it is part of the MMS composition.

Again, this is not the final patch, so please ignore the debugging codes
Attachment #8336834 - Flags: feedback- → feedback?(waldron.rick)
Comment on attachment 8336834 [details] [review]
pre-patch v1

I've left some significant notes on the PR, please r? me when ready
Attachment #8336834 - Flags: feedback?(waldron.rick) → feedback-
(In reply to Fernando Campo (:fcampo) from comment #49)
> Comment on attachment 8336834 [details] [review]
> pre-patch v1
> 
> Changed subject to textarea (we need multiline), and addressed some other
> comments. 

Are you sure we need multiline ? Ayman is not here right now so I can't ask directly, I'm needinfo him then.
Flags: needinfo?(aymanmaat)
Just discussed with Ayman: yes we need multiline.
I was only considering the case of the user pressing enter (where there are other solutions) but not the case of wrapping the text.

Then Fernando it's probably easier to use a div[contenteditable] that will grow automatically rather than a textarea that you'll need to grow yourself on keypress.
Flags: needinfo?(aymanmaat)
Ayman, another question for you: assuming there are some recipients, when the user entered some text in the subject line, but no content in the message field, should the "send" button be enabled or disabled? (I'd say "disabled").
Flags: needinfo?(aymanmaat)
Ok, Ayman said 'enabled' in that case ;)
Flags: needinfo?(aymanmaat)
Comment on attachment 8336834 [details] [review]
pre-patch v1

I think the general feeling is good, except the subject markup/css itself. We need another way to do this than complex calculations and reflows. The current way was made before we had the final flex layout spec, and I'm sure we can convert at least some of it to the flex layout.

Let's move forward with our comments :)
Attachment #8336834 - Flags: feedback?(felash) → feedback+
(In reply to Julien Wajsberg [:julienw] from comment #52)
> 
> Then Fernando it's probably easier to use a div[contenteditable] that will
> grow automatically rather than a textarea that you'll need to grow yourself
> on keypress.

That was my first approach, but as Corey pointed out on the review, the current code assumes that Compose will have only one [contenteditable] at a time, so apart from the changes that implies, we would have to calculate the height limit for both subject and message every time we have an input (if it change the number of lines) on any of the fields anyway.
(In reply to Julien Wajsberg [:julienw] from comment #55)
> ...The current way was made before we had the final flex layout spec, and I'm sure
> we can convert at least some of it to the flex layout.
> 
I think that a transformation to flex layout would imply too many changes that shouldn't be treated in this bug (that focuses on only add a subject) (e.g. compose form and message container needs to be flex boxes), so maybe we can handle all at the same time in a followup better?
It seems strange to allow multiline input when all newline characters are removed anyway...
(In reply to Rick Waldron [:rwaldron] from comment #58)
> It seems strange to allow multiline input when all newline characters are
> removed anyway...
It seems weird indeed, but this is because the subject length limit of 40 characters is bigger than the space that we have to show them (around 32 in one line I think), so a second line is permitted to avoid horizontal scrolling. The fact that we allow pressing the new line button is a problem with the keyboard, because afaik we don't have a layout without the enter key (and just disable the action when the user presses it would be disorienting to the user).
(In reply to Fernando Campo (:fcampo) from comment #56)
> (In reply to Julien Wajsberg [:julienw] from comment #52)
> > 
> > Then Fernando it's probably easier to use a div[contenteditable] that will
> > grow automatically rather than a textarea that you'll need to grow yourself
> > on keypress.
> 
> That was my first approach, but as Corey pointed out on the review, the
> current code assumes that Compose will have only one [contenteditable] at a
> time, so apart from the changes that implies, 

I'm not sure these changes would be that big, especially compared to the code we need to add because of this.

> We would have to calculate the
> height limit for both subject and message every time we have an input (if it
> change the number of lines) on any of the fields anyway.

Except you won't need to "set" a height as often since it will be handled by the engine, and therefore it will be more efficient.


(In reply to Fernando Campo (:fcampo) from comment #59)
> (In reply to Rick Waldron [:rwaldron] from comment #58)
> > It seems strange to allow multiline input when all newline characters are
> > removed anyway...
> It seems weird indeed, but this is because the subject length limit of 40
> characters is bigger than the space that we have to show them (around 32 in
> one line I think), so a second line is permitted to avoid horizontal
> scrolling. The fact that we allow pressing the new line button is a problem
> with the keyboard, because afaik we don't have a layout without the enter
> key (and just disable the action when the user presses it would be
> disorienting to the user).

Just FYI With Ayman we discussed of the following behavior: pressing enter would move to the next field. But we agreed about not doing this now (because we need a multiline input for the reason you outlined anyway).


About the full flex box change, I agree this could be done in a follow-up... But I was wondering if we could not do only some of it now. Maybe that's not possible...
(In reply to Fernando Campo (:fcampo) from comment #59)
> (In reply to Rick Waldron [:rwaldron] from comment #58)
> > It seems strange to allow multiline input when all newline characters are
> > removed anyway...
> It seems weird indeed, but this is because the subject length limit of 40
> characters is bigger than the space that we have to show them (around 32 in
> one line I think), so a second line is permitted to avoid horizontal
> scrolling. The fact that we allow pressing the new line button is a problem
> with the keyboard, because afaik we don't have a layout without the enter
> key (and just disable the action when the user presses it would be
> disorienting to the user).

Pressing enter while in the subject line could bring you to the content area... I.E. "tab"
Corey, see my previous comment :p
Comment on attachment 8336834 [details] [review]
pre-patch v1

I addressed the comments and added some tests. There still an open discussion about using a [contenteditable] <div> or a <textarea>, flex layout and subject multiline, but guess we can handle it on the comments.
Attachment #8336834 - Flags: review?(waldron.rick)
Attachment #8336834 - Flags: review?(felash)
I'm unclear about the "[contenteditable] <div> or a <textarea>" discussion, using a contenteditable is really easy and resizing is free: http://jsfiddle.net/rwaldron/8ABu6/

Previously, when I suggested an input or textarea, I didn't realize there were multiline display cases to be considered.
Updated the PR to address latest comments. 

Sorry for the pressure, but deadline is tomorrow (or subject will go to 1.4), so if you are busy I can change the review to Borja, as he have some free cycles.
I started looking at this again yesterday but since we already talked about the big things I think Borja could take the final steps. (and give me some free time ;) )
Comment on attachment 8336834 [details] [review]
pre-patch v1

Hey Borja,

could you help finishing this review?

Thanks!
Attachment #8336834 - Flags: review?(felash) → review?(borja.bugzilla)
Ok! I'll take a look right now! :)
Attachment #8336834 - Flags: review?(waldron.rick)
Attachment #8336834 - Flags: review?(borja.bugzilla)
Attachment #8336834 - Flags: review+
Let's wait to Travis and merge!
I don't want to be a killjoy, but you need approval from Fabrice before merging.
So let's ask for it in a humble way :D
Do we need any other flag or a ni is enough?
Flags: needinfo?(fabrice)
The best way is to raise an approval flag on the patch, and filling in the form, so that Fabrice can take the decision. eg: safety of the patch, was it well tested, user impact, etc.
(In reply to Julien Wajsberg [:julienw] from comment #72)
> The best way is to raise an approval flag on the patch, and filling in the
> form, so that Fabrice can take the decision. eg: safety of the patch, was it
> well tested, user impact, etc.

Yep. I would appreciate some rationale on why this needs to go in. How risky is this, do we have automated tests, smoketests?
Flags: needinfo?(fabrice)
Comment on attachment 8336834 [details] [review]
pre-patch v1

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 #): MMS Subject user story commited for 1.3 (bug #919966)
[User impact] if declined: user won't be able to send MMS with a subject
[Testing completed]: added unit tests and checked both on desktop and device
[Risk to taking this patch] (and alternatives if risky): medium risk, as we are messing with message composing. alternative is not using a subject
[String changes made]: added necessary strings for warn user and errors (4 added)
Attachment #8336834 - Flags: approval-gaia-v1.3?(fabrice)
Attachment #8336834 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
r+ approval+ and travis green. Can anyone merge it? Apparently I don't have rights to do so.
landed in gaia
https://github.com/fcampo/gaia/commit/ee3fe8c84cd5edfc2061510409039b7858620051
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 948346
is this uplifted to v1.3?
cannot find the  status-b2g-v1.2 flag yet so therefore ni?
thanks
Flags: needinfo?(gchen)
Seems these files change have been already on v1.3 branch, so no need to uplift any more.
You need to log in before you can comment on or make changes to this bug.