[Messages] Report panel visual refresh

RESOLVED FIXED in 2.1 S7 (24Oct)

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: julienw, Assigned: steveck)

Tracking

unspecified
2.1 S7 (24Oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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)

Comment 1

4 years ago
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)
(Reporter)

Comment 2

4 years ago
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)

Comment 3

4 years ago
(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
(Reporter)

Comment 5

4 years ago
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 :)

Comment 7

4 years ago
Created attachment 8463201 [details]
[Message] Message report v1.0.pdf

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

Comment 8

4 years ago
Hey Fang, I don't remember if you've sent a design spec for message report somewhere else?
Flags: needinfo?(fshih)
(Reporter)

Updated

4 years ago
Whiteboard: [sms-most-wanted]
Per product prioritization, this could be 2.2 focus.
blocking-b2g: 2.1? → backlog
feature-b2g: --- → 2.2

Comment 10

4 years ago
Created attachment 8478136 [details]
2.1_Message_Report_VisualSpec.zip

Attached the visual spec for message report, Thanks!
Flags: needinfo?(fshih)

Comment 11

4 years ago
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
(Reporter)

Updated

4 years ago
Blocks: 1067820
Whiteboard: [sms-most-wanted] → [sms-most-wanted][p=2]
Target Milestone: --- → 2.1 S5 (26sep)

Comment 12

4 years ago
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!

Comment 14

4 years ago
Hey Oleg,
Thank you for the reply,
Will look into the mentored sms app related bugs.
(Reporter)

Updated

4 years ago
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)

Updated

4 years ago
Assignee: nobody → schung
Status: NEW → ASSIGNED
(Assignee)

Comment 15

4 years ago
Created attachment 8496079 [details] [review]
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 :)
Attachment #8496079 - Flags: feedback?(felash)
(Reporter)

Comment 16

3 years ago
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
(Reporter)

Comment 17

3 years ago
Comment on attachment 8496079 [details] [review]
Link to github

left comments on github :) looks good !
Attachment #8496079 - Flags: feedback?(felash) → feedback+
(Reporter)

Updated

3 years ago
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
(Reporter)

Updated

3 years ago
Blocks: 1074714
Whiteboard: [sms-most-wanted][p=2] → [sms-most-wanted][p(2.1S6)=2][p(2.1S5)=2]
(Reporter)

Updated

3 years ago
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
(Reporter)

Comment 18

3 years ago
I'm away for 3 days so please request review from oleg once you're ready !
(Assignee)

Comment 19

3 years ago
Created attachment 8499695 [details] [review]
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)
(Assignee)

Comment 21

3 years ago
Created attachment 8502320 [details]
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.
Attachment #8502320 - Flags: ui-review?(fshih)
(Assignee)

Comment 22

3 years ago
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 23

3 years ago
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-
(Assignee)

Comment 24

3 years ago
Created attachment 8502438 [details]
2014-10-09-19-48-30.png

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+
(Assignee)

Comment 26

3 years ago
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)
(Reporter)

Comment 27

3 years ago
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)
(Reporter)

Comment 28

3 years ago
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)

Comment 29

3 years ago
Created attachment 8503980 [details]
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!

Updated

3 years ago
Attachment #8502438 - Flags: ui-review?(fshih) → ui-review-
(Assignee)

Comment 30

3 years ago
Created attachment 8504020 [details]
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)
(Assignee)

Comment 31

3 years ago
(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 :-/
(Reporter)

Comment 32

3 years ago
(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 :)
(Assignee)

Comment 33

3 years ago
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)
(Reporter)

Comment 34

3 years ago
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)
(Reporter)

Updated

3 years ago
Attachment #8496079 - Attachment is obsolete: true
(Assignee)

Comment 35

3 years ago
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)

Updated

3 years ago
Attachment #8504020 - Flags: ui-review?(fshih) → ui-review+
(Reporter)

Updated

3 years ago
Blocks: 1083069
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]
(Reporter)

Updated

3 years ago
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (24Oct)
(Reporter)

Comment 36

3 years ago
(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!
(Reporter)

Comment 37

3 years ago
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)
(Assignee)

Comment 38

3 years ago
Comment on attachment 8499695 [details] [review]
Link to github

These 2 issues is critical, thanks for pointing out :)
Attachment #8499695 - Flags: review?(felash)
(Reporter)

Comment 39

3 years ago
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+
(Assignee)

Comment 40

3 years ago
(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
(Assignee)

Comment 41

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

In master: 3ec94f448bb5c1c9c264896685c6ef77ab718c87
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Updated

3 years ago
Depends on: 1089198
No longer depends on: 1089198
Duplicate of this bug: 1010318
(Reporter)

Comment 43

3 years ago
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)
(Assignee)

Comment 44

3 years ago
(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)
(Assignee)

Updated

3 years ago
Blocks: 1092021
let's keep the already-landed improvement in 2.2.
feature-b2g: 2.2? → 2.2+
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.