865411 - [SMS] Banner required to hold message when user reaches the Max Segments in SMS composer

Status: VERIFIED FIXED

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

Description

[ayman maat :maat]

Attachment: HTML5_SMS-MMSUserStorySpecifications_20130424_V6.0

Referencing..
Wireframe pack : HTML5_SMS-MMSUserStorySpecifications_20130424_V6.0
Page : 45

Banner required from Visual Design to hold the message the user sees when they reaches the Max Segments in SMS composer 

Referenced wireframe pack attached.
also reference bug: https://bugzilla.mozilla.org/show_bug.cgi?id=843511

Comment 1

[ayman maat :maat]

RFI to Steve P to provide the necessary visual design components for Juilen to close this bug

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking for now, since this seems to go hand in hand with the other tef+ bug 843511, if disabling the send button is enough user feedback though - let's remove the block on this.

Comment 3

[Julien Wajsberg [:julienw]]

Lukas, in bug 843511 we actually did more than merely disabling the send button: we display an alert box. This is a huge user feedback, and probably too huge, hence this bug.

Comment 4

[ayman maat :maat]

(In reply to [e-mail] from comment #2)
> Blocking for now, since this seems to go hand in hand with the other tef+
> bug 843511, if disabling the send button is enough user feedback though -
> let's remove the block on this.

Disabling the send button is absolutely not the right thing to do for 2 reasons:

1) its not enough feedback for the user as they have no idea why the send button is disabled.
2) **and this is the most important point**  ...the user should be able to send a message that is at its maximum length... so disabling the send button at this point is inappropriate. What is appropriate is to prevent the message field from receiving any more the input at the point where the user reaches the max message length so that the user cannot continue to add text content to a message that cannot be sent. (i.e. we stop the error - which is 'message too long to send' - before it happens) When we prevent the user from inputting more text into the text field, they need to be notified why... and this is why this banner is essential.

There is also slightly more to this banner.

referencing the attached wireframe specification..
The pattern of banner is also required for instances:
1) page 42 Successfully saving attachment from inside a thread
2) page 47, 48, 51 Conversation of SMS to MMS and converting from MMS to SMS

referencing the SMS wireframe specification HTML5_SMS_20121212_R2S1_V8.0..
The pattern of banner is also required for this instance:
3) page 17 notification when the user passes between SMS packets
(this is currently being brought over into the new specifications)

Visual Design for the banner already exists in the email app. It is used to inform the user (for example) when a message has been deleted. All that is required is for this banner to be moved up the screen into the specified position. I therefore do not perceive a big effort at all for Visual Design to complete this task..

Comment 5

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Lukas, in bug 843511 we actually did more than merely disabling the send
> button: we display an alert box. This is a huge user feedback, and probably
> too huge, hence this bug.

He julien. Yes it is too big and also presents a barrier to usage... as like i say. it should be the message field that can receive no further text, not the ability to send denied.

Comment 6

[Julien Wajsberg [:julienw]]

yep we must never disable the send button, and we don't.

I wanted to explain correctly the current state because I'm really not sure at all that this must be a blocker now. even if it could, I don't mind, but this is a product decision and Lukas comment implies that he didn't get the whole picture.

Comment 7

[Julien Wajsberg [:julienw]]

I mean that I think it is not more important than some Leo+ work we're doing, like MMS stuff. that's why I'm not sure it should be TEF+. but Ayman if you still think this should be then let's go !

Comment 8

[Julien Wajsberg [:julienw]]

Ayman, there is one case you didn't designed. What if the user is in a case where there are more than 10 segments. Should we keep the same banner + disable the send button ? Should we have a different banner ? My preference go to having the same banner so that we don't have another l10n change for v1.0.1.

This can happen when switching from 140-character SMS to 70-character SMS because the user inserted some "wrong" character.

Comment 9

[Victoria Gerchinhoren [:vicky]]

Hi Julien,

I am attaching the design for the in APP message banner. It is a new element that is not used anywhere in the OS, but as Ayman provided specs for various cases where it's going to be used, so this shu¡ould be a Building Blocks and i am creating a bug to include it as a common element. 

I've created this bug: 867173, to solve it.

Thanks!

Comment 10

[Victoria Gerchinhoren [:vicky]]

Hi Julien,

I am attaching the design for the in APP message banner. It is a new element that is not used anywhere in the OS, but as Ayman provided specs for various cases where it's going to be used, so this shu¡ould be a Building Blocks and i am creating a bug to include it as a common element. 

I've created this bug: 867173, to solve it.

Thanks!

Comment 11

[Victoria Gerchinhoren [:vicky]]

Attachment: Shows the visual solution for this need

Comment 12

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Ayman, there is one case you didn't designed. What if the user is in a case
> where there are more than 10 segments. Should we keep the same banner +
> disable the send button ? Should we have a different banner ? My preference
> go to having the same banner so that we don't have another l10n change for
> v1.0.1.


Hi Juilen. I am not fully understanding how the following case you outlined is triggered and how it affects the number of segments in the SMS message:

> This can happen when switching from 140-character SMS to 70-character SMS
> because the user inserted some "wrong" character.

Could you give me more detail please. Thanks

Comment 13

[Julien Wajsberg [:julienw]]

Ayman, depending on the characters that are in an SMS, it can use different encodings. Depending on these encodings, one segment can contain either 160, 140 or 70 characters. So when you switch from one encoding to another (because eg you entered a character that does not fit the previous encoding, I like using "ê" for example, even if doesn't always trigger the 70-characters encoding), we might double the number of segments with only one more character.

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: patch v1

* implement the notification according to UX and Visual design
* display this notification if we reach the max length
* disable the send button if over 10 segments, but keep it enabled if we have
  exactly the max length
* rewrote the enableSend function to make it more readable and manageable
* use the new markup loader in thread_ui_test.js, and make it global
---
 apps/sms/index.html                  |    7 +
 apps/sms/js/startup.js               |    3 +-
 apps/sms/js/thread_ui.js             |   51 +++++---
 apps/sms/style/images/pattern.png    |  Bin 0 -> 6851 bytes
 apps/sms/style/notification.css      |   23 ++++
 apps/sms/style/sms.css               |    2 +-
 apps/sms/test/unit/setup.js          |    2 +-
 apps/sms/test/unit/sms_test.js       |    1 -
 apps/sms/test/unit/thread_ui_test.js |  240 +++++++++++++++++++++++-----------
 9 files changed, 230 insertions(+), 99 deletions(-)
 create mode 100644 apps/sms/style/images/pattern.png
 create mode 100644 apps/sms/style/notification.css

(see also PR https://github.com/mozilla-b2g/gaia/pull/9515)

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: (screenshot) Reaching the max length without "skipping" it

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: (screenshot) Reaching the max length "skipping" it

we can skip it by modifying the encoding (eg with the character ě that you can have on some keyboard layouts)

Comment 17

[Julien Wajsberg [:julienw]]

(note that the counters show 2 or 3 segments on the screenshots because I changed the limit while testing)

Comment 18

[Julien Wajsberg [:julienw]]

Attachment: patch v2

sorry, the previous patch was breaking the unit tests.

So here is the new one with the green unit tests.

same PR https://github.com/mozilla-b2g/gaia/pull/9515

Comment 19

[Borja Salguero [:borjasalguero]]

Your patch looks great. Let me check in my device. Could you deliver the PR meanwhile? Thanks!

Comment 20

[Julien Wajsberg [:julienw]]

The PR is already there borja ;) see comment 18

Comment 21

[Julien Wajsberg [:julienw]]

borja, any chance to r+ this today ?

Comment 22

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> Ayman, depending on the characters that are in an SMS, it can use different
> encodings. Depending on these encodings, one segment can contain either 160,
> 140 or 70 characters. So when you switch from one encoding to another
> (because eg you entered a character that does not fit the previous encoding,
> I like using "ê" for example, even if doesn't always trigger the
> 70-characters encoding), we might double the number of segments with only
> one more character.

Hey Julien

Thanks for bringing this to my attention. Regarding characters having a different encoding I had no idea that this happened. However my current understanding is that if a user make an action that crosses the SMS limit (so lets say adds 131th character when the limit is 130 or adds a country specific character such as ä which changes the number of segments available) we convert the SMS to MMS at which point segments are no longer in play so we would not have this issue. This is what i see happening on the android device I am referring to.

However I am going to RFI Daniel to be sure of this before I specify.

Comment 23

[Julien Wajsberg [:julienw]]

Ayman> that will happen on 1.1 but remember this bug is about 1.0.1 too.

My current plan was to land this quickly on master/v1-train/v1.0.1 before we change how the mms composition work that will invalidate most of this. Unfortunately I think this won't happen because I'd like to land the mms composition work today and this bug was not reviewed today. So I think I'll have to make a specific 1.0.1 patch.

Comment 24

[ayman maat :maat]

RFI to Tyler to look at the copy of the banner.

Tyler I would like the message delivered to be only a single line. refer to attachment 744749 [details] [diff] [review] for current text being used.

thanks

Comment 25

[Julien Wajsberg [:julienw]]

ayman> maybe we can ask Arnau if we can lower the font size too.

Comment 26

[Julien Wajsberg [:julienw]]

tyler, see comment 24. I think you gave this sentence at first.

Comment 27

[Julien Wajsberg [:julienw]]

arnau, please see comment 25, there is a question for you :)

Comment 28

[tyler.altes]

Julien, if we're needing a simple, one-line, translatable message for this, I would just put: "Character limit reached"

Comment 29

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Julien, I think is better if you fine tune font-size with Victoria.

Comment 30

[Julien Wajsberg [:julienw]]

I think the new proposition from Tyler is indeed quite "unfriendly" because too short, I do favor a smaller font-size or even a 2-line notification...

Victoria, Ayman, I'm waiting for your decision now.

Comment 31

[tyler.altes]

Agreed. If there is an option to adjust font size or distribution to make space for a friendlier message, it would be a big improvement. 

Also, we can remove "for an SMS." from the message to make it shorter -- it should be more than obvious that we're talking about the length of the current SMS message. So it would just be:

"You've reached the maximum length."

It doesn't fit on one line in the current font size, but should be easier to adjust for.

Comment 32

[Julien Wajsberg [:julienw]]

Yeah I like that, thanks :)
especially that we'll have MMS soon, so the same message works in that case too !

Comment 33

[Julien Wajsberg [:julienw]]

Attachment: patch v3

see also PR https://github.com/mozilla-b2g/gaia/pull/9515/files

I reduced the font-size and changed the string. I made the font-size smaller by 0.1rem than what was necessary because I thought that in some languages (eg: french, spanish) the text would probably be longer. Victoria, Arnau, maybe we can also reduce the margin, and make the font-size bigger.

About the string change, if UX is ok with that, we could keep the old string for 1.0.1, and the text would be displayed on 2 lines. Ayman, would it be good enough for 1.0.1 ? That would keep our l10n-drivers not mad ;-)

Comment 34

[Julien Wajsberg [:julienw]]

Attachment: new screenshot with reduced font-size

Comment 35

[Julien Wajsberg [:julienw]]

Attachment: screenshot experimenting with margin

reduced the margin, increased the font size

Comment 36

[Julien Wajsberg [:julienw]]

Please tell me what you prefer here and I can update my patch accordingly to your preferences.

Also, this won't work correctly on master in a "new thread" mode, until the patch for Bug 861227 lands. Borja, do you think I should do here some of the changes you did in Bug 861227 (mainly: moving the "to" container outside of the header + fiddling with z-indexes) ? It will probably need an ad hoc patch for v1.0.1 too.

Comment 37

[Victoria Gerchinhoren [:vicky]]

Hi Julien, 
Please go with the last option, the reduced font size and smaller margin and please make sure the margin is 30px so if we change it, it is consistent with other margins through the UI.

Thanks!

Comment 38

[ayman maat :maat]

for font size i defer to Victoria for copy I defer to Julien. From a UX perspective my only request is that the message is one line.

Comment 39

[ayman maat :maat]

(In reply to ayman maat :maat from comment #38)
> for font size i defer to Victoria for copy I defer to Julien. From a UX
> perspective my only request is that the message is one line.

sorry typo - i mean:

for font size i defer to Victoria for copy I defer to *Tyler*. From a UX perspective my only request is that the message is one line.

Comment 40

[Julien Wajsberg [:julienw]]

Ayman, is this request true for both v1.1 and v1.0.1 ? As explained previously, this will need a string change, and I'm reluctant to this in v1.0.1...

Comment 41

[Alex Keybl [:akeybl]]

We'll reconsider this bug for landing on v1.0.1 if it doesn't hit before 5/10.

Comment 42

[Julien Wajsberg [:julienw]]

(In reply to Victoria Gerchinhoren from comment #37)
> Hi Julien, 
> Please go with the last option, the reduced font size and smaller margin and
> please make sure the margin is 30px so if we change it, it is consistent
> with other margins through the UI.

Victoria, in https://bugzilla.mozilla.org/attachment.cgi?id=745870 I experimented with a margin of 20px which lets me having a bigger font size.

Could you please tell me what you prefer between https://bugzilla.mozilla.org/attachment.cgi?id=745870 and https://bugzilla.mozilla.org/attachment.cgi?id=745867.

Comment 43

[ayman maat :maat]

Attachment: HTML5_SMS-MMSUserStorySpecifications_length exceded_20130506_V0.2

(In reply to Julien Wajsberg [:julienw] from comment #40)
> Ayman, is this request true for both v1.1 and v1.0.1 ? As explained
> previously, this will need a string change, and I'm reluctant to this in
> v1.0.1...

Ok guys. Had a conversation with Julien and am updating this bug with the conclusion for documentation and clarity.

We have two situations that we need to cover with the banner: 

1) where the user reaches the limit of a message by 'natural' means. i.e. the maximum message length is 300 characters, the user has input 299 characters and up on inputting the 300th character reaches the limits of the message length. This is covered in tyler's #comment 31. This is the one i would like to see as a single line if possible as it will also exist in V1.1

2) where the user can exceed the limit of a message by including a diacritic character: So for example the maximum message length is 300 characters and the user has input 275 characters meaning that they have 25 characters left to input. The user then adds a diacritic character, like ü as the 276th character. The inclusion of this character switches the encoding of the message which reduces its maximum length from 300 characters to 250 characters. This results in the user now being in a state whereby they have exceeded the maximum length of the message. (Julien correct me if i am incorrect in this summary). Therefore we are now in state whereby we have to communicate to the user why they are suddenly, and unexpectedly over their limit and how to recover from this.

I am attaching document 'HTML5_SMS-MMSUserStorySpecifications_length exceded_20130506_V0.2' to outline the solution to point 2 above so that UX does not block the progress of dev on this bug. This wireframe will be included in the next version of the MMS/SMS wireframe pack to be released (v9.0)

Comment 44

[Victoria Gerchinhoren [:vicky]]

Sorry Julien, I realise that 30px was the previous one. Thanks for your try outs, very useful, let's go for this one:
https://bug865411.bugzilla.mozilla.org/attachment.cgi?id=745870 as it will line up with the left divider in the header, so it's still sticking to a main layout.

Thanks!

Comment 45

[Julien Wajsberg [:julienw]]

Attachment: screenshot with the second message from maat

Comment 46

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #45)
> Created attachment 746121 [details]
> screenshot with the second message from maat

could we put the 'please' on the next line like this:

This character is overweight and 
requires more space than others.
Please remove it to sent his message

apart from that it is fine by me...

Comment 47

[Victoria Gerchinhoren [:vicky]]

 (In reply to Julien Wajsberg [:julienw] from comment #45)
> Created attachment 746121 [details]
> screenshot with the second message from maat

Julien, the line height seems a bit large to me, can you make sure the line hight is 2.2 for 1.7 rem? And if the text is 1.9 rem the line height should be 2.5 rems

Reference: https://docs.google.com/spreadsheet/ccc?key=0Ari8z5WN5YqndFJQOHpIMjZyQzFkejYxcW9jRUtLY2c#gid=0

Comment 48

[Julien Wajsberg [:julienw]]

Attachment: new thread screenshot with the 1-line notification

Comment 49

[Julien Wajsberg [:julienw]]

Attachment: new thread screenshot with the special character notification

Comment 50

[Julien Wajsberg [:julienw]]

Attachment: patch v4

* implement the notification according to UX and Visual design
* display this notification if we reach the max length
* disable the send button if over 10 segments, but keep it enabled if we have
  exactly the max length
* rewrote the enableSend function to make it more readable and manageable
* use the new markup loader in thread_ui_test.js, and make it global
---
 apps/sms/index.html                   |   15 +-
 apps/sms/js/startup.js                |    3 +-
 apps/sms/js/thread_ui.js              |   54 ++++---
 apps/sms/locales/sms.en-US.properties |    3 +-
 apps/sms/style/common.css             |    5 +-
 apps/sms/style/images/pattern.png     |  Bin 0 -> 6851 bytes
 apps/sms/style/notification.css       |   21 +++
 apps/sms/style/sms.css                |   15 +-
 apps/sms/test/unit/setup.js           |    2 +-
 apps/sms/test/unit/sms_test.js        |    6 +-
 apps/sms/test/unit/thread_ui_test.js  |  250 +++++++++++++++++++++++----------
 11 files changed, 266 insertions(+), 108 deletions(-)
 create mode 100644 apps/sms/style/images/pattern.png
 create mode 100644 apps/sms/style/notification.css

see also PR https://github.com/mozilla-b2g/gaia/pull/9515

Comment 51

[tyler.altes]

Two comments:

1. Every phone I've used has the same pattern with special characters reducing the character limit of a message (even old Nokia's), and I've never seen one that explains this to users. It has always seems very intuitive/obvious. 

Why do we feel that it is necessary to explain this?


2. If we decide it is necessary to explain, the "overweight" text should be changed. I suggest:

"You've exceeded the maximum length. Remove special characters or shorten the message to send it."

Comment 52

[Julien Wajsberg [:julienw]]

(In reply to tyler.altes from comment #51)
> Two comments:
> 
> 1. Every phone I've used has the same pattern with special characters
> reducing the character limit of a message (even old Nokia's), and I've never
> seen one that explains this to users. It has always seems very
> intuitive/obvious. 
> 
> Why do we feel that it is necessary to explain this?

Well, to me, it has never seemed obvious at all.

But here we're explaining specifically because we disable the send button.

> 
> 
> 2. If we decide it is necessary to explain, the "overweight" text should be
> changed. I suggest:
> 
> "You've exceeded the maximum length. Remove special characters or shorten
> the message to send it."

ok, thanks a lot

(In reply to Victoria Gerchinhoren from comment #47)
>  (In reply to Julien Wajsberg [:julienw] from comment #45)
> > Created attachment 746121 [details]
> > screenshot with the second message from maat
> 
> Julien, the line height seems a bit large to me, can you make sure the line
> hight is 2.2 for 1.7 rem? And if the text is 1.9 rem the line height should
> be 2.5 rems

the text is 1.5rem and the line height is 2.2rem. I'll change to 2rem as your reference doesn't mention what it should be at this size, so picked one inbetween.

thanks

Comment 53

[Julien Wajsberg [:julienw]]

Attachment: patch v5

ok, fixed the last things from Victoria, Tyler and Ayman.

Note that if that is not reviewed before 5pm CET today I won't be able to work on this before next monday and therefore it won't likely go to 1.0.1 and therefore it will likely be just lost because the MMS stuff obsoletes all this. I sincerely hope we can at last go forward and land this today.

Thanks

Comment 54

[Julien Wajsberg [:julienw]]

Github PR is still in https://github.com/mozilla-b2g/gaia/pull/9515.

Comment 55

[Julien Wajsberg [:julienw]]

Comment on attachment 746320 [details] [diff] [review]
patch v5

requesting review from l10n.

Sorry about that guys, I tried to not change the string but UX wanted too.

Comment 56

[Axel Hecht [:Pike]]

Comment on attachment 746320 [details] [diff] [review]
patch v5

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

I have two nits that made the patch harder to review, the non-localizable string, and that the keys that do localize it don't show up in the code.

With that, f+=me.

::: apps/sms/index.html
@@ +117,5 @@
> +                  role='notification'
> +                  class='hide'>
> +            <p>
> +              You've reached the maximum length.
> +            </p>

I'm wondering if you should start with this <p> being empty, or adding a data-l10n-id?

::: apps/sms/js/thread_ui.js
@@ +470,5 @@
>  
> +    if (hasMaxLength || exceededMaxLength) {
> +      this.input.setAttribute('maxlength', value.length);
> +      var key = hasMaxLength ? 'max' : 'exceeded';
> +      var message = navigator.mozL10n.get('messages-' + key + '-length-text');

Can you make the keys show up verbatim in the patch here? It confused the heck out of me, and it seems like it's not adding any real value to the code flow.

Comment 57

[Julien Wajsberg [:julienw]]

Attachment: patch v6

fixes to pike's comments. Thanks Pike, carrying the f+ from you.

Still the PR in https://github.com/mozilla-b2g/gaia/pull/9515

Comment 58

[Julien Wajsberg [:julienw]]

Attachment: screenshot with the single line in the existing thread view

Comment 59

[Julien Wajsberg [:julienw]]

Attachment: screenshot with the special character line in the existing thread view

Comment 60

[Julien Wajsberg [:julienw]]

Attachment: screenshot with the single line in the new thread view

Comment 61

[Julien Wajsberg [:julienw]]

Attachment: screenshot with the special character line in the new thread view

Comment 62

[Julien Wajsberg [:julienw]]

needinfo maat and vpg to check that the screenshots look good.

Anyway if you don't check them before 5pm CET I won't be able to change anything and this won't land. So it'd better be ok.

Comment 63

[Victoria Gerchinhoren [:vicky]]

Looks good for me Julien.

Comment 64

[Borja Salguero [:borjasalguero]]

Comment on attachment 746361 [details] [diff] [review]
patch v6

The code in the PR is working as expected. There are several issues that will be fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=861227, so we are going to try to land all this today. Thanks Julien!

Comment 65

[Julien Wajsberg [:julienw]]

master: a64ee1c346e71d7ff64904fcfe7dfbd834f1877d

thanks

Comment 66

[Julien Wajsberg [:julienw]]

v1-train: 90c3cdc4e0fbab5960ae5ea80ab32ddf3993f42a

Comment 67

[Julien Wajsberg [:julienw]]

V1.0.1: 89a9c5de7313dfd1095d0ff46988bc91a35cbe9c

Comment 68

[Dan Heberden]

This is currently not working with all of the new layout patches and #840069. Please see http://github.com/bocoup/gaia/tree/messaging for all of the in-progress code for MMS.

Comment 69

[Dan Heberden]

My apologies; this is specific to in-dev work on MMS; please see bug https://bugzilla.mozilla.org/show_bug.cgi?id=870120

Comment 70

[Angela Hubenya]

Issue is Verified as Fixed. 
The banners are displayed as they are displayed in attachments in comments 58-61 when the user maxes out segments in SMS composer. Tested the new message option as well as when replying. 

Unagi, Build ID: 20130520070208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/dbecb0c45504
Gaia: ee70d7517689116622c5125ce33e56d46dd3c948

Unagi, Build ID: 20130520070207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/321d5b4db55a
Gaia: b161f42c5647b37e35ba5a20f394ba40bf674d9c