1037620 - [v2.1] Feature Proposal: Late Arrival Notice in Messages App

Status: RESOLVED FIXED

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

Description

[Mozilla_BEC]

This is a feature that was in webOS (can't remember if it was a PreWare patch or stock haha, I'm not sure I ever used a vanilla webOS device).

In short, if a message arrives "late", the timestamp on the message's bubble will alert the user as to this discrepancy and show them how late the message is.

Basically, the Message Info screen is wonderful, but it requires user intervention. In this fashion, the user is immediately and intuitively made aware that the message they've just received is not as fresh as the message bubble timestamp would make them believe.

Comment 1

[Mozilla_BEC]

Attachment: 2014-07-11-13-58-54.png

This screenshot shows that the MMS came about 17 minutes late

Comment 2

[Mozilla_BEC]

Attachment: 2014-07-11-13-59-00.png

This screenshot shows a VERY, VERY simple/preliminary mock-up of what I'm talking about with the "late" notification being a part of the timestamp on the message bubble.

Comment 3

[Julien Wajsberg [:julienw]]

Hey Jenny, what do you think of this?

I personally think this is a great idea!

Comment 4

[Mozilla_BEC]

Thanks for the support Julien :)

I'm thinking that in cases where the message is REALLY late (hours & minutes), the late timestamp might be better off below the arrival timestamp in order to not be as cramped.

Comment 5

[Jenny Lee]

Hey guys, before making any decision, can you provide me the average time difference between sent and received time? I know this highly depends on operator and signal strength.
I think this late time info would only matter if it is abnormal and worth the attention of user. Since showing the info consumes precious screen space and normally people won't care if the message is only a few minutes late, let's simply point out the ones that is received very late, and for that, we need to know how late is too late. Let me know, tks!

Comment 6

[Julien Wajsberg [:julienw]]

I'd say we can warn when the delay is > 5 minutes. In my daytoday use, it's when the delay starts being a concern. I think that most of the time the delay is less than this in my normal use (unless I am in the subway, etc, and in that case it's also nice to know). I'll check on my latest and future received messages and report if I find anything else :)

Comment 7

[Mozilla_BEC]

Jenny, I'm of the mind that if it is more than a minute delayed, it is late.

This becomes problematic when you're having a sustained conversation as any messages that arrive late will ruin the flow of conversation and can make things confusing for both parties.

Would a threshold settable in the user.js be an acceptable way to make this tunable for those with the inclination/knowledge to do so?

As for the consumption of screen space, I'm not sure I get your concern. In my mock-up, I have the late stamp within the padding of the chat bubble. That was all dead space to begin with and the late stamp doesn't reduce available screen real-estate in that instance.

Were you thinking of having it be its own timestamp or line or?

Comment 8

[Jenny Lee]

(In reply to Brett Edmond Carlock from comment #7)

Hello Brett, can you elaborate on the flow of conversation being ruined part?

IMO, if user will not take action seeing the "late time" info, we shouldn't show it at all. For example, as Julien mentioned, when in subway, the message can be received late but there's is nothing user can/will do about it (of course unless there's another operator who has better signal coverage at subway stations and user is wiling to change operator). Even if we are using the dead space (note that it might not appear to be dead space in some other cases), the info is still redundant. This might also reflects badly on operator.  

We can consider providing a tunable threshold UI. As for the actual placement of the late time info, I need to know exactly how are we going to implement this feature then decide.

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Jenny Lee from comment #8)
> (In reply to Brett Edmond Carlock from comment #7)
> 
> Hello Brett, can you elaborate on the flow of conversation being ruined part?
> 
> IMO, if user will not take action seeing the "late time" info, we shouldn't
> show it at all. For example, as Julien mentioned, when in subway, the
> message can be received late but there's is nothing user can/will do about
> it (of course unless there's another operator who has better signal coverage
> at subway stations and user is wiling to change operator). Even if we are
> using the dead space (note that it might not appear to be dead space in some
> other cases), the info is still redundant. This might also reflects badly on
> operator.  

> As for the actual
> placement of the late time info, I need to know exactly how are we going to
> implement this feature then decide.


I think the point is especially to show this info in case it goes out of the normal case, without needing to go to the other panel.

I think it would be good enough to show a pictogram that means the message was late, without displaying the real information. If the user wants to know more he can check out the panel.

> 
> We can consider providing a tunable threshold UI.

I'd rather do not. I prefer less options, more sensible defaults.

Comment 10

[Mozilla_BEC]

(In reply to Jenny Lee from comment #8)
> (In reply to Brett Edmond Carlock from comment #7)
> 
> Hello Brett, can you elaborate on the flow of conversation being ruined part?
> 
Yes, please see this attached YouTube video for a prime example of how a late arrival can butcher the flow of a conversation and confuse both parties.

http://youtu.be/qKsxbLf0g7s

Sadly, under FFXOS, late arrival is a far, far, far more common an occurrence than I would like.

Comment 11

[Jenny Lee]

(In reply to Brett Edmond Carlock from comment #10)

Thanks for the video Brett, that helps! I'm working on the spec for this feature and I'll get back to you when it's done!

Comment 12

[Mozilla_BEC]

Thanks Jenny!

I really think making this feature logical and easy to understand is critical, so I'd really love to see what you have figured out for the spec.

I'm sure I'll have plenty of opportunity to test it out sadly :P

Comment 13

[Jenny Lee]

Attachment: [Message] Late indicator v1.0.pdf

Hi Brett, see attached for the initial draft spec. Note that I included a "reset message order" feature which is not in scope of this bug. I think it'd make the late indication feature more complete, however, to implement this feature involves gecko effort, so feel free to simply focus on the indication part. Also note that visual style is TBD. Thanks!

Comment 14

[Mozilla_BEC]

Jenny,

I really like your idea of "resetting" the flow of conversation. Brilliant! (And to my knowledge, no other platform does this.)

I'm still going to have to pull for having a quick time-stamp drawn next to the indicator icon simply because that is, to my mind, the best place to put it.

What would you say to using that icon as a clock icon to show (roughly, say 5min increments?) about how late the message was using the icon. In this way, the user can get a quick idea of how late it was, and could see exactly how late when they view the report (if they so choose).

Comment 15

[Julien Wajsberg [:julienw]]

Note that Jenny merely does the UX design (means: behavior, which information should be displayed, etc), for Visual Design Fang is coming up :)

Comment 16

[Julien Wajsberg [:julienw]]

Reading bug 1118439, what happens sometimes is that we receive a bunch of SMS when we power up the phone. I feel having a simple "late indicator" is not enough in that case, and Brett's idea in attachment 8454646 [details] makes more sense.

What do you think Jenny?

Comment 17

[Jenny Lee]

Hello Julien,
In this case, I think showing minutes is too much, if there are tons of messages that are late at the same time, would it really matter exactly how late each single message arrived? A general impression of "this group of messages are late" would seem enough and a lot visually less cluttered. 

I suppose we can alter the look of late clock indicator to give a sense of how late the message is as Brett mentioned in comment 14. 
ni Fang for visual comment, thanks Fang =)

Comment 18

[Mozilla_BEC]

Jenny,

Do you have a clock asset you'd want me to modify? 

I was making one up myself in SVG, but after making the various states I realized it'd likely not be good enough and would be either thrown out, or would require almost as much work to fix it as it would to make it fresh, so I abandoned it.

Comment 19

[Julien Wajsberg [:julienw]]

Hope Fang will be able to provide one soon :)

Comment 21

[Fang Shih [:fang]]

Attachment: late_message_spec.zip

After an offline discussion with Jenny, we think it would be better to show both clock and minutes.
And use a slightly darker color to distinguish between two different times and also a clock icon as a hint. I've attached the spec with clock assets and mock up. Let me know if it works! 
Thanks!

Comment 22

[Mozilla_BEC]

Fang,

I like showing the clock icon and the minutes (the minutes were what I was after in the beginning, after all :P).

What do you think about having the clock icon have different "gross"/coarse time states on it? Redundant? Unnecessary? To me, I think it'd look cleaner if the clock and the text following it weren't completely divorced from one another.

If not the 5 minute increments, perhaps 10 or 15 minutes would be coarse enough?

In any case, this will be very, very useful for me as I still get many late arrivals and late sends (perhaps late send should be a case we need to look into for visualizing as well?)

Comment 23

[Jesse Ruderman]

It should simply show when the message was sent, e.g. "Just now" or "17 minutes ago".

All that matters for replying is knowing whether the message is still fresh.

It's irrelevant whether the message is old due to:
* network problems (on either end)
* because my phone was off
* because my ringer was off
* because I failed to hear it ring

A "lateness" indicator would distinguish the first one (or first two?) from the others, but this distinction isn't important most of the time.

Comment 25

[Julien Wajsberg [:julienw]]

Hey Fang,

I know it's a late feedback :D I find that the color you chose is not contrasted enough for accessibility.

You can check the colors on http://webaim.org/resources/contrastchecker/:

background: #39C3C2
text: #026a69

For small text we need at lesat a contrast of 4.5 if we want to pass AA.

See also http://www.w3.org/WAI/WCAG20/quickref/#qr-visual-audio-contrast-contrast and http://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-contrast.html for more information about color accessibility and contrast.

Comment 26

[Stéphane Roucheray]

I am working on this.

Comment 27

[Julien Wajsberg [:julienw]]

Thanks Stéphane !

Comment 28

[Stéphane Roucheray]

As we have to humanize a duration between a sent and a received time, I would like to share some cases before exposing to you my assumptions.

What should we display for a late arrival of :
 1) 1min30sec : "1m late" or "2m late" or "1m30s late"
 2) 1min59sec : "1m late" or "2m late"or "1m59s late"
 3) 2 hours : "2h late" or "120m late"
 4) 47 hours : "2d late" or "47h late"

I have made some assumptions (please, tell me if you disagree) based on two facts:
* we don't have much space to display the late arrival notice in the conversation view
* as a secondary information, late arrival notice, while being visible should not clutter the UI

So here are my assumptions:
1 - no decimal values (it has no sense, time is not based on a radix of 10)
2 - only one integer value, to keep it short (good: 2h late, bad: 2h 22min 30s late)
3 - be relevant (as much accurate as we can) while being concise (good: 46 hours late, bad: "1 day late" or "2 days late")

Based on these assumptions here is what I suggest, if late arrival...
... is less than a lower bound (say 1 minute) : don't display the notice
... is less than 2 hours : display notice in minutes, ex. 80m late
... is less than 2 days : display notice in hours, ex. 36h late
... is less than 2 months : display notice in days, ex.  40 days late
... is less than 2 years : display notice in months, ex. 18 months late
... is more than 2 years (is it possible ?) display in years : 3 years late

Why choosing 2 (hours, days, months, years) instead of 1 ? IMO below 2, it's better to be more accurate (better to say 90m late than 1h late or 2h late). Above 2 it's more difficult to figure out what the value means, thus in this case less accuracy seems more relevant (better to say 18h late than 1054m late). I admit that choosing 2 is based on a personal feeling, tell me if you have other feeling.

Finally should we display the late arrival notice in the message report view ?
If yes, as we have more space we could humanize the delay in more accurate way, ex: 2h 22min 30s late
What do you think ?

Comment 29

[Julien Wajsberg [:julienw]]

I think all you write here makes sense :)
I don't know if we can receive a message older than a few days though. Maybe Bevis will know. (NI Bevis for this)
If we can't have a message older than (say) 7 days, it will also simplify your proposal a lot.

Let's also ask our new UX designer Morpheus !

Hey Bevis, Morpheus, for some context, you can look at the existing spec that is attached to this bug.

Comment 30

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] from comment #29)
> I think all you write here makes sense :)
> I don't know if we can receive a message older than a few days though. Maybe
> Bevis will know. (NI Bevis for this)

Yes, it's possible to receive a message sent few days ago if that message was not expired in the server side.
For both SMS and MMS, there could be an "expiration period" predefined by the carrier if not set or directly provided by the sender.
Hence, if a message was delivered to the service center and the recipient was offline, the message will be either expired or delivered to the recipient if the recipient is online before expired.

> If we can't have a message older than (say) 7 days, it will also simplify
> your proposal a lot.
> 
> Let's also ask our new UX designer Morpheus !
> 
> Hey Bevis, Morpheus, for some context, you can look at the existing spec
> that is attached to this bug.

Comment 31

[Julien Wajsberg [:julienw]]

Bevis, do you know what is the typical expiration period? I'd say it's never more than a few days. I think it's 3 days here in France.

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> Bevis, do you know what is the typical expiration period? I'd say it's never
> more than a few days. I think it's 3 days here in France.

Sorry to say that I don't have the answer as well. This depends on carrier's configuration if not set.
But if it was set by the user, it is possible to be longer than 1 week according to 3GPP TS 23.040.
http://www.etsi.org/deliver/etsi_ts/123000_123099/123040/11.05.00_60/ts_123040v110500p.pdf
Take 9.2.3.12.1 TP-VP (Relative format) as example:
The representation of time is as follows:
TP-VP value   Validity period value
0 to 143      (TP-VP + 1) x 5 minutes (i.e. 5 minutes intervals up to 12 hours)
144 to 167    12 hours + ((TP-VP -143) x 30 minutes)
168 to 196    (TP-VP - 166) x 1 day
197 to 255    (TP-VP - 192) x 1 week

Comment 33

[Julien Wajsberg [:julienw]]

So the technical maximum is 63 weeks, which is 441 days.

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> So the technical maximum is 63 weeks, which is 441 days.

But it could be shorter if carrier can not promise to keep this message so long. :(

Comment 35

[Julien Wajsberg [:julienw]]

Of course :) I think in real life it's always a few days, 1 week max, but we must have code that works for the max technical.

Comment 36

[Stéphane Roucheray]

Ok, thanks for the infos and reference. So at most we will use months, years are not needed.

Comment 37

[Fang Shih [:fang]]

(In reply to Julien Wajsberg [:julienw] from comment #25)
> Hey Fang,
> 
> I know it's a late feedback :D I find that the color you chose is not
> contrasted enough for accessibility.
> 
> You can check the colors on http://webaim.org/resources/contrastchecker/:
> 
> background: #39C3C2
> text: #026a69
> 
> For small text we need at lesat a contrast of 4.5 if we want to pass AA.
> 
> See also
> http://www.w3.org/WAI/WCAG20/quickref/#qr-visual-audio-contrast-contrast and
> http://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-contrast.
> html for more information about color accessibility and contrast.

Hi Julien,

Sorry for the late reply. 
And thanks for the feedback. For the text color, I understand it may not pass AA, but originally it should be more like a hint. That's why I prefer to make it less pop, so it won't take over the main message. It will be more like a secondary visual item. Thanks!

Comment 38

[Morpheus Chen [:morpheus] UX]

Thanks for Stéphane providing the rules, it's really thoughtful.

But I would like to suggest making things easier. In my opinion, the main purpose of late time stamp shown in the bubbles is to inform user the message is late to prevent misunderstandings and keep conversation going. So I think it doesn't have to be "precisely" accurate since user can find message report for the most accurate late notice. Based on Stéphane's suggestion, my proposal is to show below if late arrival...

... is less than a lower bound (say 1 minute) : don't display the notice
... is less than 1 hours : display notice in minutes, ex. 59m late
... is less than 1 days : display notice in hours, ex. 23h late
... is less than 1 months : display notice in days, ex.  28 days late, 1 month late.

My purpose is to make the rule simple and understandable to users and keep Message app more focuses on communicating. If user really needs a precisely accurate late arrival time, user can find the message report for the more accurate time as Stéphane mentioned. What do you think?

Comment 39

[Stéphane Roucheray]

Hi Morpheus, Thanks for your feedback. Ok, I'll go with this simpler version, I just need a clarification.

> ... is less than 1 months : display notice in days, ex.  28 days late, 1 month late.
Can you confirm that you meant when lateness is less than 1 month -> display in days ; and when lateness is more than a month -> display in months ?
If yes, for lateness more than a month how would you reduce the accuracy of the value. We have 3 options :
1 - floor : 32 days and 59 days will translate to 1 month
2 - ceil : 32 days and 59 days will translate to 2 months
3 - round: 32 days will translate to 1 month and 59 days will translate to 2 months

I would choose round but floor might be good as well. What would be your choice ?

Comment 40

[Morpheus Chen [:morpheus] UX]

(In reply to Stéphane Roucheray from comment #39)

> > ... is less than 1 months : display notice in days, ex.  28 days late, 1 month late.
> Can you confirm that you meant when lateness is less than 1 month -> display
> in days ; and when lateness is more than a month -> display in months?
> If yes, for lateness more than a month how would you reduce the accuracy of
> the value. We have 3 options :
> 1 - floor : 32 days and 59 days will translate to 1 month
> 2 - ceil : 32 days and 59 days will translate to 2 months
> 3 - round: 32 days will translate to 1 month and 59 days will translate to 2
> months
> 
> I would choose round but floor might be good as well. What would be your
> choice ?

Sorry, my bad, I didn't make it clearer. IMO, I would suggest floor since it's more reasonable and safest way to prevent misunderstanding.

Comment 41

[Julien Wajsberg [:julienw]]

Also, can we say that 1 standard month is 30 days for these calculations ? Do you have something else in mind ?

Comment 42

[Stéphane Roucheray]

Julien this is what I have done and yes, I have two other questions.

1) Fang, in the UI spec (Spec.jpg) you set the space between the time and the late arrival icon to be 0.1rem but the root font-size is 10px which means I have a 1px space. It seems that the space in your design is 10px wide so I suppose you meant 1rem instead of 0.1rem. Can you confirm or did I miss something ?

2) Julien, for RTL languages, I suppose the clock icon must be placed to the right of the text, this can be done in CSS[1], but the whole notice (icon + text) must be placed on the left of the time, as well, and I am not sure how to handle that. Do I have to create two templates or dynamically insert the notice on the left when RTL ? Can you point me to existing examples of such case ?

[1] https://wiki.mozilla.org/Gaia/CSS_Guidelines#Direction_.28RTL.2FLTR_and_BiDi.29

Comment 43

[Stéphane Roucheray]

Ok, I answer my own question for (2), css property `direction:rtl;` seems the way of doing it and it works.

Comment 44

[Stéphane Roucheray]

(In reply to Stéphane Roucheray from comment #43)
> Ok, I answer my own question for (2), css property `direction:rtl;` seems
> the way of doing it and it works.

Actually after few more testings -moz-dir(rtl) and -moz-dir(ltr) do the trick.

Comment 45

[Julien Wajsberg [:julienw]]

about 1) yeah I think it's 1rem.

about 2) You can also use flexible layout (aka flex box) which adapts to LTR/RTL automatically.

Actually, the ".message-details" element is "display: flex" already :) I think we already have the same issue when we display the "delivery/read" icons for sent messages. So I think this is the way to go.

Comment 46

[Stéphane Roucheray]

> about 2) You can also use flexible layout (aka flex box) which adapts to
> LTR/RTL automatically.
> 
> Actually, the ".message-details" element is "display: flex" already :) I
> think we already have the same issue when we display the "delivery/read"
> icons for sent messages. So I think this is the way to go.
I was trying to modify the bad iframe's 'dir' attribute and nothing changed until I realised that :) So because of 'display:flex' there is nothing to do apart changing margin orientation on the icon with -moz-dir :)

Thanks Julien

Comment 47

[Julien Wajsberg [:julienw]]

Even margin and padding and border can be done in a way that's automatically handled by Gecko.

see https://wiki.mozilla.org/Gaia/CSS_Guidelines#Direction_.28RTL.2FLTR_and_BiDi.29 :)

Comment 48

[GitHub Autolander Bot]

Attachment: [gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

Comment 49

[Stéphane Roucheray]

Hey Julien,

Not really sure you were flagged for the review. Can you look at the patch please ?

Comment 50

[Julien Wajsberg [:julienw]]

For next time, to ask a review, you need to press the "Details" link next to your attachment, and here you'll have on the right a "review" select box. You can select '?' and add the bugzilla handle of someone to review the patch (in that case, myself :) ).

Comment 51

[Stéphane Roucheray]

(In reply to Julien Wajsberg [:julienw] from comment #50)
> For next time, to ask a review, you need to press the "Details" link next to
> your attachment, and here you'll have on the right a "review" select box.
> You can select '?' and add the bugzilla handle of someone to review the
> patch (in that case, myself :) ).

Ah, Ok ! I thought Autolander did it itself grabbing the r=? string in the PullRequest. No I know :)

Comment 52

[Stéphane Roucheray]

> PullRequest. No I know :)
Now I know ;)

Comment 53

[Julien Wajsberg [:julienw]]

I left some comments on github but haven't actually tried the patch yet, so I'm keeping the review request for now.

Comment 54

[Julien Wajsberg [:julienw]]

BTW please add 1 separate commit containing your changes :)

Comment 55

[Stéphane Roucheray]

(In reply to Julien Wajsberg [:julienw] (PTO Sept 23rd) from comment #54)
> BTW please add 1 separate commit containing your changes :)

Do you mean that my PR should only include 1 commit for all changes ? 
If that's the case, what must I do to fix issues you commented on github, do I need to amend my previous commit or to commit all *new* changes in 1 commit or something else ?

Comment 56

[Julien Wajsberg [:julienw]]

I mean:

* 1 commit with the changes corresponding to the first review (that means squashing your 3 first commits)
* another commit with the changes to fix the comments I made, so that I can look at this commit only for the next review
* if we do another review round, you'll add another commit
etc

does it make sense ? :)

Comment 57

[Julien Wajsberg [:julienw]]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

I tested on the device, this seems to work well. I confirmed the issue with the image on high-dpi devices though. More info on github.

Thanks a lot for your work, this was already quite solid for a first review round !

Comment 58

[Stéphane Roucheray]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

Thanks Julien,

I pushed a commit trying to fix issues and answering your comments. 

As I commented on github I managed to get HighDPI icons in a pseudo element with background-image and background-size. I am not 100% sure it works as expected but I prefer pseudo element because IMO the icon decorates the text notice so it avoids adding unnecessary HTML elements just for the icons. Tell me if you don't agree :)

Comment 59

[Julien Wajsberg [:julienw]]

See bug 1207131 for the CSSLint issue.

Comment 60

[Julien Wajsberg [:julienw]]

Hey Fang, one more question for you. The "late arrival notice" has a smaller font than what is already on the line (the time for example). Should it be vertically centered compared to the other text, or should it be on the same baseline ?

Thanks !

Comment 61

[Julien Wajsberg [:julienw]]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

Hey Stéphane,

I left some more comments, both on the PR and the additional commit [1].
I also reached Fang directly by mail about the additional question, so I think we'll have the answer soon :)

[1] https://github.com/sroucheray/gaia/commit/74e3054c1053dbe6d1e1e397b9ba0191f787436a (putting the link because it's easy to lose when rebasing)

Comment 62

[Julien Wajsberg [:julienw]]

And as previously, please add your changes in a separate commit :) Thanks again !

Comment 63

[Julien Wajsberg [:julienw]]

Hey Morpheus,

here is another question for you, sorry ;)

In the unusual case where an incoming message is in error state (I _think_ this can happen if we can't retrieve a MMS, for example), do you think we should still display the Late Arrival Notice here? I think we should not display it in that case.

Thanks !

Comment 64

[Morpheus Chen [:morpheus] UX]

(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #63)
> Hey Morpheus,
> 
> here is another question for you, sorry ;)
> 
> In the unusual case where an incoming message is in error state (I _think_
> this can happen if we can't retrieve a MMS, for example), do you think we
> should still display the Late Arrival Notice here? I think we should not
> display it in that case.
> 
> Thanks !

I think we should not display late arrival notice until error state is removed, for instance, show notice after user retrieves a MMS.

Comment 65

[Stéphane Roucheray]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

Hey Julien !
I did some changes based on your comments, as a summary :

- Change minimum notice display from 1 to 5 minutes
- Late arrival icon has its own element + arial label
- Use setAttributes to translate late notice arrival in conversation view (do not use Promise anymore)
- Use querySelector locally in information panel instead of putting it in the panel init
- Clean up CSS (remove flex, remove ~ selector, update icon size, decrease xfailist error counts)
- Change test dynamic titles generation in conversation_test.js

Comment 66

[Fang Shih [:fang]]

(In reply to Stéphane Roucheray from comment #42)
> Julien this is what I have done and yes, I have two other questions.
> 
> 1) Fang, in the UI spec (Spec.jpg) you set the space between the time and
> the late arrival icon to be 0.1rem but the root font-size is 10px which
> means I have a 1px space. It seems that the space in your design is 10px
> wide so I suppose you meant 1rem instead of 0.1rem. Can you confirm or did I
> miss something ?
> 


Hey Stephane, 
You are right, it's supposed meant 1 ram instead of 0.1 REM. Sorry about that. Thanks for pointing it out.

Comment 67

[Fang Shih [:fang]]

(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #60)
> Hey Fang, one more question for you. The "late arrival notice" has a smaller
> font than what is already on the line (the time for example). Should it be
> vertically centered compared to the other text, or should it be on the same
> baseline ?
> 
> Thanks !

Hey Julien, Sorry for the late reply. 
Would it be possible for changing the "late arrival notice" font size to the same font size as the time ( 1.4rem )? Originally I wanted to make it smaller so it shows more like a secondary information, but after looking at it awhile, it actually looks fine with the same size because we have it in different color already. So do you think we can change it to 1.4rem? That way we can solve the alignment problem too. Hope it works for you too, Thanks!

Comment 68

[Stéphane Roucheray]

Julien, as I have not seen any activity yet on my last commit on github I amended it with this change. Now the notice has a font-size of 1.4em.

Comment 69

[Julien Wajsberg [:julienw]]

I left few comments but I didn't try the new version on a device yet.  This really looks good, I think we can land this very soon ! (if I review quicker than what I did lately...)

Comment 70

[Julien Wajsberg [:julienw]]

The comments are at this commit: https://github.com/sroucheray/gaia/commit/37ad63f1883cb8245d974bc87295efc7b2c1f392

One of the biggest comments I had turned out to be wrong, so I think only small comments are left. Tell me when you're ready !

Comment 71

[Stéphane Roucheray]

I answered some of your comments on GitHub. Please tell me what do you think before I push a new commit !

Comment 72

[Julien Wajsberg [:julienw]]

Answered !
push it to a new commit ;)
(and please rebase as well)

Comment 73

[Julien Wajsberg [:julienw]]

(Added 1 more comment in the PR too)

Comment 74

[Stéphane Roucheray]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

I did the last changes and a rebase as well !

Comment 75

[Stéphane Roucheray]

Oh ! And I had missed your comment about adding default parameter in the template library. 
Do you think it can be done here or shouldn't we open another bug for that ?

Comment 76

[Julien Wajsberg [:julienw]]

I think it's simple enough that we can do it here.

Comment 77

[Julien Wajsberg [:julienw]]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

sorry, I failed to see some needed changes, especially:
- Some unit tests
- time-unit-based names -> "value" in the locale file

And I think you can remove the option to Template's "prepare" without touching the lib... But maybe I'm missing something here.

And you need to rebase once more.

You can squash all the changes in one commit now, and add "r=julien" at the end of the commit log. I think we'll be ready at the next review so better make it ready to land :)

Comment 78

[Stéphane Roucheray]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

Hey Julien,

I added missing unit tests, extracted the 'narrow' unit l10n strings, rebased to master and squashed to a single commit ! Please can you review it !

Comment 79

[Julien Wajsberg [:julienw]]

Comment on attachment 8663143 [details] [review]
[gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master

r=me with the additional comment.

Once you did it, push to your PR, wait that the treeherder is green, and add "checkin-needed" in the "keywords" section of the bug.

Great job, thanks for this very nice feature !

Comment 80

[Stéphane Roucheray]

(In reply to Julien Wajsberg [:julienw] from comment #79)
> Comment on attachment 8663143 [details] [review]
> [gaia] sroucheray:1037620-late-arrival-notice > mozilla-b2g:master
> 
> r=me with the additional comment.
> 
> Once you did it, push to your PR, wait that the treeherder is green, and add
> "checkin-needed" in the "keywords" section of the bug.
> 
> Great job, thanks for this very nice feature !

Comment 81

[Stéphane Roucheray]

Thanks for the review Julien. Great and very instructive experience !

Comment 82

[Julien Wajsberg [:julienw]]

BTW Stéphane, can you file a separate bug to change the template library to support having no parameter ?

Comment 83

[Stéphane Roucheray]

Here it is https://bugzilla.mozilla.org/show_bug.cgi?id=1214509

Comment 84

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/18fca1ea74e7a41049e7d44bebecbc6a8876896d

Comment 85

[Zibi Braniecki [:zbraniecki][:gandalf]]

Quick question for the follow up: Would you be ok with changing "Late arrival: 5 months late" to "Message sent: 5 months ago"?

This way we could remove the strings and reuse RelativeTimeFormat from Intl which has strings like "5 months ago", "2 minutes ago" and will soon be part of the Ecma 2016.

Comment 86

[Julien Wajsberg [:julienw]]

I like the precision of using "late"... but let's discuss about this in a post-2.5 follow-up, OK? I don't think we'll use this new API from Intl in 2.5 anyway.

Comment 87

[Zibi Braniecki [:zbraniecki][:gandalf]]

Well, we have it already polyfilled in mozIntl, so we could, and it would reduce the amount of localization work so it would be a value to consider this in 2.5.

But if you like the precision, I think we can try to do - "5 months" using UnitFormat from mozIntl and "{{relativetime}} late" in your custom l10n string. But UnitFormat is post 2.5, so then we don't have to hurry and we'll add some work for localizers in 2.5.

Comment 88

[Stéphane Roucheray]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #85)
> Quick question for the follow up: Would you be ok with changing "Late
> arrival: 5 months late" to "Message sent: 5 months ago"?

IMO it has not the same meaning. The message could have been received 10 months ago with a delay of 5 months (diff between received and sent time) thus 5 months late. The lateness indicator brings a different level of information from a simple humanized received time.

Comment 89

[Julien Wajsberg [:julienw]]

I agree with Stéphane here.

Also I don't really like your idea with "{{ relativetime }} late"... Couldn't this be problematic for some locales ?