978067 - [Call Log] Remove position: sticky as it hurts scrolling performance

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: call_log.no.sticky.patch

Basically same story than bug 977680.

Comment 1

[Anthony Ricaud (:rik)]

Comment on attachment 8383627 [details] [diff] [review]
call_log.no.sticky.patch

r+ if UX is ok to drop the sticky headers.

Redirecting to Carrie since she is our UX designer for Dialer.

Comment 2

[Carrie Wang [New account: [e-mail]]]

Hey guys, 

I need more information on this. Why should we remove the sticky headers? I think it's quite a pleasant feature! If it's performance issues, are we going to remove the effect in every APP or only Message and Dialer? 
I've check the patch. It looks good to me (but personally I prefer the one with the sticky effect). Thanks!

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Carrie Wang [:carrie] from comment #2)
> Hey guys, 
> 
> I need more information on this. Why should we remove the sticky headers? I
> think it's quite a pleasant feature! If it's performance issues, are we
> going to remove the effect in every APP or only Message and Dialer? 
> I've check the patch. It looks good to me (but personally I prefer the one
> with the sticky effect). Thanks!

position: sticky is used for Messages, Call Log and Contacts. I'm trying to get rid of it for SMS and the Call Log, but keep it for Contacts.

Comment 4

[Anthony Ricaud (:rik)]

position: sticky is nice visually but really slows down scrolling performance. For call log and sms, the date headers being sticky do not provide a lot of value since you don't have that many calls/sms in a day.

We're doing an engineering trade-off here. The platform can't give us good performance (yet) so we're modifying the UX because responsiveness is more important than anything else.

Comment 5

[Carrie Wang [New account: [e-mail]]]

OK. Got it. Thanks!

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug978067.patch

Finally let's keep it but replace it with something cheap that only does repaints instead of reflows + repaints.

Comment 7

[Anthony Ricaud (:rik)]

Comment on attachment 8390414 [details] [diff] [review]
bug978067.patch

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

Very nice!
not r+ because of the mock issue.

::: apps/communications/dialer/index.html
@@ +55,5 @@
>      <script defer src="/shared/js/fb/fb_reader_utils.js"></script>
>      <script defer src="/shared/js/fb/fb_tel_index.js"></script>
>      <script defer src="/shared/js/binary_search.js"></script>
> +    <script defer src="/shared/js/sticky.js"></script>
> +    <script defer src="/dialer/js/newsletter_manager.js"></script>

Looks like it is twice in here.

::: apps/communications/dialer/test/unit/call_log_test.js
@@ +13,4 @@
>  
>  requireApp('communications/shared/test/unit/mocks/mock_notification.js');
>  
> +require('/shared/js/sticky.js');

You need to create a mock here.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug978067.patch

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/70471c959c6583c8dc275f3c32fea45164ab19a8