Closed Bug 990481 Opened 6 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.2 fixed)

RESOLVED FIXED
2.2 S3 (9jan)
Tracking Status
b2g-v2.2 --- fixed

People

(Reporter: zhang.ruili, Assigned: ythej, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(5 files, 1 obsolete file)

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.
blocking-b2g: --- → 1.3?
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
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.
Blocks: 923023
Keywords: regression
Yes, images can be resized automatically. I add 6 images making it reaching 300kb. And then I can add more images continually.
This is a dupe.
Whiteboard: DUPEME
No longer blocks: 923023
Status: UNCONFIRMED → RESOLVED
blocking-b2g: 1.3? → ---
Closed: 6 years ago
Resolution: --- → DUPLICATE
Whiteboard: DUPEME
Duplicate of bug: 988768
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.
(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.
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!
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
blocking-b2g: --- → 1.3?
QA,

Please check for max capacity of MMS and error message in the preview
Keywords: qawanted
(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?
QA Contact: jharvey
> 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
Keywords: qawanted
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.
blocking-b2g: 1.3? → backlog
Correction to Environmental Variables.

Device: ZTE OpenC 1.3 MOZ
BuildID: 20140402040052
Gaia: 24f562fce468fc948ac9e6185e002c23350cb9ee
Gecko: 7016a3cddb4b63462f93888b3783cf5362f2383f
Version: 28.0
Base Image: P821A10-ENG_20140321
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.
Flags: needinfo?(ofeng)
Whiteboard: [mentor=:julienw][lang=javascript]
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.
Flags: needinfo?(ofeng)
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.
Flags: needinfo?(ofeng)
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.
Flags: needinfo?(ofeng)
(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.
Flags: needinfo?(ofeng)
(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.
Flags: needinfo?(ofeng) → needinfo?(felash)
(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.
Flags: needinfo?(felash) → needinfo?(ofeng)
(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.
Flags: needinfo?(ofeng)
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.
blocking-b2g: backlog → 1.3?
Assignee: nobody → felash
The regression I'm talking about in comment 21 comes from bug 952109.
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.
blocking-b2g: 1.3? → ---
So, Omega, once bug 995173 is fixed, what should we do here then ? :)

I think my original questions in comment 13 still apply ;)
Flags: needinfo?(ofeng)
Since the exceeding-maximum-size-message stays there, we can disable Attach button. (Let user know there is a button but not functional now.)
Flags: needinfo?(ofeng)
Should there be a visual design change for the disabled button?
Flags: needinfo?(ofeng)
See Also: → 952407
(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!
Flags: needinfo?(ofeng) → needinfo?(vpg)
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.
Flags: needinfo?(vpg) → needinfo?(arnau)
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.
Flags: needinfo?(arnau)
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
Assignee: felash → nobody
Whiteboard: [mentor=:julienw][lang=javascript] → [mentor=:julienw][lang=js]
Mentor: felash
Whiteboard: [mentor=:julienw][lang=js] → [lang=js]
Assignee: nobody → vish.thej
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...
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.
Attachment #8516910 - Flags: review?(felash)
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 :)
Attachment #8516910 - Flags: review?(felash) → review?(azasypkin)
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
Flags: needinfo?(felash)
Yes Beatriz, don't worry, we still keep this configurable :)
Flags: needinfo?(felash)
Changing the title to clarify this.
Summary: [ZTE][OPENC]MMS app allows to attach more contents even when the capacity of 300k reached → [ZTE][OPENC]MMS app allows to attach more contents even when the max MMS size is reached
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!
Attachment #8516910 - Flags: review?(azasypkin)
Mentor: felash → azasypkin
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.
Attachment #8520576 - Flags: ui-review?
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.
Attachment #8520582 - Flags: ui-review?(fshih)
(In reply to Julien Wajsberg [:julienw] from comment #37)
> Changing the title to clarify this.
Thanks a lot!
Comment on attachment 8520582 [details]
Screenshot from 2014-11-11 16:25:22_1.jpg

Looks fine to me! Thanks!
Attachment #8520582 - Flags: ui-review?(fshih) → ui-review+
(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!
pull request for the bug fix 990481
https://github.com/mozilla-b2g/gaia/pull/26826
Attached patch bug_990481.patch (obsolete) — Splinter Review
Pull request:
https://github.com/mozilla-b2g/gaia/pull/26913/files
Attachment #8539230 - Flags: review?(azasypkin)
Keywords: checkin-needed
Attachment #8540726 - Flags: review?(azasypkin)
Attachment #8539230 - Attachment is obsolete: true
Attachment #8539230 - Flags: review?(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!
Attachment #8540726 - Flags: review?(azasypkin)
Hi Oleg,
Done the changes accordingly but need to decide the comment line's.
Thanks :)
Attachment #8541218 - Flags: review?(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!
Attachment #8541218 - Flags: review?(azasypkin) → review+
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/2531434a0c9aa03314722953381f1525a021a938
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S3 (9jan)
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!
Credits to Julien and Oleg . :)
Duplicate of this bug: 952407
You need to log in before you can comment on or make changes to this bug.