963013 - [Messages][Refresh] Update bubbles style and layout of the thread.

Status: VERIFIED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

The goal of this bug is to implement the new layout based on https://bug951672.bugzilla.mozilla.org/attachment.cgi?id=8361733

In this bug *the photo of the user will not be included in the layout of the bubbles*. This will be solved in other bug, keeping an eye on the performance. I'll attach the bug number as a dependency.

Comment 1

[Jason Smith [:jsmith]]

VD refresh issues are not blockers for the release, so not blocking.

Comment 2

[Oleg Zasypkin [:azasypkin]]

Hey Joan, do you have plans to work on this any time soon?

Comment 3

[Joan Leon [:joan]]

Hi Aleh, tomorrow I want to start with it

Comment 4

[Oleg Zasypkin [:azasypkin]]

(In reply to Joan Leon [:joan] from comment #3)
> Hi Aleh, tomorrow I want to start with it

Great! Thanks for the update, just trying to figure out what we can proceed with and what is dependent on Building Blocks so that we can push it forward.

Comment 5

[Julien Wajsberg [:julienw]]

Joan, basically, Oleg can help you on the Visual Refresh, so you can assign him some bugs if necessary.

Comment 6

[Oleg Zasypkin [:azasypkin]]

Hey Joan, it looks like bug 980461 overlaps with this one. Could you please let me know what of the issues described in bug 980461 (see attachment) are going to be fixed in the scope of the patch you're working on? I just like to avoid double work.

Comment 7

[Joan Leon [:joan]]

Hi Oleg, seeing the Bug 980461 refers to the attachment, I'm not working on them. After a few days of vacation, I return to work and this afternoon I will spend a PR of my branch

Comment 8

[Julien Wajsberg [:julienw]]

Joan, do you need help with this bug?

Comment 9

[Joan Leon [:joan]]

Hi Julien, yo function header date with property stiky the groups: 

<header>...</header>
<ul>...</ul>

should be in a container, such as <section>. 

<section>
   <header>...</header>
   <ul>...</ul>
</section>

It is controlled from JS, since the list is assembled dynamically.

Comment 10

[Julien Wajsberg [:julienw]]

Joan, do you mean that this bug should only update how the JS generates the markup? Then maybe the SMS team can take it?

Is there a pending patch in bug 951672 that we can use to test our code?

Comment 11

[Julien Wajsberg [:julienw]]

Joan, we discussed internally and we can take this bug for this sprint, please answer the question in comment 10 and we'll be able to move forward.

Thanks!

Comment 12

[Joan Leon [:joan]]

Hai Julien,

the only thing missing is that the header + ul groups are in a container, as <section>. Should be changed from JS, I think if you look at a person skilled in JS may expedite this Bug.

Comment 13

[Julien Wajsberg [:julienw]]

This will be taken by someone from the SMS team.

Comment 14

[Oleg Zasypkin [:azasypkin]]

I'm looking into it right now.

Comment 15

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (wrapper for header+ul) [checked in: comment 36]

Added WIP patch for wrapping date header and message list into single section. Patch based on patch from bug 871432.

Joan, could you please preliminarily check this out and say whether it's what you was missing to fully resolve this bug?

Thanks!

Comment 16

[Joan Leon [:joan]]

Hi Oleg,

I have conflicts to apply the patch… you can do a rebase and you let me know?

Thanks

Comment 17

[Oleg Zasypkin [:azasypkin]]

Hey Joan, I've just rebased my PR, please check it once again.

Thanks!

Comment 18

[Joan Leon [:joan]]

Attachment: bubbles thread 1

Comment 19

[Joan Leon [:joan]]

Attachment: bubbles thread 2

Comment 20

[Joan Leon [:joan]]

Attachment: bubbles thread sticky date (push previous date)

Comment 21

[Joan Leon [:joan]]

Attachment: bubbles thread sticky date

Comment 22

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8424885 [details]
bubbles thread 2

Can you please use regular font for timestamp and put the subheader text (99791495) in #575757? thanks

Comment 23

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8424886 [details]
bubbles thread sticky date (push previous date)

Can you please use regular font for timestamp and put the subheader text (99791495) in #575757? thanks

Comment 24

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8424887 [details]
bubbles thread sticky date

Can you please use regular font for timestamp and put the subheader text (99791495) in #575757? thanks

Comment 25

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8424883 [details]
bubbles thread 1

Sticky header should contain date but not time if the time is displayed inside the bubbles. 

Can you please use regular font for timestamp and put the subheader text (99791495) in #575757? thanks

Comment 26

[Julien Wajsberg [:julienw]]

That's strange because we definitely removed the time from the headers in bug 871432. I think one rebase regressed it.

Comment 27

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> That's strange because we definitely removed the time from the headers in
> bug 871432. I think one rebase regressed it.

Yep, it was an issue in my template, now it's fixed.

Comment 28

[Joan Leon [:joan]]

Attachment: Patch in GitHub with Styles for VR

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 8423706 [details] [review]
GitHub pull request URL (wrapper for header+ul) [checked in: comment 36]

This looks good,

I also checked that performance-wise there is no regression.

(pretty empirically: I run this in one shell:

  adb shell top -s cpu -d 1 -m 3 | grep <message pid> | wc -l

and this in another shell:

  adb shell top -s cpu -d 1 -m 3

then open big thread, then wait that the process disappears in the second shell, then "killall adb".)

I'd like a second round of review: can you please fix the nits in a separate commit to make the second review easy?

Thanks !

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 8425445 [details] [review]
Patch in GitHub with Styles for VR

added lots of comments on github

this looks good but I think we can simplify and clean up more things + one small remaining issue.

If you can add your next changes in a separate commit, it would make it easier to review the next round.
Please request review once you're ready !

Thanks !

Comment 31

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8423706 [details] [review]
GitHub pull request URL (wrapper for header+ul) [checked in: comment 36]

(In reply to Julien Wajsberg [:julienw] from comment #29)
> Comment on attachment 8423706 [details] [review]
> GitHub pull request URL (wrapper for header+ul)
> 
> This looks good,
> 
> I also checked that performance-wise there is no regression.
> 
> (pretty empirically: I run this in one shell:
> 
>   adb shell top -s cpu -d 1 -m 3 | grep <message pid> | wc -l
> 
> and this in another shell:
> 
>   adb shell top -s cpu -d 1 -m 3
> 
> then open big thread, then wait that the process disappears in the second
> shell, then "killall adb".)
> 
> I'd like a second round of review: can you please fix the nits in a separate
> commit to make the second review easy?
> 
> Thanks !

Thanks for review! I've fixed nits and added more "ThreadUI.appendMessage" tests.

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8423706 [details] [review]
GitHub pull request URL (wrapper for header+ul) [checked in: comment 36]

r=me for the structure refactoring patch

Comment 33

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #32)
> Comment on attachment 8423706 [details] [review]
> GitHub pull request URL (wrapper for header+ul)
> 
> r=me for the structure refactoring patch

Thanks for review! Also as we agreed I've filed follow-up bug 1014686.

Travis is green, do you want this patch to be landed only once CSS part is ready or we can land it separately?

Comment 34

[Julien Wajsberg [:julienw]]

Oleg, since it does not break the current app, you can land now this patch.

Comment 35

[Oleg Zasypkin [:azasypkin]]

Note to sheriffs: please, land "GitHub pull request URL (wrapper for header+ul)" patch only. Second one will follow later.

Thanks!

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8423706 [details] [review]
GitHub pull request URL (wrapper for header+ul) [checked in: comment 36]

Master: https://github.com/mozilla-b2g/gaia/commit/30e0a8a6665433a605adb7ee0dfb91a67e4727d6

Comment 37

[Oleg Zasypkin [:azasypkin]]

Hey Victoria,

Can you let us know what we should do with the long MMS subject? In the current Joan's patch ellipssis is used ("..."), but currently it's wrapped into two lines.

Thanks!

Comment 38

[Oleg Zasypkin [:azasypkin]]

Attachment: Colors and text for incoming and outgoing SMS

Hey Victoria, I'm going to post several screenshots for you to review. I'll add comment for every screenshot to what I'd like to look at :):

* Colors and text for incoming and outgoing SMS

Comment 39

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header (default state)

* Sticky date header (default state, at the top)

Comment 40

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header (intermediate state)

* Sticky date header (intermediate state, overlaying message)

Comment 41

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header + subheader (carrier header), default state

* Sticky date header + subheader (carrier header), default state

Comment 42

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header + subheader (carrier header), intermediate state

* Sticky date header + subheader (carrier header), intermediate state. Sticky header is overlaying message with transparency while subheader is in a plain white color.

Comment 43

[Oleg Zasypkin [:azasypkin]]

Attachment: Incoming MMS with subject

* Incoming MMS with subject, color and text of the subject

Comment 44

[Oleg Zasypkin [:azasypkin]]

Attachment: Outgoing MMS with subject

Outgoing MMS with subject, colors and text

Comment 45

[Oleg Zasypkin [:azasypkin]]

Attachment: Long subject (wrap into two lines)

* Long subject (wrap into two lines);
* Error state icon;
* Error state colors and text.

Comment 46

[Oleg Zasypkin [:azasypkin]]

Attachment: Long subject (ellipsis)

* Long subject (ellipsis). Related to the comment 37

Comment 47

[Oleg Zasypkin [:azasypkin]]

Attachment: File name (changed to italic font style)

* File name (changed to italic font style)

Comment 48

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429229 [details]
Sticky date header (default state)

The text inside the sticky header looks a bit light, can you please make it #575757?

thanks!

Comment 49

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429230 [details]
Sticky date header (intermediate state)

Transparency is ok. (please fix the text color for the date)

Comment 50

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header (default state)

* Sticky date header (default state), updated text color to #575757

Does it look better now?

Thanks!

Comment 51

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429241 [details]
Long subject (ellipsis)

Ellipsis OK. But the "!" icon has a wrong alignment

Comment 52

[Oleg Zasypkin [:azasypkin]]

(In reply to Victoria Gerchinhoren [:vicky] from comment #51)
> Comment on attachment 8429241 [details]
> Long subject (ellipsis)
> 
> Ellipsis OK. But the "!" icon has a wrong alignment

Just to confirm, in case of ellipsis user won't have ways to see full subject if it's too long, is that OK?

Comment 53

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429240 [details]
Long subject (wrap into two lines)

Can you please check that the subject text is #333333, it looks lighter.

"!" icon is not well aligned, it should be vertically centered in the available bubble space, minus the subject strip.

Please make the line height 1.7 rem for the 2 lines subject.

Comment 54

[Victoria Gerchinhoren [:vicky]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #52)
> (In reply to Victoria Gerchinhoren [:vicky] from comment #51)
> > Comment on attachment 8429241 [details]
> > Long subject (ellipsis)
> > 
> > Ellipsis OK. But the "!" icon has a wrong alignment
> 
> Just to confirm, in case of ellipsis user won't have ways to see full
> subject if it's too long, is that OK?

How's specified in the interaction specs?

Comment 55

[Oleg Zasypkin [:azasypkin]]

(In reply to Victoria Gerchinhoren [:vicky] from comment #54)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #52)
> > (In reply to Victoria Gerchinhoren [:vicky] from comment #51)
> > > Comment on attachment 8429241 [details]
> > > Long subject (ellipsis)
> > > 
> > > Ellipsis OK. But the "!" icon has a wrong alignment
> > 
> > Just to confirm, in case of ellipsis user won't have ways to see full
> > subject if it's too long, is that OK?
> 
> How's specified in the interaction specs?

I definitely can check, but can you point me to the place where I can find this spec?

Comment 56

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429231 [details]
Sticky date header + subheader (carrier header), default state

Oleg, this case with numbered date and only a number as phone information creates a really odd situation in the subheader area. 

Please follow the specs I have just uploaded indicating a divider line, opacity, color and alignment for the elements in the header:

https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8429571

(It's uploaded in messaging metabug)

Comment 57

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429234 [details]
Sticky date header + subheader (carrier header), intermediate state

Please adjust this to new specs. Thanks!

https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8429571

Comment 58

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429236 [details]
Incoming MMS with subject

Please make sure time stamp has a 70% opacity. The rest OK.

Comment 59

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8429395 [details]
Sticky date header (default state)

Please check the specs for alignment, opacity and color: https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8429571

Comment 60

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #55)
> (In reply to Victoria Gerchinhoren [:vicky] from comment #54)
> > (In reply to Oleg Zasypkin [:azasypkin] from comment #52)
> > > (In reply to Victoria Gerchinhoren [:vicky] from comment #51)
> > > > Comment on attachment 8429241 [details]
> > > > Long subject (ellipsis)
> > > > 
> > > > Ellipsis OK. But the "!" icon has a wrong alignment
> > > 
> > > Just to confirm, in case of ellipsis user won't have ways to see full
> > > subject if it's too long, is that OK?
> > 
> > How's specified in the interaction specs?
> 
> I definitely can check, but can you point me to the place where I can find
> this spec?

The interaction spec for MMS subject is at [1] (I try to link the various repositories in [2]).

[1] https://mozilla.app.box.com/s/0u4jt353ei9ov2c150ip/1/1170795225/11918301182/1
[2] https://wiki.mozilla.org/Gaia/SMS#Design_Specs

Comment 61

[Julien Wajsberg [:julienw]]

We can also NI Omega or Jenny to ask for more information here.

Comment 62

[Oleg Zasypkin [:azasypkin]]

(In reply to Victoria Gerchinhoren [:vicky] from comment #56)
> Comment on attachment 8429231 [details]
> Sticky date header + subheader (carrier header), default state
> 
> Oleg, this case with numbered date and only a number as phone information
> creates a really odd situation in the subheader area. 
> 
> Please follow the specs I have just uploaded indicating a divider line,
> opacity, color and alignment for the elements in the header:
> 
> https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8429571
> 
> (It's uploaded in messaging metabug)

Great, thanks!

Comment 63

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #61)
> We can also NI Omega or Jenny to ask for more information here.

Thanks for the link! This spec declares that subject should wrap if it's too long. So if it's the latest spec, patch shouldn't change this behaviour.

Comment 64

[Julien Wajsberg [:julienw]]

Hey Jenny, Omega, can you please give a definitive answer for comment 63?

Comment 65

[Oleg Zasypkin [:azasypkin]]

Hey Victoria, I'd like to confirm several points:

> Please make sure time stamp has a 70% opacity. The rest OK.
Yes its opacity is 70%, color #000 (https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8403322). Or we should use #333 now?

> Can you please check that the subject text is #333333, it looks lighter.
Do we have the same color for subject for incoming and outgoing messages?

> Please adjust this to new specs. Thanks!
> https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8429571
I see that spec introduces new format of carrier header ("phone_type, phone_number carrier") + "carrier" has different text color. Currently we have the following formats:
 1. "phone_type | carrier" - if carrier defined;
 2. "phone_type | phone_number" - if no carrier defined;
 3. "phone_type" - if contact doesn't have name and carrier defined;

So do you want change " | " to ", " in cases #1-#2 and additionally add "phone_number" to the case #1?

Also I've noticed that we have CSS rule that changes text color of the message that is in "sending" state. Do we still need this, I can't find this requirement in previous specs?

Comment 66

[Victoria Gerchinhoren [:vicky]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #65)
> Hey Victoria, I'd like to confirm several points:
> 
> > Please make sure time stamp has a 70% opacity. The rest OK.
> Yes its opacity is 70%, color #000
> (https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8403322). Or we
> should use #333 now?
> 
No, it might be a rendering issue, keep it at #00 as I say and 70% opacity.

> > Can you please check that the subject text is #333333, it looks lighter.
> Do we have the same color for subject for incoming and outgoing messages?
> 
Yes.

> > Please adjust this to new specs. Thanks!
> > https://bug950175.bugzilla.mozilla.org/attachment.cgi?id=8429571
> I see that spec introduces new format of carrier header ("phone_type,
> phone_number carrier") + "carrier" has different text color. Currently we
> have the following formats:
>  1. "phone_type | carrier" - if carrier defined;
>  2. "phone_type | phone_number" - if no carrier defined;
>  3. "phone_type" - if contact doesn't have name and carrier defined;

This was already specd and I am changing the alignment, and color to make it neater. Maybe it is being changed in a different bug, but I can't recall which.

The way information is shown was decided a while ago from a IA POV. So please follow it how it is, I don't have a reason to change it.
> 
> So do you want change " | " to ", " in cases #1-#2 and additionally add
> "phone_number" to the case #1?

In general, we don't use the "|" because of a very old issue with localization. So everything migrated to the "," and I suggest to use the comma in order to keep consistency.
> 
> Also I've noticed that we have CSS rule that changes text color of the
> message that is in "sending" state. Do we still need this, I can't find this
> requirement in previous specs?

I think this is still valid, so better to keep it.

Thanks!

Comment 67

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (styling part)

Hey Steve,

It's a follow-up from unfinished Joan's patch (see https://github.com/mozilla-b2g/gaia/pull/19437 for first review round comments). I've tried to address Julien's comments in a separate commit in my PR so that you can see diff + addressed almost all comments from Victoria (except different carrier text styling part as it depends on bug 1008890), so I think it's ready for review.

Once bug 1008890 is resolved, I'll make necessary changes and ask for the second review round (including ui-review) only for the carrier header part :)

Thanks!

Comment 68

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header + subheader (carrier header), default state

Hey Victoria, here are some updates for sticky header and subheader:
* Height and paddings have been adjusted per spec you provided;
* Color of the carrier name differs from the rest of the header text now( #237171);
* " | " separator is still here, we'll update it in the separate bug.

Comment 69

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header + subheader (carrier header), intermediate state

* Opacity of the header is 95%;
* Opacity of the subheader is 85%.

Comment 70

[Oleg Zasypkin [:azasypkin]]

Attachment: Sticky date header (default state)

* Default state of sticky header without without carrier header.

Comment 71

[Julien Wajsberg [:julienw]]

See bug 883911 for the "|" issue, and bug 925404 for always showing the phone number even when the carrier is known.

Comment 72

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8432321 [details]
Sticky date header + subheader (carrier header), default state

The "|" separator should eventually be changed for a comma. 

Thanks!

Comment 74

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8429241 [details]
Long subject (ellipsis)

As discussed with Omega and Jenny offline we'll keep current behaviour for the long subject case: wrap it into two lines if needed + reduce line-height as Victoria suggested.

Comment 75

[Oleg Zasypkin [:azasypkin]]

Attachment: Long subject (wrap into two lines)

Hey Victoria, I've adjusted line-height and padding for the MMS subject. Does it look better now?

Comment 76

[Steve Chung [:steveck]]

Hi Julien, I just have a concern about the sticky position in thread view[1]. Since Vicky agreed having 2 headers(carrier/date) on the top, does this really affects performance if we simply apply sticky position here?

[1]https://github.com/mozilla-b2g/gaia/pull/19671/files#discussion_r13380890

Comment 77

[Julien Wajsberg [:julienw]]

About "position: sticky", I discussed about this with Vivien. Performance issues if you have a lot (read: more than 3 or 4) of sticky positioned _visible_ elements. As long as they're not visible, they don't impair performance.

In the inbox, it is very likely to have several of them, that's why the sticky header library is still useful. In the thread view, it's a lot less likely, because messages are likely several lines high.

So I'd say it's fine to use "position: sticky" for the headers in this view, unless we find issues later.

Hope this is clear enough !

Comment 78

[Oleg Zasypkin [:azasypkin]]

Hey Steve, thanks for your suggestions on the PR! I've updated it and made Travis green :)

Comment 79

[Steve Chung [:steveck]]

Comment on attachment 8430975 [details] [review]
GitHub pull request URL (styling part)

Thanks for the great work! r=me and let's focus on the carrier header next.

Comment 80

[Steve Chung [:steveck]]

in master: 874df997cb6f527fe96cc78e26e339f26fd2c7d8

Comment 82

[Loli (:lolimartinezcr)]

Tested and working:
Hamachi
2.0
Gecko-7a31c32
Gaia-1bd6e89