1021608 - [Messages] Report panel visual refresh

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

See attachment: https://bug1021387.bugzilla.mozilla.org/attachment.cgi?id=8435416

Looking at this screen, I can't help thinking a "resend" button would be useful.

Comment 1

[Jenny Lee]

It is quite useful but not exactly necessary since we already have two ways to resend: (1) short tap on the message and there will be a dialogue box pop up asking user if they want to try send again, (2) long press on the message and select "Resend this message" from action sheet. 
Here's another idea, I am thinking getting rid of (1) and add a "resend" button right next to the "!" icon on messages that had failed to deliver, that's based on the assumption that resend is the most common action people would take after failure of delivery (rather than deleting the message). However, it might not be visually pleasing :p    

Hello Vicky, what do you think of the idea to add a "resend" button on messages that had failed to send? The "resend" button can be placed ex. right next to the "!" button.

Comment 2

[Julien Wajsberg [:julienw]]

Jenny, tapping the "!" already does a resend (but with a confirmation). Do you suggest to remove this confirmation ?

Also, I thought the "resend" button on the "report" panel could be useful. It would not be the primary way to resend a message, but as a user, when I am on this screen and I want to resend the message, here are the actions I need to do:

* I first need to close the panel
* then I need to visually locate the message (again)
* then I need to either longpress it or tap the "!" icon

This is cumbersome.

However, when I am on the "report" screen and see the message is in "error", it feels completely logical, from an UX point of view, to have a button to act on this error (ie: resend the message).

Comment 3

[Jenny Lee]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Jenny, tapping the "!" already does a resend (but with a confirmation). Do
> you suggest to remove this confirmation ?
> 
Yes I was originally suggesting putting a resend button (in text) on screen directly next to the "!" button (this is similar to when you failed to send a picture with whatsapp, there will be an upload icon on the picture for you to 1 tap resend), but it doesn't work after I actually tried to layout. So let's keep the short tap and confirm to resend behavior.   
 
> However, when I am on the "report" screen and see the message is in "error",
> it feels completely logical, from an UX point of view, to have a button to
> act on this error (ie: resend the message).

I'm totally okay with adding the "resend" button on report page. I was just assuming people would normally try to resend before they go to the report page, so resending is probably not what they are looking for on report page. However, as you pointed out, it can be very helpful, I'll update the wireframe accordingly. Tks!

Comment 4

[kumar rishav (:rishav_)]

Hi jeeny and Juliew
@jenny I think what you told in comment 1 is right.We should remove (1).
First reason :most of actions now run (reference to android) on long press rather than short tap.so user get used to it.
second :for dialogue box we have tap on red icon(!) instead of error message, which is very small and not that user friendly.
So i think we should remove this and (2) i.e Long press is quite enough as long press become general now a days.

@juliew
a/c to my POV , user not frequently check 'report message option' because icon (!) is enough to tell them (except new/first-time user).We may add 'resend option' in 'report message' but it's not that much necessary.

Thanks

Comment 5

[Julien Wajsberg [:julienw]]

We don't check "report message panel" because we want to check the status, but to check other things (like size, etc); but _then_ we might want to resend asap.

Comment 6

[kumar rishav (:rishav_)]

Now completely agree with you :)

Comment 7

[Jenny Lee]

Attachment: [Message] Message report v1.0.pdf

Hi all, see attached for the latest message report sepc. Thanks!

Comment 8

[Julien Wajsberg [:julienw]]

Hey Fang, I don't remember if you've sent a design spec for message report somewhere else?

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Per product prioritization, this could be 2.2 focus.

Comment 10

[Fang Shih [:fang]]

Attachment: 2.1_Message_Report_VisualSpec.zip

Attached the visual spec for message report, Thanks!

Comment 11

[Kai-Chih Hu [:khu]]

Use feature-b2g:2.2? rather than just 2.2.

Comment 12

[Anusha]

Hi,
I want to work on this bug,
Thanks.

Comment 13

[Oleg Zasypkin [:azasypkin]]

Hey Anusha,

Thanks for expressing interest! We've already planned this bug for the SMS team itself for the current sprint, and it's not really good first bug :)

We would be happy if you can choose something that you like from list of our mentored bugs [1]

[1] https://bugzilla.mozilla.org/buglist.cgi?f10=bug_mentor&f1=OP&list_id=11182777&f0=OP&f8=CP&v3=gaia%3A%3Asms&v10=felash%40gmail.com%2C%20schung%40mozilla.com%2C%20azasypkin%40mozilla.com&emailtype1=substring&f9=CP&emailassigned_to1=1&query_format=advanced&o10=anywordssubstr&j1=OR&bug_status=UNCONFIRMED&bug_status=NEW&email1=nobody%40mozilla.org&component=Gaia%3A%3ASMS

Thanks!

Comment 14

[Anusha]

Hey Oleg,
Thank you for the reply,
Will look into the mentored sms app related bugs.

Comment 15

[Steve Chung [:steveck]]

Attachment: Link to github

Hi julien, it's the first WIP for report view refresh. The layout change and resend button functionality is ready. The testing part is still ongoing and we might still got some concern in UX spec, but any early feedback would be really helpful :)

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8496079 [details] [review]
Link to github

Gave a first feedback but I didn't look on the device yet so keeping the feedback request

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8496079 [details] [review]
Link to github

left comments on github :) looks good !

Comment 18

[Julien Wajsberg [:julienw]]

I'm away for 3 days so please request review from oleg once you're ready !

Comment 19

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Oleg, sorry that I create another branch for it because I added unit test and some fixing after julien's 1st review. But I could not reopen the branch after some force update. The new patch is based on julien's feedback without test review yet, I'll add some comments for the new fixing, and really sorry for the inconvenience.

Comment 20

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8499695 [details] [review]
Link to github

Looks good, left some comments on GitHub, mostly just nits. Also it would be great to confirm with VD some changes that doesn't look similar to the spec.

Thanks!

Comment 21

[Steve Chung [:steveck]]

Attachment: report-screenshots.zip

Hi Fang, here is the report panel screenshots for each cases. Oleg has some concern about the list layout that does not match the visual spec you provided. So it's safer to request review here since the original visual spec might need some changes as we discussed offline before, thanks.

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8499695 [details] [review]
Link to github

Hi Oleg, thanks for the review. I've addressed most of comments and will leave some comments on github about the changes. I'll also attach images for ui-review for sure.

Comment 23

[Fang Shih [:fang]]

Comment on attachment 8502320 [details]
report-screenshots.zip

Hi Steve, 
Overall it looks good. I just need you to make some adjustments to the line height. The spacing between each text is too big right now. Can we reduce the spacing to smaller size for each text? 

And for the contact name, currently we have it "bold", I would like to change to "400"(medium) so is closer to the spec. For the contact photo, they need to be vertically center aligned. 
Please let me know if you have any questions. Thanks!

Comment 24

[Steve Chung [:steveck]]

Attachment: 2014-10-09-19-48-30.png

Hi Fang, I adjusted the layout per your suggestion, could you please review again? Thanks.

Comment 25

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8499695 [details] [review]
Link to github

r=me, thanks! Just a few nits and question on Github.

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8499695 [details] [review]
Link to github

Thanks for point out some critical issues in the patch, and I think it's necessary to request a review again for the chages, sorry for the inconvenience...

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8499695 [details] [review]
Link to github

Since Oleg is in PTO, I'll have a look shortly

Comment 28

[Julien Wajsberg [:julienw]]

Comment on attachment 8499695 [details] [review]
Link to github

left my comments about the race issue resolution and the unit test change.

I really think we should avoid the race instead of adding complexity to resolve it. Please tell me if you disagree, in that case we could go on and land this, and discuss this in a separate bug.

Comment 29

[Fang Shih [:fang]]

Attachment: Message report_bug.png

Hi Steve, 
Thanks for the updated! It's really close! : D
For some reason the spacing of the phone number one (not in contact list) is still too large compared to the other list. Can we make all the spacing in same size? Also, can we move up the icons by 2 px vertically. Currently it looks lower than text. You can refer to the file attached for more details. Thanks!

Comment 30

[Steve Chung [:steveck]]

Attachment: 2014-10-13-18-55-22.png

Hi Fang, I've updated per your request in the latest patch.

Comment 31

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> Comment on attachment 8499695 [details] [review]
> Link to github
> 
> left my comments about the race issue resolution and the unit test change.
> 
> I really think we should avoid the race instead of adding complexity to
> resolve it. Please tell me if you disagree, in that case we could go on and
> land this, and discuss this in a separate bug.

Since I could not figure out a simpler way to update the report block only, maybe we could apply your 1st proposal on github that reduce contact list rendering times? And sorry I'm not quite sure the unit test changes you mentioned here :-/

Comment 32

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #31)
> (In reply to Julien Wajsberg [:julienw] from comment #28)
> > Comment on attachment 8499695 [details] [review]
> > Link to github
> > 
> > left my comments about the race issue resolution and the unit test change.
> > 
> > I really think we should avoid the race instead of adding complexity to
> > resolve it. Please tell me if you disagree, in that case we could go on and
> > land this, and discuss this in a separate bug.
> 
> Since I could not figure out a simpler way to update the report block only,
> maybe we could apply your 1st proposal on github that reduce contact list
> rendering times?

Yep, as said on github, let's do this :)

> And sorry I'm not quite sure the unit test changes you
> mentioned here :-/

I was mentioning the latest comment from Oleg :)

Comment 33

[Steve Chung [:steveck]]

Comment on attachment 8499695 [details] [review]
Link to github

Adding the refinement for the rendering racing issue and unit test. The method I took might be slightly different than you suggested, but the goal is the same.

Comment 34

[Julien Wajsberg [:julienw]]

Comment on attachment 8499695 [details] [review]
Link to github

>  The method I took might be slightly different than you suggested, but the goal is the same.

Sorry, but the method you took doesn't work here :(

Comment 35

[Steve Chung [:steveck]]

Comment on attachment 8499695 [details] [review]
Link to github

You are right about the edge cases, although it seems not possible to happen. I also add a test case for this scenario, thanks!

Comment 36

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #35)
> Comment on attachment 8499695 [details] [review]
> Link to github
> 
> You are right about the edge cases

Just want to point out that your previous proposal was not failing in an edge case only, but when requesting several times a report for a group MMS (with several recipients).

> although it seems not possible to
> happen. I also add a test case for this scenario, thanks!

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8499695 [details] [review]
Link to github

Sorry, some nits that I didn't see in my previous review, my fault :/

2 nits are more important than others:
* you need to change the l10n key when you change the value.
* when we're in the "group view" panel, we're also still in the thread. I don't think this mistake has any consequence in the current code, but it can have later if the function is wrong, when we'll do group MMS right.

Comment 38

[Steve Chung [:steveck]]

Comment on attachment 8499695 [details] [review]
Link to github

These 2 issues is critical, thanks for pointing out :)

Comment 39

[Julien Wajsberg [:julienw]]

Comment on attachment 8499695 [details] [review]
Link to github

r=me with the last nit, provided the tests are green. Don't forget to squash :)

I see a glitch when displaying the panel: the space at the top (just above "Text Message (SMS)") is increasing about 1 sec after the report is displayed. Also, I see that the "delivered" tick image flash at the same moment. I think the best is to track this in a separate bug, but we should fix it though. Also maybe this could be the issue with flex layout in bug 1081072, as I have an older Gecko build currently.

Can you check this on a current build and possibly file a separate bug?
Thanks !

Comment 40

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #39)
> Comment on attachment 8499695 [details] [review]
> Link to github
> 
> r=me with the last nit, provided the tests are green. Don't forget to squash
> :)
> 
> I see a glitch when displaying the panel: the space at the top (just above
> "Text Message (SMS)") is increasing about 1 sec after the report is
> displayed. Also, I see that the "delivered" tick image flash at the same
> moment. I think the best is to track this in a separate bug, but we should
> fix it though. Also maybe this could be the issue with flex layout in bug
> 1081072, as I have an older Gecko build currently.
> 
> Can you check this on a current build and possibly file a separate bug?
> Thanks !

After bug 1081072 landed, the glitch issue is gone. So let's wait for the gaia-try now

Comment 41

[Steve Chung [:steveck]]

Gaia Try is green now, thanks for all the review! 

In master: 3ec94f448bb5c1c9c264896685c6ef77ab718c87

Comment 43

[Julien Wajsberg [:julienw]]

Hey Steve,

I see 2 issues with the patch, can you file 1 follow-up ?

1. we miss one string value for "report-status-not-applicable". I think it happens when I don't have read/delivery report enabled, I send a MMS, and display the report for this outgoing MMS. Here is the error I get in the console:

10-30 18:21:13.824  1460  1460 W Messages: Content JS WARN: mozL10n: A non-existing entity requested: report-status-not-applicable 

I don't know, maybe that's on purpose because we don't display it? In that case maybe we can have a "null" in the REPORT_MAP so that we don't request the l10n value, just to not have a log when we don't need to?

2. The line "Mobile, ...." is cut in the bottom in that case. We can clearly see the "," is cut off, and same if the carrier or phone type part has a "g" (ex: Orange).

Comment 44

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #43)
> Hey Steve,
> 
> I see 2 issues with the patch, can you file 1 follow-up ?
> 
> 1. we miss one string value for "report-status-not-applicable". I think it
> happens when I don't have read/delivery report enabled, I send a MMS, and
> display the report for this outgoing MMS. Here is the error I get in the
> console:
> 
> 10-30 18:21:13.824  1460  1460 W Messages: Content JS WARN: mozL10n: A
> non-existing entity requested: report-status-not-applicable 
> 
> I don't know, maybe that's on purpose because we don't display it? In that
> case maybe we can have a "null" in the REPORT_MAP so that we don't request
> the l10n value, just to not have a log when we don't need to?
> 
Ya you are right, we don't need l10n for not-applicable case. I will propose a patch for this. 

> 2. The line "Mobile, ...." is cut in the bottom in that case. We can clearly
> see the "," is cut off, and same if the carrier or phone type part has a "g"
> (ex: Orange).

Ah, will address this in new patch as well, thanks!

Comment 45

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

let's keep the already-landed improvement in 2.2.