Closed Bug 978067 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S3 (14mar)

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

(Keywords: perf, Whiteboard: [c=handeye p= s=2014.03.14 u=])

Attachments

(1 file, 2 obsolete files)

Attached patch call_log.no.sticky.patch (obsolete) — Splinter Review
Basically same story than bug 977680.
Attachment #8383627 - Flags: ui-review?(aymanmaat)
Attachment #8383627 - Flags: review?(anthony)
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.
Attachment #8383627 - Flags: ui-review?(cawang)
Attachment #8383627 - Flags: ui-review?(aymanmaat)
Attachment #8383627 - Flags: review?(anthony)
Attachment #8383627 - Flags: review+
Keywords: perf
Whiteboard: [c=handeye p= s= u=]
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!
Flags: needinfo?(anthony)
Flags: needinfo?(21)
(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.
Flags: needinfo?(21)
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.
Flags: needinfo?(anthony)
Attachment #8383627 - Flags: ui-review?(cawang) → ui-review+
Attached patch bug978067.patch (obsolete) — Splinter Review
Finally let's keep it but replace it with something cheap that only does repaints instead of reflows + repaints.
Attachment #8390414 - Flags: review?(anthony)
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.
Attachment #8390414 - Flags: review?(anthony) → feedback+
Attached patch bug978067.patchSplinter Review
Attachment #8383627 - Attachment is obsolete: true
Attachment #8390414 - Attachment is obsolete: true
Attachment #8390657 - Flags: review?(anthony)
Attachment #8390657 - Flags: review?(anthony) → review+
https://github.com/mozilla-b2g/gaia/commit/70471c959c6583c8dc275f3c32fea45164ab19a8
Assignee: nobody → 21
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s=2014.03.14 u=]
Target Milestone: --- → 1.4 S3 (14mar)
You need to log in before you can comment on or make changes to this bug.