995173 - [Messages] The in-app notification for the "message is too big" state should not disappear after 2 seconds

Status: VERIFIED FIXED

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

Description

[Julien Wajsberg [:julienw]]

From Bug 990481:

STR:
* attach enough less-than-300k videos to display the in-app notification that the message is too big.

Expected:
* the notification is displayed and stays in place

Actual:
* the notification is displayed but disappears.

That's a regression from bug 952109.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: 2nd proposal: github PR for master

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: 2nd proposal: github PR for v1.3

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: 1st proposal: github PR

Comment 4

[Julien Wajsberg [:julienw]]

Hey Steve,

I made 2 proposals here:

* 1st proposal: keeps basically the same way of working than before except we can decide whether the notice is transient

I found that it has an issue: when the user reaches the message max length, and then just focus the subject and focus elsewhere, the notice would be removed. This happens because both the message max length and subject max length notices share the same DOM object.

That's why I made the 2nd proposal, slightly more changes, but in the end it's simpler because they don't share a DOM element anymore.

I actually do prefer the 2nd proposal but I wanted to take your advice too.

Comment 5

[Julien Wajsberg [:julienw]]

Can we please have a rationale for the minusing?

* regression
* bad for UX and thus for the user
* simple patch

If it's not blocking for 1.3 (because at this stage we want to let only critical patch in), is it for 1.4?

Comment 6

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Can we please have a rationale for the minusing?
> 
> * regression
> * bad for UX and thus for the user

I wouldn't call a severe UX fallout. It's a minor UX fallout at best, as the message is still presented to show the error.

> * simple patch

This has no influence on a blocking decision.

> 
> If it's not blocking for 1.3 (because at this stage we want to let only
> critical patch in), is it for 1.4?

comment 5 isn't enough to block. At this point, it has to be a cert blocker in order to block 1.3. This likely wouldn't block 1.4 either, as it's a minor regression at best. We can consider this for approval though.

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8405365 [details] [review]
2nd proposal: github PR for master

I agree we should sperate the notification into different element because both of them might show up in the same time but different reason.
Since this bug is move to backlog, how about we think for farther: We put the notification into another widget and generate it with template only when we need it. That could clean up the thread_ui and index.html a little bit. wdyt?

Comment 8

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8405365 [details] [review]
> 2nd proposal: github PR for master
> 
> I agree we should sperate the notification into different element because
> both of them might show up in the same time but different reason.
> Since this bug is move to backlog, how about we think for farther: We put
> the notification into another widget and generate it with template only when
> we need it. That could clean up the thread_ui and index.html a little bit.
> wdyt?

Actually I like the current simplicity of having these notices with a constant text (except for one) and displaying them when needed.

Using a template makes sense when we have one layout and changing and unpredictable text (like for displaying a thread... where we don't use templates yet ;) ). But here I feel it's unnecessary because we have constant texts and using simple DOM is good enough. It's not like we have dozens of different notices, we have only 3 (the 4th is different). If we have more in the future we can definitely revisit your proposal though !

Comment 9

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> (In reply to Steve Chung [:steveck] from comment #7)
> > Comment on attachment 8405365 [details] [review]
> > 2nd proposal: github PR for master
> > 
> 
> Actually I like the current simplicity of having these notices with a
> constant text (except for one) and displaying them when needed.
> 
> Using a template makes sense when we have one layout and changing and
> unpredictable text (like for displaying a thread... where we don't use
> templates yet ;) ). But here I feel it's unnecessary because we have
> constant texts and using simple DOM is good enough. It's not like we have
> dozens of different notices, we have only 3 (the 4th is different). If we
> have more in the future we can definitely revisit your proposal though !

Ok then let's focus on 2nd proposal for now and I'll review it later.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8405365 [details] [review]
2nd proposal: github PR for master

Only some small nits but overall looks good, thanks!

Comment 11

[Julien Wajsberg [:julienw]]

Fixed the nits, rebased with conflicts, so waiting for a green travis before merging.

Comment 12

[Julien Wajsberg [:julienw]]

master: 9f12ef085ff8095d5da81ea1658e4705013fd64e

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8405365 [details] [review]
2nd proposal: github PR for master

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 #): bug 952109
[User impact] if declined: The banner explaining that the max length has been reached disappears after 2 seconds, and as a result the user things the issue is only transient while it's really permanent.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low, there is unit tests and the changes itself is not risky
[String changes made]: no

Comment 14

[Preeti Raghunath(:Preeti)]

Comment on attachment 8405365 [details] [review]
2nd proposal: github PR for master

for incorrect UX on the user

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/d47cd04fb40b556d5f4c72d6d9ad99adb724c669#diff-1

Comment 16

[Rachel Pribble [:rpribble]]

This issue has been verified fixed on v1.4 on the following devices:

Environmental Variables:
Device: Buri v1.4 MOZ ril
BuildID: 20140522000200
Gaia: 233dd43b3b976e66a619dbc1b4044ed1e3ca3e34
Gecko: c3c0c57daef8
Version: 30.0
RIL Version:
Firmware Version: v1.2-device.cfg

Environmental Variables:
Device: Flame v1.4 MOZ ril
BuildID: 20140522000200
Gaia: 233dd43b3b976e66a619dbc1b4044ed1e3ca3e34
Gecko: c3c0c57daef8
Version: 30.0
Firmware Version: v10F-3

Environmental Variables:
Device: Open_C 1.4
BuildID: 20140522000200
Gaia: 233dd43b3b976e66a619dbc1b4044ed1e3ca3e34
Gecko: c3c0c57daef8
Version: 30.0
Firmware Version: P821A10V1.0.0B06_LOG_DL

Comment 17

[Rachel Pribble [:rpribble]]

This issue is verified fixed on the Buri v2.0 master:

Environmental Variables:
Device: Buri v2.0 MOZ ril
BuildID: 20140522040230
Gaia: ef66efa34ed8a559c8998bde688fae88eb943a7a
Gecko: b40296602083
Version: 32.0a1
Firmware Version: v1.2-device.cfg