1014178 - [Visual Refresh][Messages] Thread messages view contains last-child selectors and is slightly misaligned

Status: RESOLVED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

Attachment: Screenshot of bottom bar overlapping with messages.

The thread messages view contains a couple of last-child selectors which are slowing down reflowing and repainting when the messages list is altered. In addition, since the visual refresh of the edit view landed, there seems to be a regression in the visuals. The bottom menu bar is overlapping with the messages container now. See attached screenshot.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Remove last-child selectors and fix message container alignment.

I did some benchmarks with and without this patch.

  w: http://people.mozilla.org/~bgirard/cleopatra/#report=643765e0ce2fb5414da181d29a24602245a3aa2a
  w/o: http://people.mozilla.org/~bgirard/cleopatra/#report=3599b74aa79b0b1fb66e5963cc833783d2ac7416

Disregard the total time taken as I was deleting from different threads. The important part is the tail, in particular:

  w: 215000ms - 216000ms
  w/o: 30500ms - 31400ms

As Julien predicted in bug 1001467, removing the last-child selectors seems to have drastically reduced the amount of activity in the messages app after the initial deletion is completed. Unfortunately, this seems to have almost no perceptible difference to the user from my testing.

This patch also fixes the alignment issues that I described. Not requesting review yet since I want to get UI review first.

PR: https://github.com/mozilla-b2g/gaia/pull/19496

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Screenshot, with patch applied, of the messages view mode.

Vicky, please compare this with attachment 8394780 [details]. I notice in that screenshot that there's a big gap between the composer and last message, but I'm not sure if that was intentional.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Screenshot, with patch applied, of the messages edit mode.

Vicky, please compare this with attachment 8370691 [details]. I notice in that screenshot that there's a small gap between the bottom menu bar and the last message, but I'm not sure if that was intentional.

Comment 4

[Julien Wajsberg [:julienw]]

Actually, I filed bug 1009098 that has the same cause IMO, can you check if your patch fixes this as well?

Comment 5

[Doug Sherk (:drs) (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Actually, I filed bug 1009098 that has the same cause IMO, can you check if
> your patch fixes this as well?

I just tried the STR for that. I was able to repro, and this patch doesn't fix that issue. But without digging further, I think it's caused by a separate problem. We should just be hiding the composer input field in edit mode. This is a padding/margin issue, including the last-child selector (in which case I moved its props to other elements).

Comment 6

[Julien Wajsberg [:julienw]]

Ok, thanks for checking.

Comment 7

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8426483 [details]
Screenshot, with patch applied, of the messages edit mode.

Edit mode OK. The time stamp shouldn't be italic though, but I guess this is part of a different bug.

Comment 8

[Victoria Gerchinhoren [:vicky]]

(In reply to Doug Sherk (:drs) from comment #2)
> Created attachment 8426482 [details]
> Screenshot, with patch applied, of the messages view mode.
> 
> Vicky, please compare this with attachment 8394780 [details]. I notice in
> that screenshot that there's a big gap between the composer and last
> message, but I'm not sure if that was intentional.

Hi Doug,

The gap is not that big, it's just 2 rem counting the total height of the input area. I think it's a safe distance to avoid the user tapping the wrong object (ex. the download link instead of the input area) why do you think it would be a problem?

Thanks.

Comment 9

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8426482 [details]
Screenshot, with patch applied, of the messages view mode.

What should I look at here? I think it is ok besides the color bubbles and the time stamp being italic.

Comment 10

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8426479 [details] [diff] [review]
Remove last-child selectors and fix message container alignment.

(In reply to Victoria Gerchinhoren [:vicky] from comment #8)
> The gap is not that big, it's just 2 rem counting the total height of the
> input area. I think it's a safe distance to avoid the user tapping the wrong
> object (ex. the download link instead of the input area) why do you think it
> would be a problem?

When I tried it on a real device, it felt like a very big gap to me. I understand your reasoning, but I think the margin that I left in my screenshot is reasonable and more consistent with edit mode. I'm not sure what to do now because you gave ui-review+ but your comment seems to indicate that you're not happy with it. :) So I'll ni? you again.

We can also iron this out in the future during the rest of the visual refresh. The main purpose of this patch is to get rid of the last-child selector. I just wanted to fix an obvious visual problem that I saw while I was at it.

Asking for review from Julien since it got ui-review+.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8426479 [details] [diff] [review]
Remove last-child selectors and fix message container alignment.

Redirect review to Steve because he will work on bug 1009098 and this is somewhat related.

Comment 12

[Victoria Gerchinhoren [:vicky]]

(In reply to Doug Sherk (:drs) from comment #10)
> Comment on attachment 8426479 [details] [diff] [review]
> Remove last-child selectors and fix message container alignment.
> 
> (In reply to Victoria Gerchinhoren [:vicky] from comment #8)
> > The gap is not that big, it's just 2 rem counting the total height of the
> > input area. I think it's a safe distance to avoid the user tapping the wrong
> > object (ex. the download link instead of the input area) why do you think it
> > would be a problem?
> 
> When I tried it on a real device, it felt like a very big gap to me. I
> understand your reasoning, but I think the margin that I left in my
> screenshot is reasonable and more consistent with edit mode. I'm not sure
> what to do now because you gave ui-review+ but your comment seems to
> indicate that you're not happy with it. :) So I'll ni? you again.
> 
> We can also iron this out in the future during the rest of the visual
> refresh. The main purpose of this patch is to get rid of the last-child
> selector. I just wanted to fix an obvious visual problem that I saw while I
> was at it.
> 
> Asking for review from Julien since it got ui-review+.

As per my comment, please put the exact reason why you're asking the ui review so I do know where to look at. The spacing at the bottom is not a problem so my UI review is +. The title of the bug is not related with the issue you are arising. Thanks!

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8426479 [details] [diff] [review]
Remove last-child selectors and fix message container alignment.

Review of attachment 8426479 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry that I turn down the review because of a regression in the patch. Please set again once you fix it, thanks!

::: apps/sms/style/sms.css
@@ +665,5 @@
>  
>  /* Add a margin at the end of the last thread or message node to avoid overlay
>   * with the edit mode menu */
> +.edit .edit-container {
> +  padding-bottom: 7rem;

This change seems not working and you can see the thread is covered under the banner in thread list view. I didn't investigat it yet but maybe the different structure of view container leads different result. Please revert this change or adjust the height for view-body with edit container class if you want to get rid of :last-child pseudo class.

Comment 14

[Julien Wajsberg [:julienw]]

Things to remember when testing here: test with very small thread (1 message), with small thread (2 or 3 messages, doesn't scroll, but next message would scroll) and a thread that scrolls, and check you can see all messages completely (especially the first and last messages).

Comment 15

[Doug Sherk (:drs) (inactive)]

Attachment: Remove last-child selectors and fix message container alignment.

(In reply to Steve Chung [:steveck] from comment #13)
> adjust the height for view-body with edit container class if
> you want to get rid of :last-child pseudo class.

Thanks, I chose to use this method.

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Things to remember when testing here: test with very small thread (1
> message), with small thread (2 or 3 messages, doesn't scroll, but next
> message would scroll) and a thread that scrolls, and check you can see all
> messages completely (especially the first and last messages).

I just tested the attached patch against all of these cases and it seems to work fine for me.

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8429394 [details] [diff] [review]
Remove last-child selectors and fix message container alignment.

Review of attachment 8429394 [details] [diff] [review]:
-----------------------------------------------------------------

This patch works fine and I just left some nits, thanks!

::: apps/sms/style/sms.css
@@ +110,5 @@
>    background-repeat: no-repeat;
>    background-position: 1.5rem 0;
>  }
>  
> +.edit #threads-container[data-type="list"] {

nit: You can move this rule set under L287 /* Styles for Edit mode in Thread List */ block

@@ +668,5 @@
>  }
>  
>  /* Add a margin at the end of the last thread or message node to avoid overlay
>   * with the edit mode menu */
> +.edit .edit-container {

Since neither list view nor message view will have any impact with this rule applied in edit mode, maybe we should just remove it.

Comment 17

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

triage: Not blocking.

Comment 18

[Steve Chung [:steveck]]

Hey Doug, since bug 1009098 might also effect this bug, do you mind adjust the patch if bug 1009098 land first? Sorry for the inconvenience...

Comment 19

[Doug Sherk (:drs) (inactive)]

Attachment: Remove last-child selectors and fix message container alignment.

(In reply to Steve Chung [:steveck] from comment #18)
> Hey Doug, since bug 1009098 might also effect this bug, do you mind adjust
> the patch if bug 1009098 land first? Sorry for the inconvenience...

Yeah, it did. I removed one of the additions I made and it seems fine now from my testing. Carrying r+.

Comment 20

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/33d9a9b24d47cd87b00c643f97a6ceab50a669c5