Closed Bug 1021608 Opened 6 years ago Closed 5 years ago

[Messages] Report panel visual refresh

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2+, tracking-b2g:backlog)

RESOLVED FIXED
2.1 S7 (24Oct)
feature-b2g 2.2+
tracking-b2g backlog

People

(Reporter: julienw, Assigned: steveck)

References

Details

(Whiteboard: [sms-most-wanted][p(2.1S7)=1][p(2.1S6)=2][p(2.1S5)=2])

Attachments

(4 files, 4 obsolete files)

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.
Flags: needinfo?(jelee)
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.
Flags: needinfo?(vpg)
Flags: needinfo?(jelee)
Flags: needinfo?(felash)
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).
Flags: needinfo?(felash) → needinfo?(jelee)
(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!
Flags: needinfo?(vpg)
Flags: needinfo?(jelee)
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
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.
Now completely agree with you :)
Hi all, see attached for the latest message report sepc. Thanks!
Hey Fang, I don't remember if you've sent a design spec for message report somewhere else?
Flags: needinfo?(fshih)
Whiteboard: [sms-most-wanted]
Per product prioritization, this could be 2.2 focus.
blocking-b2g: 2.1? → backlog
feature-b2g: --- → 2.2
Attached the visual spec for message report, Thanks!
Flags: needinfo?(fshih)
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Whiteboard: [sms-most-wanted] → [sms-most-wanted][p=2]
Target Milestone: --- → 2.1 S5 (26sep)
Hi,
I want to work on this bug,
Thanks.
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!
Hey Oleg,
Thank you for the reply,
Will look into the mentored sms app related bugs.
Summary: [Messages] Consider adding a "resend" button in the message report page, if there is an error → [Messages] Consider adding a "resend" button in the message report page, if there is an error, and other visual refresh
Assignee: nobody → schung
Status: NEW → ASSIGNED
Attached file Link to github (obsolete) —
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 :)
Attachment #8496079 - Flags: feedback?(felash)
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 on attachment 8496079 [details] [review]
Link to github

left comments on github :) looks good !
Attachment #8496079 - Flags: feedback?(felash) → feedback+
Summary: [Messages] Consider adding a "resend" button in the message report page, if there is an error, and other visual refresh → [Messages] Report panel visual refresh
Whiteboard: [sms-most-wanted][p=2] → [sms-most-wanted][p(2.1S6)=2][p(2.1S5)=2]
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
I'm away for 3 days so please request review from oleg once you're ready !
Attached file 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.
Attachment #8499695 - Flags: review?(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!
Attachment #8499695 - Flags: review?(azasypkin)
Attached file report-screenshots.zip (obsolete) —
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.
Attachment #8502320 - Flags: ui-review?(fshih)
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.
Attachment #8499695 - Flags: review?(azasypkin)
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!
Attachment #8502320 - Flags: ui-review?(fshih) → ui-review-
Attached image 2014-10-09-19-48-30.png (obsolete) —
Hi Fang, I adjusted the layout per your suggestion, could you please review again? Thanks.
Attachment #8502438 - Flags: ui-review?(fshih)
Comment on attachment 8499695 [details] [review]
Link to github

r=me, thanks! Just a few nits and question on Github.
Attachment #8499695 - Flags: review?(azasypkin) → review+
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...
Attachment #8499695 - Flags: review+ → review?(azasypkin)
Comment on attachment 8499695 [details] [review]
Link to github

Since Oleg is in PTO, I'll have a look shortly
Attachment #8499695 - Flags: review?(azasypkin) → review?(felash)
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.
Attachment #8499695 - Flags: review?(felash)
Attached image Message report_bug.png (obsolete) —
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!
Attachment #8502438 - Flags: ui-review?(fshih) → ui-review-
Attached image 2014-10-13-18-55-22.png
Hi Fang, I've updated per your request in the latest patch.
Attachment #8502320 - Attachment is obsolete: true
Attachment #8502438 - Attachment is obsolete: true
Attachment #8503980 - Attachment is obsolete: true
Attachment #8504020 - Flags: ui-review?(fshih)
(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 :-/
(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 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.
Attachment #8499695 - Flags: review?(felash)
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 :(
Attachment #8499695 - Flags: review?(felash)
Attachment #8496079 - Attachment is obsolete: true
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!
Attachment #8499695 - Flags: review?(felash)
Attachment #8504020 - Flags: ui-review?(fshih) → ui-review+
Whiteboard: [sms-most-wanted][p(2.1S6)=2][p(2.1S5)=2] → [sms-most-wanted][p(2.1S7)=1][p(2.1S6)=2][p(2.1S5)=2]
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (24Oct)
(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 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.
Attachment #8499695 - Flags: review?(felash)
Comment on attachment 8499695 [details] [review]
Link to github

These 2 issues is critical, thanks for pointing out :)
Attachment #8499695 - Flags: review?(felash)
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 !
Attachment #8499695 - Flags: review?(felash) → review+
(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
Gaia Try is green now, thanks for all the review! 

In master: 3ec94f448bb5c1c9c264896685c6ef77ab718c87
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1089198
No longer depends on: 1089198
Duplicate of this bug: 1010318
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).
Flags: needinfo?(schung)
(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!
Flags: needinfo?(schung)
Blocks: 1092021
let's keep the already-landed improvement in 2.2.
feature-b2g: 2.2? → 2.2+
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.