Closed Bug 1014178 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(4 files, 2 obsolete files)

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.
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
Assignee: nobody → drs+bugzilla
Status: NEW → ASSIGNED
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.
Attachment #8426482 - Flags: ui-review?(vpg)
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.
Attachment #8426483 - Flags: ui-review?(vpg)
Actually, I filed bug 1009098 that has the same cause IMO, can you check if your patch fixes this as well?
Blocks: 1001467
(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).
Ok, thanks for checking.
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.
Attachment #8426483 - Flags: ui-review?(vpg) → ui-review+
(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 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.
Attachment #8426482 - Flags: ui-review?(vpg) → ui-review+
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+.
Attachment #8426479 - Flags: review?(felash)
Flags: needinfo?(vpg)
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.
Attachment #8426479 - Flags: review?(felash) → review?(schung)
(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!
Flags: needinfo?(vpg)
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.
Attachment #8426479 - Flags: review?(schung)
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).
blocking-b2g: --- → 2.0?
(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.
Attachment #8426479 - Attachment is obsolete: true
Attachment #8429394 - Flags: review?(schung)
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.
Attachment #8429394 - Flags: review?(schung) → review+
triage: Not blocking.
blocking-b2g: 2.0? → backlog
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...
See Also: → 1009098
(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+.
Attachment #8429394 - Attachment is obsolete: true
Attachment #8432355 - Flags: review+
https://github.com/mozilla-b2g/gaia/commit/33d9a9b24d47cd87b00c643f97a6ceab50a669c5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.