990481 - [ZTE][OPENC]MMS app allows to attach more contents even when the max MMS size is reached

Status: RESOLVED FIXED

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

Description

[RuiLi]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.154 Safari/537.36

Steps to reproduce:

Open message app.
Attach images until 300KB is reached.
Then continue to attach an image from gallery.


Actual results:

The image is added into message, even though messages-exceeded-length-text shows.


Expected results:

The image should not be allowed to attach any more when message size reach the maximum length.

Comment 1

[Julien Wajsberg [:julienw]]

Images are automatically resized, how many images do you add to make room for 300 KB?

That said, I can reproduce with videos (that are not resized). I think it's a regression from bug 923023.

Not sure it should block though.

Comment 2

[RuiLi]

Yes, images can be resized automatically. I add 6 images making it reaching 300kb. And then I can add more images continually.

Comment 3

[Jason Smith [:jsmith]]

This is a dupe.

Comment 5

[RuiLi]

It seems this issue different from 988768. bug 988768 describes such a case that cannot send a message. This bug is only related to UI instead of sending. It's expected when maximum size reached while composing, no attachment is allowed to add any more.

Comment 6

[Jason Smith [:jsmith]]

(In reply to RuiLi from comment #5)
> It seems this issue different from 988768. bug 988768 describes such a case
> that cannot send a message. This bug is only related to UI instead of
> sending. It's expected when maximum size reached while composing, no
> attachment is allowed to add any more.

The root way to solve bug 988768 is the same here - they both deal with attachments with a size larger than 300 KB and how to do error handling.

Comment 7

[Julien Wajsberg [:julienw]]

This is different.

 Bug 988768 will likely end up being either resolved wontfix (tor the operator to change their configuration) or us changing the default limit.

Here we have an issue that the button to attach an attachment is not disabled. This was the case in 1.1 so it's a regression. Definitely not the same issue!

Comment 8

[Preeti Raghunath(:Preeti)]

QA,

Please check for max capacity of MMS and error message in the preview

Comment 9

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> QA,
> 
> Please check for max capacity of MMS and error message in the preview

Specifically - we're looking to know that if you try to attach an image/recording/etc. that is larger than 300 KB, is there an error present saying you've passed the max size for the MMS?

Comment 10

[Harvey]

> Specifically - we're looking to know that if you try to attach an
> image/recording/etc. that is larger than 300 KB, is there an error present
> saying you've passed the max size for the MMS?

Selecting to attach ONE file that is larger than 300kb brings up a message screen that reads "The file you have selected is too large" and doesn't allow the user to attach the file to the massage.

Selecting to attach TWO files with a combined size that is larger than 300kb brings up a popup message within the app that reads "You've exceeded the maximum length. Remove special characters or shorten the message to send it." and will not allow the user to send the message.


1.3 Environmental Variables:
Device: ZTE 1.3 MOZ
BuildID: 20140402040052
Gaia: 24f562fce468fc948ac9e6185e002c23350cb9ee
Gecko:
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 11

[Jason Smith [:jsmith]]

In that case, we've got user feedback here that the size of the message has been exceeded, so that means this isn't a blocking issue.

Comment 12

[Harvey]

Correction to Environmental Variables.

Device: ZTE OpenC 1.3 MOZ
BuildID: 20140402040052
Gaia: 24f562fce468fc948ac9e6185e002c23350cb9ee
Gecko: 7016a3cddb4b63462f93888b3783cf5362f2383f
Version: 28.0
Base Image: P821A10-ENG_20140321

Comment 13

[Julien Wajsberg [:julienw]]

I think the issue is that we used to set a button to "disabled" but bug 923023 changed it to a link.

Now I have several questions for UX:
* should we disable the "attach" button when we exceed the max size? Or should we hide it?
* in 1.1, when the button is disabled, its syle changes to reflect this. So if we go by disable the button, should we also change its style?

My preference would be to hide the button in that case.

Comment 14

[Omega Feng [:Omega] [:馮於懋]]

IMO we should use the same solution in bug 929027, just let user choose files. When pressing OK/Done/V buttons in the file picker, inform user with a dialog and don't attach anything in that picking.

Comment 15

[Julien Wajsberg [:julienw]]

Omega, don't you think this is awkward to let the user do an action instead of preventing it in the same place?

The case I'm talking here is that we _already_ exceeds the maximum size.

Comment 16

[Omega Feng [:Omega] [:馮於懋]]

Graying out or hiding a button makes user worried: "why can't I attach anymore?" or "where is the attach button?" It's better to let user know what happen, and that's why I suggest the same solution in bug 929027.
In real case, correct me if I'm wrong, it cannot already exceed the maximum size because we don't allow it in bug 929027. So, it could be 299KB, and user can attach one more 1KB file without warning, or attach a 2KB file with warning.
If it's really really lucky 300KB reached, it could still use the same behavior. Although it's not quite perfect, it's still acceptable since it's just a corner case.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Omega Feng [:Omega] from comment #16)
> Graying out or hiding a button makes user worried: "why can't I attach
> anymore?" or "where is the attach button?" It's better to let user know what
> happen

Actually there is an in-app notification being displayed in that case, so the user knows what happens.

>  and that's why I suggest the same solution in bug 929027.
> In real case, correct me if I'm wrong, it cannot already exceed the maximum
> size because we don't allow it in bug 929027. 

Actually it's possible :)
Add small videos (less than 300K) instead of images; one by one.
There is a flaw in bug 929027 that we don't check the _current_ attached size. Probably we should, but the reporter says he can reproduce with only images, by attaching enough images, and for images we don't check the size because we resize them.
So in the end, this can happen even if we fix the flaw.

> So, it could be 299KB, and
> user can attach one more 1KB file without warning, or attach a 2KB file with
> warning.
> If it's really really lucky 300KB reached, it could still use the same
> behavior. Although it's not quite perfect, it's still acceptable since it's
> just a corner case.

I really don't like letting the user do an action, and at the end of the action cancel this action, whereas we could prevent it before.

For example, if you don't like disabling the button, we could display a dialog as soon as the user pressed the "attach" button, instead of waiting for the pick.

I know the previous UX designer tried to minimize the amount of dialogs because the user does not really read them, and I don't really like adding more of them :(

NI Omega for the additional information added here.

Comment 18

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> (In reply to Omega Feng [:Omega] from comment #16)
> > Graying out or hiding a button makes user worried: "why can't I attach
> > anymore?" or "where is the attach button?" It's better to let user know what
> > happen
> 
> Actually there is an in-app notification being displayed in that case, so
> the user knows what happens.

Julien, could you explain when and how it will display?
I'm thinking about maybe this in-app notification is enough for user to understand the status.

> I really don't like letting the user do an action, and at the end of the
> action cancel this action, whereas we could prevent it before.

I agree with you.

> For example, if you don't like disabling the button, we could display a
> dialog as soon as the user pressed the "attach" button, instead of waiting
> for the pick.

Yes, we can add this dialog, if the in-app notification is not enough.

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Omega Feng [:Omega] from comment #18)
> (In reply to Julien Wajsberg [:julienw] from comment #17)
> > (In reply to Omega Feng [:Omega] from comment #16)
> > > Graying out or hiding a button makes user worried: "why can't I attach
> > > anymore?" or "where is the attach button?" It's better to let user know what
> > > happen
> > 
> > Actually there is an in-app notification being displayed in that case, so
> > the user knows what happens.
> 
> Julien, could you explain when and how it will display?
> I'm thinking about maybe this in-app notification is enough for user to
> understand the status.

Here is how I get it to display:
* attach an image of about 180KB
* attach a video of about 200KB (less than 300KB, but more than (300 - size of image KB))

I just tried again and now I notice that this in-app notification does not stay in place. That's a change from bug 952109 but I think it's a bug because while I understand why we want it to disappear when the subject reaches max length, we don't want it to disappear when the _message_ is exceeding length. Do you think the same, Omega?

If yes we can definitely fix it in this bug too.

Comment 20

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> I just tried again and now I notice that this in-app notification does not
> stay in place. That's a change from bug 952109 but I think it's a bug
> because while I understand why we want it to disappear when the subject
> reaches max length, we don't want it to disappear when the _message_ is
> exceeding length. Do you think the same, Omega?
Yeah, keep the notification displaying and it disappears as soon as the total size is lower than 300KB.

> If yes we can definitely fix it in this bug too.

Comment 21

[Julien Wajsberg [:julienw]]

Requesting 1.3? because the in-app notification does not stay in place and thus does not give enough information to the user which in my opinion invalidate the decision in comment 11.

A 1.3-specific patch would only fix this regression and would not do the other behavior changes discussed in this bug. We can file another bug if you prefer but I feel this can be done here too.

Comment 22

[Julien Wajsberg [:julienw]]

The regression I'm talking about in comment 21 comes from bug 952109.

Comment 23

[Julien Wajsberg [:julienw]]

I thought more and I filed bug 995173 for the fix I'm talking about in comment 21. Let's keep this bug for disabling buttons when we're in the "message too big" state.

Comment 24

[Julien Wajsberg [:julienw]]

So, Omega, once bug 995173 is fixed, what should we do here then ? :)

I think my original questions in comment 13 still apply ;)

Comment 25

[Omega Feng [:Omega] [:馮於懋]]

Since the exceeding-maximum-size-message stays there, we can disable Attach button. (Let user know there is a button but not functional now.)

Comment 26

[Julien Wajsberg [:julienw]]

Should there be a visual design change for the disabled button?

Comment 27

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Should there be a visual design change for the disabled button?

Vicky, could you design a Disabled Attach icon for Messages? Thanks!

Comment 28

[Victoria Gerchinhoren [:vicky]]

Hey, the disabled state of action icons in headers is already defined in Building Blocks. They should all behave the same. Already confirmed with Arnau, but NI to him so a minor opacity tweak is made to adjust contrast a bit.

Comment 29

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

Yep, please use:  <button disabled> or <a href="..." aria-disabled="true"> to disable it.
Right now opacity is set to 30%.
Maybe after fixing that bug, we could file a new one to fine-tune this value to something that works for VD in all header skins.

Comment 30

[Julien Wajsberg [:julienw]]

Great, thanks !

For a contributor, the final solution is to disable the button when we reach the max capacity. These buttons are currently links (<a>) but maybe we can seize this opportunity to transform them to <button>.

I think we should convert the "lock" property in compose.js to 2 methods lock() and unlock(), and these methods would add/remove the disabled property on the button. f

Comment 31

[Vishnu Teja [:ythej]]

what if we take 'if(size of the attachent >= 300KB)' condition and in that we will make compose.lock=true; and disable the attach button(we decrease the opacity of the button to show the user that the button is disabled) and we may take the remaining code in else...

Comment 32

[Julien Wajsberg [:julienw]]

If you set "disabled" on the button, I think we already have a style that reduces the opacity.

The solution is in my opinion to replace all "compose.lock = true" by a "Compose.lock()" call; in that "lock" function you can set the internal lock property _and_ set the disabled state on the button. Same for "unlock".

If you have another idea feel free to propose in a pull request, but I really think the idea I propose is the best one here.

Comment 33

[Vishnu Teja [:ythej]]

Attachment: pull request for the bug fix 990481

Comment 34

[Julien Wajsberg [:julienw]]

Comment on attachment 8516910 [details] [review]
pull request for the bug fix 990481

Hey Oleg, do you have the time to look at this?

I'd like to make compose.lock private but maybe it's good enough to rename it to "_lock". And I'd like locking/unlocking to be called lock/unlock. And obviously more unit tests :)

Comment 35

[Beatriz Rodríguez [:brg]]

Some operators allow MMS bigger than 300KB. Will this patch limit MMS size to only 300KB?
Is the patch taking the "operatorSizeLimitation" data from [1] into consideration?

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/apn.json

Comment 36

[Julien Wajsberg [:julienw]]

Yes Beatriz, don't worry, we still keep this configurable :)

Comment 37

[Julien Wajsberg [:julienw]]

Changing the title to clarify this.

Comment 38

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8516910 [details] [review]
pull request for the bug fix 990481

Hey Vishnu,

I've left some comments on GitHub + please follow Julien's advice on method naming and obviously we need more unit tests to cover new changes.

Thanks for contribution and ask review again when you're ready!

Comment 39

[Vishnu Teja [:ythej]]

Attachment: Screenshot from 2014-11-11 16:34:08_1.jpg

When the attach button is disabled there is no visual effect on the attach button.
User may not be able recognize that the attach button is actually disabled.

Comment 40

[Vishnu Teja [:ythej]]

Attachment: Screenshot from 2014-11-11 16:25:22_1.jpg

We can decrease the opacity of the the attach button to show the user that the attach button is disabled due to excess size of attachments.

Comment 41

[Beatriz Rodríguez [:brg]]

(In reply to Julien Wajsberg [:julienw] from comment #37)
> Changing the title to clarify this.
Thanks a lot!

Comment 42

[Fang Shih [:fang]]

Comment on attachment 8520582 [details]
Screenshot from 2014-11-11 16:25:22_1.jpg

Looks fine to me! Thanks!

Comment 43

[Vishnu Teja [:ythej]]

(In reply to Fang Shih [:grasspizza] from comment #42)
> Comment on attachment 8520582 [details]
> Screenshot from 2014-11-11 16:25:22_1.jpg
> 
> Looks fine to me! Thanks!

Thank you!

Comment 44

[Vishnu Teja [:ythej]]

pull request for the bug fix 990481
https://github.com/mozilla-b2g/gaia/pull/26826

Comment 45

[Vishnu Teja [:ythej]]

Attachment: bug_990481.patch

Pull request:
https://github.com/mozilla-b2g/gaia/pull/26913/files

Comment 46

[Vishnu Teja [:ythej]]

Attachment: pull request for the bug fix 990481

Comment 47

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8540726 [details] [review]
pull request for the bug fix 990481

Hey Vishnu,

Looks good to me, I've left few comments, mostly nits at GitHub. Please make changes within separate commit (same PR, second commit).

And ask for r? again whenever you're ready.

Thanks for your work!

Comment 48

[Vishnu Teja [:ythej]]

Attachment: pull request for the bug fix 990481(Round No.1)

Hi Oleg,
Done the changes accordingly but need to decide the comment line's.
Thanks :)

Comment 49

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8541218 [details] [review]
pull request for the bug fix 990481(Round No.1)

(In reply to Vishnu Teja from comment #48)
> Created attachment 8541218 [details] [review]
> pull request for the bug fix 990481(Round No.1)
> 
> Hi Oleg,
> Done the changes accordingly but need to decide the comment line's.
> Thanks :)

Looks good now, please fix the rest of nits (just added few tiny suggestions), squash commits into one and append "r=zasypkin" at the end of commit message, push this commit to PR.

Once Treeherder is green please add "checkin-needed"!

Thanks a lot!

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/2531434a0c9aa03314722953381f1525a021a938

Comment 51

[Beatriz Rodríguez [:brg]]

Tested with today build in Flame( Gecko-7a1f501.Gaia-9946a49) and 21407 SIM card, when I reach 495KB in a single MMS, there is a warning message saying that I should remove attachments or short the message. The attach option is not active till you delete any item. Thanks everyone for your good work!

Comment 52

[Vishnu Teja [:ythej]]

Credits to Julien and Oleg . :)