1. Title : Possible to compose an SMS containing more than 10 segments. 2. Precondition : Composing of a new message should be possible. 3. Tester's Action : Message-New message-enter message containing more than 10 segments-still send button is enabled 4. Detailed Symptom (ENG.) : The send button is enabled even when the maximum segments reached. 5. Expected :The send button should be disabled when the maximum segments reached. 6.Reproducibility: Y 1)Frequency Rate : 100% 7.Gaia Revision: f46757775e07d314913b4a5ae975fa131d1c7c4b

New pull request is raised with the modified file thread_ui.js

Thanks! I will review it asap.

not blocking leo, but tracking b2g. needinfo ayman; to comment if this is a issue for UX.

(In reply to Tony Chung [:tchung] from comment #4) > not blocking leo, but tracking b2g. needinfo ayman; to comment if this is a > issue for UX. I would like Borja's input on this before give a definitive answer. This is because my understanding at the moment is that when an SMS overruns its segment limit it becomes an MMS. If this is so then disabling the send button when the maximum limit of SMS segments is reached would not be appropriate.. So RIF to Borja to clarify what happens when the segment limit of an SMS is reached.

There's no MMS for 1.0.1, so does this need to land for that release? Once MMS lands, this patch should be reverted, yes?

OK thanks for clarifying that this is for 1.0.1 Dietrich. In this case the Send button should not be disabled when the user reaches the maximum number of segments. What should happen is: - The message input field does not accept any more content from the keyboard so that the messages cannot be added to - A message is delivered to the user via the banner that informs them when they cross from one SMS segment to another informing them that they have reach the maximum message length. - the send button remains active so that they can send the SMS Let me know if you need any more information.

Nominating for tef? in that case since this should only be an issue where there is no MMS.

Ayman, displaying a message, as you suggested, will bring in l10n changes. I'd be happy if you had another suggestion. If you still want to display a message, then please propose the message, and where exactly you want it. Here is my proposition : "You reached the maximum length that can be sent in one message." But I don't know where it could be displayed. In this situation, the input area takes all the available space, so the easiest could be to have it displayed over the input area, at the top of it.

stealing this

WIP - start test - first version of code (untested) --- apps/sms/index.html | 4 ++ apps/sms/js/thread_ui.js | 8 ++- apps/sms/locales/sms.en-US.properties | 1 + apps/sms/style/sms.css | 17 ++++++ apps/sms/test/unit/mock_utils.js | 13 +++++ apps/sms/test/unit/mocks_helper.js | 58 +++++++++++++++++++ apps/sms/test/unit/setup.js | 5 ++ apps/sms/test/unit/thread_ui_test.js | 102 +++++++++++++++++++++++++++++++++ 8 files changed, 207 insertions(+), 1 deletion(-) create mode 100644 apps/sms/test/unit/mock_utils.js create mode 100644 apps/sms/test/unit/mocks_helper.js create mode 100644 apps/sms/test/unit/setup.js create mode 100644 apps/sms/test/unit/thread_ui_test.js (see also PR https://github.com/mozilla-b2g/gaia/pull/9154)

(In reply to Julien Wajsberg [:julienw] from comment #10) > Ayman, displaying a message, as you suggested, will bring in l10n changes. > I'd be happy if you had another suggestion. > > If you still want to display a message, then please propose the message, and > where exactly you want it. > > Here is my proposition : > "You reached the maximum length that can be sent in one message." > > But I don't know where it could be displayed. In this situation, the input > area takes all the available space, so the easiest could be to have it > displayed over the input area, at the top of it. Hi Julien Referencing the original SMS wireframes document: HTML5_SMS_20121212_R2S1_V8.0, page 17, annotation 02. It was specified that a banner message would be displayed every time the user crossed the boundary between SMS packets in inform them that they have done so. I would suggest that this banner message is user to inform the user they have reached the maximum length of SMS. I would suggest that 'max message length' would be an appropriate message.

(In reply to ayman maat :maat from comment #13) > (In reply to Julien Wajsberg [:julienw] from comment #10) > > Ayman, displaying a message, as you suggested, will bring in l10n changes. > > I'd be happy if you had another suggestion. > > > > If you still want to display a message, then please propose the message, and > > where exactly you want it. > > > > Here is my proposition : > > "You reached the maximum length that can be sent in one message." > > > > But I don't know where it could be displayed. In this situation, the input > > area takes all the available space, so the easiest could be to have it > > displayed over the input area, at the top of it. > > > Hi Julien > > Referencing the original SMS wireframes document: > HTML5_SMS_20121212_R2S1_V8.0, page 17, annotation 02. sorry, typo: i meant annotation 01, not 02.

Ayman, could you please attach this document to this bug, and also the design document if you can. I especially need the UX design for the banner. Currently I think there is no banner when we cross the boundary. I honestly think this is not so mandatory because we have the counter. But I agree that this must be done for this current bug. But I really don't like "max message length"... I think this is really not very understandable for the user... :/

Hi Ayman, Julien. Why dont we use the 'system' banner/prompt (the same as when you set an alarm)? Thanks!

the clock banner is handled by the clock app itself. Also, I'd like this message to be static and not hide after a few seconds, I think this is less confusing for the user. The message that would be displayed when we change segments could hide after a few seconds, but we should handle this one in a separate bug imho.

Guys, I know we would like to fix it properly, but tef+ counter is still high :) For tihs bug let's just prevent user for entering more characters and open a follow-up for the banner thing?

A preliminary banner is already there, so I'll finish that hopefully today so that we move forward :)

Discussed this with Ayman, so I will continue with the UX I proposed. We only need a short enough, explicit enough text, Maria will provide one.

I've already contacted with Tylor for the sentence, waiting for his feedback. Julien,be aware that we will need l10 revision for it!!

Suggested banner message: "You've reach the maximum length for an SMS."

Suggested banner message: "You've reached the maximum length for an SMS."

(In reply to tyler.altes from comment #22) > Suggested banner message: > > "You've reach the maximum length for an SMS." Hit the send button before noticing the type. Comment 23 is the correct text.

ok thanks a lot !

Add a banner to display that the max message length for an SMS is reached. Added tests for the counter text and the max message length banner. --- apps/sms/index.html | 4 + apps/sms/js/thread_ui.js | 15 +- apps/sms/locales/sms.en-US.properties | 1 + apps/sms/style/sms.css | 17 +++ apps/sms/test/unit/mock_utils.js | 13 ++ apps/sms/test/unit/mocks_helper.js | 58 ++++++++ apps/sms/test/unit/setup.js | 5 + apps/sms/test/unit/thread_ui_test.js | 241 +++++++++++++++++++++++++++++++++ 8 files changed, 351 insertions(+), 3 deletions(-) create mode 100644 apps/sms/test/unit/mock_utils.js create mode 100644 apps/sms/test/unit/mocks_helper.js create mode 100644 apps/sms/test/unit/setup.js create mode 100644 apps/sms/test/unit/thread_ui_test.js see also PR https://github.com/mozilla-b2g/gaia/pull/9154/files Not asking for review because I still need the final visual design.

WIP design

WIP design

NEEDINFO Victoria for having the final visual design, and CCing Steve.

Comment on attachment 738602 [details] [diff] [review] patch v2 I'd like to move forward, so requesting review from Borja. Rick, I'd like a feedback from you as well.

My recommendation from an UX point of view is that since the user cannot continue writing when it reaches the limit, a building block overlay should appear. This having only an OK button to go back to the message. This way we avoid creating new patterns and using consistently the existing ones. Thanks!

We should *really* avoid a string for 1.0.1 at this point. Localizing that 'til the end of April is getting unlikely. Also, Julien, what happens if the localized string is twice as long? Does the string wrap? Does that impact UX' opinion? I'd like to get a copy review, I'm torn on "a SMS" or "an SMS".

Comment on attachment 738602 [details] [diff] [review] patch v2 will provide a new patch later to implement comment 31.

(In reply to Axel Hecht [:Pike] from comment #32) > We should *really* avoid a string for 1.0.1 at this point. Localizing that > 'til the end of April is getting unlikely. > > Also, Julien, what happens if the localized string is twice as long? Does > the string wrap? Does that impact UX' opinion? Now we'll put that in an alert overlay so this is not a concern anymore. Thanks Axel !

see also PR https://github.com/mozilla-b2g/gaia/pull/9154 WIP, this is untested on a device. Also, the tests are failing because of a bug in mocha, I pushed there a pull request : https://github.com/visionmedia/mocha/pull/820 In the mean time we'll ignore that global leak, I'll update the code to do this later.

Use an alert to display that the max message length for an SMS is reached. Added tests for the counter text and the alert overlay --- apps/sms/js/thread_ui.js | 16 +- apps/sms/locales/sms.en-US.properties | 1 + apps/sms/test/unit/mock_alert.js | 10 ++ apps/sms/test/unit/mock_navigatormoz_sms.js | 13 ++ apps/sms/test/unit/mock_utils.js | 13 ++ apps/sms/test/unit/mocks_helper.js | 58 +++++++ apps/sms/test/unit/setup.js | 5 + apps/sms/test/unit/thread_ui_test.js | 241 +++++++++++++++++++++++++++ 8 files changed, 354 insertions(+), 3 deletions(-) create mode 100644 apps/sms/test/unit/mock_alert.js create mode 100644 apps/sms/test/unit/mock_navigatormoz_sms.js create mode 100644 apps/sms/test/unit/mock_utils.js create mode 100644 apps/sms/test/unit/mocks_helper.js create mode 100644 apps/sms/test/unit/setup.js create mode 100644 apps/sms/test/unit/thread_ui_test.js see also PR https://github.com/mozilla-b2g/gaia/pull/9154 To be really awesome this patch needs the patch to Bug 863466 too.

Comment on attachment 739305 [details] [diff] [review] patch v3 asking review from l10n staff too.

Comment on attachment 739305 [details] [diff] [review] patch v3 Review of attachment 739305 [details] [diff] [review]: ----------------------------------------------------------------- r-, for the fear of '#' in keys creating weird unexpected fall-out somewhere. I also still have concerns on the copy. And yes, if we can not take this string for 1.0.1, that's be way better. We've got about a week 'til end of April, and the last string freeze break is half dealt with and half not, which is the worst starting point to talk about new strings. ::: apps/sms/locales/sms.en-US.properties @@ +34,5 @@ > new = New > message = Message > composeMessage.placeholder = Message > newMessage = New message > +messages#max-length-notice = You've reached the maximum length for an SMS. I'd rather not have another special char (#) enter the keys, even though it's probably OK by the spec. But there's a neat variety of things that work here and don't work there. Also, I still think that SMS starts with an 's' and not an 'e', so it should be 'a' and not 'an', but I really want a native copy writer to make the call there. Can't find prior art, at least on 1.0.1.

(In reply to Axel Hecht [:Pike] from comment #38) > Comment on attachment 739305 [details] [diff] [review] > patch v3 > > Review of attachment 739305 [details] [diff] [review]: > ----------------------------------------------------------------- > > r-, for the fear of '#' in keys creating weird unexpected fall-out somewhere. > > I also still have concerns on the copy. > > And yes, if we can not take this string for 1.0.1, that's be way better. > We've got about a week 'til end of April, and the last string freeze break > is half dealt with and half not, which is the worst starting point to talk > about new strings. Who can decide here ? > > ::: apps/sms/locales/sms.en-US.properties > @@ +34,5 @@ > > new = New > > message = Message > > composeMessage.placeholder = Message > > newMessage = New message > > +messages#max-length-notice = You've reached the maximum length for an SMS. > > I'd rather not have another special char (#) enter the keys, even though > it's probably OK by the spec. But there's a neat variety of things that work > here and don't work there. I'll use a dash. > > Also, I still think that SMS starts with an 's' and not an 'e', so it should > be 'a' and not 'an', but I really want a native copy writer to make the call > there. Can't find prior art, at least on 1.0.1. I just checked with a native speaker and he tells me it's "an" in this case, because what's important is the sound "es".

(In reply to Axel Hecht [:Pike] from comment #38) > Comment on attachment 739305 [details] [diff] [review] > And yes, if we can not take this string for 1.0.1, that's be way better. We understand this concern but we have no choice, we must fix this bug and it's the UX input. We know it's late.

fixed the locale key. hopefully the last patch. see also PR https://github.com/mozilla-b2g/gaia/pull/9154

Please add a comment explaining the 'limit' in the input and for me it's R+! Thanks Julien ;)

master: f5a6fc00b3632b79bb2e85a1827a246cb47b311e John, I'll do the merge in v1.0.1 myself but I'd appreciate that you uplift in v1-train if that works without conflict. Thanks !

>> Also, I still think that SMS starts with an 's' and not an 'e', so it should >> be 'a' and not 'an', but I really want a native copy writer to make the call >> there. Can't find prior art, at least on 1.0.1. >I just checked with a native speaker and he tells me it's "an" in this case, >because what's important is the sound "es". Exactly. The a/an is based on the next word beginning with a vowel sound, not necessarily a vowel.

this commit looks break unit test. > 1) [sms] SMS App Unit-Test Messages given a thread Thread-messages rendering (bubbles view) Check input form & send button: > TypeError: smsInfo is null > at thui_updateCount (http://sms.gaiamobile.org:8080/js/thread_ui.js:239) > at thui_enableSend (http://sms.gaiamobile.org:8080/js/thread_ui.js:214) > at (anonymous) (http://sms.gaiamobile.org:8080/test/unit/sms_test.js:443) > at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:62) > at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709) > at runTest (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4081) > at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4127) > at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4007) > at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4016) > at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3964) > at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3984) > at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4932) > > > 2) [sms] SMS App Unit-Test Messages given a thread "before each" hook: > TypeError: smsInfo is null > at thui_updateCount (http://sms.gaiamobile.org:8080/js/thread_ui.js:239) > at thui_enableSend (http://sms.gaiamobile.org:8080/js/thread_ui.js:214) > at thui_cleanFields (http://sms.gaiamobile.org:8080/js/thread_ui.js:801) > at thui_initializeRendering (http://sms.gaiamobile.org:8080/js/thread_ui.js:459) > at thui_renderMessages (http://sms.gaiamobile.org:8080/js/thread_ui.js:489) > at (anonymous) (http://sms.gaiamobile.org:8080/test/unit/sms_test.js:417) > at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:60) > at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709) > at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3973) > at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3984) > at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4932)

Julien can you uplift this one yourself to both branches? The automagical script cannot process that kind of request.

Yuren, it looks like a transient error, because I didn't have this error before merging, then I got it too and then it disappeared. I'll take a look though. James, I'll do it.

v1-train: 273a20e9515a215d2691e4e22748784dab7324fa

v1.0.1: f15358e2e5a6e5541f524565408834d1fda38a61

Yuren, ok, it's not transient after all. I know exactly what caused this and I'll modify the test to fix this in Bug 863776. Sorry about that...

FYI I filed Bug 864178 so that changing one unit test could not make another unit test fail, no matter if one is badly written. A patch is ready in Bug 863776 to fix the unit test breakage too, it was a mix of bad clean up and conflicting mocks.

hi guys I am sorry but we need to back this patch out and reopen this bug. As discussed last week and also stated in #comment 7 and 13 the message informing the user that they have reached the maximum number of segments needs to be delivered in a banner, not in a full screen overlay. The full screen overlay is problematic as it presents a barrier to usage, for example covering essential CTA's that the user might want to engage in such as the 'Send' button example of the banner in use can be viewed in wireframe pack HTML5_SMS-MMSUserStorySpecifications_20130418_V5.0 on page 42, 45 and 46 https://www.dropbox.com/s/sm6z4a8374i7eb8/HTML5_SMS-MMSUserStorySpecifications_20130418_V5.0.pdf full specification will be contained in V6.0 which will be released later today R.F.I. to steve P to provide Julien with the necessary visual designs that he needs as a matter of urgency so that he can implement this fix correctly.

I'm not sure if I should back out the code now, or wait until I have all the information to do this properly. I'll wait for now.

I'd suggest we close this bug and open a follow-up one for the visual/interaction changes.

Daniel, basically, we need to completely remove this patch and make a new patch based on my patch v2. That's why it's probably easier to back out from all branches before landing the new patch. But this can be done in a follow-up patch for sure.

Should the status-b2g18 and v1.0.1 flags be set from fixed to affected, too?

let's close this one and open a follow-up bug to be cleaner about this. Ayman, can you open a follow-up bug instead?

(In reply to Daniel Coloma:dcoloma from comment #57) > let's close this one and open a follow-up bug to be cleaner about this. > Ayman, can you open a follow-up bug instead? sure Daniel, I have opened: https://bugzilla.mozilla.org/show_bug.cgi?id=865411 ...and assigned it TEF? (please correct if its the wrong flag)

Tested on an Inari with the following build information : Build ID : 20130422054646 Git Commit Info : 2013-04-21 21:19:35 Gaia Repo : 55f009ff476687958decea8d9cc15791... (ellipsis at the end) The above issue is re-occurring; 'Send' button is enabled even after reaching maximum number of message segments (more than 10)

sreeidhi> do you have a screenshot please ? could this be because of Bug 859819 ?

sreenidhi> btw now we don't disable the send button, but we forbid the user from entering more characters, see the UX requirement in comment 7.

Julien -> I guess I'm able to type in more than ten segments

SMS with 11 segments and going on.

SMS with 13 segments and going on. The 'Send' button seems to be grayed out/disabled, but when I tap on that button, it functions normally.

Sreenidhi: this seems bad ;) Do you remember if you saw the 0/10 counter ? That's what should kick in the limitation. If you see this and there is no limitation, this is definitely strange. However I admit I completely forgot the case where could skipped to 11 segment without going to 0 first (eg: when switching from 140-character SMS to 60-character SMS). I will take care of this in Bug 865411.

The user is able to see the limitation of text banner after 10 segments. Tapping 'ok' on the banner, the user is still able to add more text to the SMS till 14 segments with 'send' button enabled. This issue reproduces on Leo device. Build ID: 20130426070204 Kernel Date: Mar 15 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6c2493de1441 Gaia: c9046a7acef33328977840176ff5574720d2c74c

Tested on an Inari with the following build information : Gecko http://hg.mozilla.org/releases/mozilla-b2g18/rev/6c2493de1441 Gaia c9046a7acef33328977840176ff5574720d2c74c BuildID 20130426070204 Version 18.0 Seems to work fine now, getting the "reached max segments" message. Did not get this message earlier.

Thanks sreenidhi and deepa, seems like the issue is not properly fixed, will test better in Bug 865411.

The sms text banner is not allowing the user to enter more characters once after it reaches the limit(10 segments).Works fine Seems issue is no longer valid on Leo and Inari Leo Build ID: 20130515070208 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/d06cfe7d67c2 Gaia: 0ddb515f15cbc6b74fc2742b7599d6ae74c6413f Inari Build ID: 20130515070203 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0b6bcb1f4175 Gaia: 9648799c2e45917ff150fa9eef8aeac79a9ac008