942460 - [Contacts] Implement position:sticky for fixed headers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

In order to improve our scroll performance we should pursue replacing our FixedHeader javascript code with the position:sticky CSS attribute.

Currently position:sticky is disabled behind the layout.css.sticky.enabled preference.  This seems to be due to both some crashes and uncertainty about a specification for the new attribute value.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Proof-of-concept

This is an incomplete proof-of-concept that we can use to investigate and profile any performance benefits.

Comment 2

[Kevin Grandon :kgrandon]

Attachment: Pull request - remove fixed headers

I had this patch sitting around for a long time, so placing it here in case we want to use it.

Comment 3

[Kevin Grandon :kgrandon]

Vivien - I think you may have been interested in this. One of the two patches here may help you test stuff :)

Comment 4

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

(In reply to Kevin Grandon :kgrandon from comment #3)
> Vivien - I think you may have been interested in this. One of the two
> patches here may help you test stuff :)

It seems to perform well with APZ. Nice.

Comment 5

[Kevin Grandon :kgrandon]

Already blocks 943849, so cleaning up the dependency chain.

Comment 6

[Kevin Grandon :kgrandon]

Attachment: Pull request - Use position:sticky for fixed headers

Updated the pull request to port the import list headers over to position: sticky as well.

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8337275 [details] [review]
Proof-of-concept

Obsoleting PoC from Ben.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

Ben - Wondering if you would like to use your contact peer powers to review this one. Let me know if you don't have the cycles or would like another peer to review it.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I can review, but why did you drop the dependency on bug 916315?  There are a bunch of crasher sub-dependencies there we probably need to address before moving forward.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

To be more clear, at this point I would r- the patch because it appears position:sticky is not ready for prime time.

Comment 11

[Kevin Grandon :kgrandon]

(In reply to Ben Kelly [:bkelly] from comment #9)
> I can review, but why did you drop the dependency on bug 916315?  There are
> a bunch of crasher sub-dependencies there we probably need to address before
> moving forward.

We've added a dependency on bug 945777 for gaia. We may end up enabling it in gaia earlier than in release Firefox, and potentially only doing so for certified apps.

(In reply to Ben Kelly [:bkelly] from comment #10)
> To be more clear, at this point I would r- the patch because it appears
> position:sticky is not ready for prime time.

We want to review these patches now, but not land them until we turn on position: sticky in gaia. As soon as it is enabled, we want to be able to quickly land these patches.

Most crashes and issues are being resolved in parallel, and after looking at them, they are not things that certified apps will run into.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I guess I'm a little uncomfortable giving r+ and having the patch sit while master changes underneath it.  How about I give feedback now and I promise to give a quick review when we are closer to landing?

Also, I just put in the overflow:hidden to improve our scroll performance as it greatly reduced the reflows due to modifying the DOM as we scroll.  I'm worried about regressing there.

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

(In reply to Ben Kelly [:bkelly] from comment #12)
> Hmm, I guess I'm a little uncomfortable giving r+ and having the patch sit
> while master changes underneath it.  How about I give feedback now and I
> promise to give a quick review when we are closer to landing?

I will try to clarify timelines, but the goal was to have these patches reviewed and ready to go. I am fine with feedback for now though, so that will work. Thanks!

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I'll review this tomorrow.  I tried it on the phone, though, and it appears to work.  I saw a visual defect (bug 946516), but it looks like that is in master.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

As I said, I plan to review this today, but it would also be good to get feedback from Francisco and Jose Manuel.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

The code looks good, but I think there are a couple visual and performance issues to solve before landing this.

1) When using the jump letters on the side of the list to scroll up you will see two headers at the top of the screen.  Scroll to the bottom and then run your finger up the jump bar on the right to see this effect.

2) We are reflowing a lot more when scrolling the list with this patch.  I believe this is probably due to removing the overflow:hidden attribute.

Profile before patch:

http://people.mozilla.org/~bgirard/cleopatra/#report=1031859c979d1053d283749879aaf5c2b2e6a8d7

Profile after patch:

http://people.mozilla.org/~bgirard/cleopatra/#report=b3c711c06147653e9197426396d1f684d5469c58

Search for Flush or Flush_Int in these profiles.  You will see that there are more reflows and they are longer now.  I think this is going to drop our fps values.

Finally, it seems there have been some style changes recently in master.  Can you rebase this to the most recent version?

Overall I agree this is where we want to go in the future, I just want to make sure we don't regress getting there.  Thanks!

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Upping the points here since solving the perf issue here may be more involved.

Comment 18

[Kevin Grandon :kgrandon]

Thanks for the in-depth work here Ben. We've received R+ for the other apps, but I will run the same profiling tests there as well to make sure we don't regress.

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

Hi Ben,

Thanks for taking a look. I've rebased the Pull Request and have ran profiles for the branch and master. Here are the profiles now:

Master branch: http://people.mozilla.org/~bgirard/cleopatra/#report=3cc8b031b848ba8dd78aa9f2db5ff3148f68c43e

Position Sticky Branch: http://people.mozilla.org/~bgirard/cleopatra/#report=0ee70bae23bc865fce28824ed7d934d4fcd2fa10

I'm seeing that the two profiles are fairly similar now. I'm thinking this could possibly be related to the new will-animate property which has landed in contacts (not sure if you were testing that, but our profiles look vastly different).

Can you take a look at these profiles to see if you can spot any obvious differences? Thanks!

Comment 20

[Jose Manuel Cantera]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

From a functional perspective I'm ok with the patch, but leave final decision on landing to Ben as he is the expert on perf issues and I wouldn't like to regress.

Nonetheless, it is great to see Gecko supporting position: sticky as it saves many JS lines to maintain!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Kevin, can you tell me more about how you took the profile?

Here is what I did for the profiles in comment 16:

1) Install reference-workload-medium
2) Open contacts and wait for images to show signifying workload is fully loaded.
3) Start profiling using this command:

./profile.sh start -p b2g -f js,stackwalk && ./profile.sh start -p Communications -f js,stackwalk -e 500000

4) Scroll down 3 or 4 flicks to about the J group.
5) ./profile.sh capture

I think perhaps you are missing most of the scroll in your profile because you don't have the -e 500000 in your profile start command.

Comment 22

[Kevin Grandon :kgrandon]

That's true, I don't have the -e argument, but the rest of my test is mostly the same. I'll change up the arguments and try capturing the profile again. Thanks!

Comment 23

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

Codewise looking good, thanks Kevin and sorry for the delay!

Comment 24

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

Kevin can you rebased an asked for r? I'm going to push for sticky in 1.4.

Comment 25

[Kevin Grandon :kgrandon]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #24)
> Kevin can you rebased an asked for r? I'm going to push for sticky in 1.4.

Originally I thought that for this one to land, we would need to solve bug 915302. This was due to the overflow: hidden not working with position: sticky. This may no longer be the case with APZ, and we will likely need to profile to determine this. I'll set aside some time to profile with APZ this week.

Comment 26

[Kevin Grandon :kgrandon]

I'm currently looking at some of the other issues in comment 16, but for the time being here are profiles with and without position sticky.

Position Sticky: http://people.mozilla.org/~bgirard/cleopatra/#report=71960e538be1d03709203f4966f0542c547cd44b
Master: http://people.mozilla.org/~bgirard/cleopatra/#report=6a544a71d5ad2d48b38d04dfbe05b98163054604

Comment 27

[Kevin Grandon :kgrandon]

Ok, here's a few new profiles. I wanted to compare with and without overflow: hidden against position: sticky to see how they would do with APZ. Some interesting things in the profiles. 

When filtering by `flush`:
Without overflow, and position sticky, the most time is spent in: nsBlockFrame::FrameStartsCounterScope(nsIFrame*)
With overflow hidden, this changes to be: nsCSSOffsetState::InitOffsets(int, int, nsIAtom*, nsMargin const*, nsMargin const*)

When not filtering, the "Most time spent in" is the same across all three profiles. I'm going to dig into these profiles a bit more in the next few days to see if I can notice any differences. For on device testing, we've been seeing better performance with position: sticky than with FixedHeader.js - I think this is due to slow scroll updates from APZ though.

Master No Overflow: http://people.mozilla.org/~bgirard/cleopatra/#report=010ed31d2795983020db0247330b2d64c909d3aa
Master /w Overflow: http://people.mozilla.org/~bgirard/cleopatra/#report=a268f86436a3135c8e71ad744863cb7091407d3c
Position Sticky: http://people.mozilla.org/~bgirard/cleopatra/#report=d7cfe211d2e9246e001eb779f2b06c210f4837ef

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

Ben - Comment 27 has some new profiles for this. At this point I'm not really sure if we have an option here as APZ is now turned on, and there are definitely scrolling problems with FixedHeaders now. (You see them jump occasionally on a hamachi). I've tried looking into the headers and testing this manually, and I could not really detect a regression here.

Please let me know what you think when you have a chance to look at this.

Comment 29

[Kevin Grandon :kgrandon]

Adding a dependency on bug 945472 as this will solve the travis issue. (I didn't want two commits in here, and will not land until the tree is green). Sorry about the confusion!

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

A few minor nits in the pull request, but otherwise the code itself looks good.

Unfortunately, though, there are a few visual defects:

1) The header underline extends too far to the left.
2) The row highlight wraps around the header when you select a contact.

I'll post some screenshots with these.

Because of the visual defects I need to r-.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Before position:sticky patch

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: After position:sticky patch (visual defects)

Comment 33

[Kevin Grandon :kgrandon]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

Hi Ben,

It turns out we were using building blocks incorrectly. I've fixed the styling so everything should be well now. I updated the PR and made the following changes:

1 - Fixed underline so it matches each item's underline as in master.
2 - Fixed highlight to only highlight until the underline does (as in building blocks).

Please review when you have a chance, thanks!

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I think Jose specifically modified the highlight to go all the way across previously.  Jose, do you recall?

Comment 35

[Kevin Grandon :kgrandon]

Seems incorrect to me, but we can check with some of the visual designers: http://buildingfirefoxos.com/building-blocks/lists.html

Comment 36

[Jose Manuel Cantera]

I would recommend nothing is landed before Arnau, our CSS expert, reviews this. Asking a review to Arnau

thanks

Comment 37

[Benoit Girard (:BenWa)]

(In reply to Kevin Grandon :kgrandon from comment #27)
> Master No Overflow:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=010ed31d2795983020db0247330b2d64c909d3aa
> Master /w Overflow:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=a268f86436a3135c8e71ad744863cb7091407d3c
> Position Sticky:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=d7cfe211d2e9246e001eb779f2b06c210f4837ef

I think you meant to profile the compositor thread here. Keep the same command for the App you're profiling but watch the b2g compositor using:
./profile.ps start -p b2g -t Compositor.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

r=me assuming Arnau signs off on the list selection highlight.

Right now it appears that FixedHeader does a better job of keeping the header at the top of the page, but overall I know we will have better performance with position:sticky.  We should land this and do further work on APZ/position:sticky to address any issues with the header not sticking to the top in a timely manner.

Comment 39

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Sorry I've been busy with the visual refresh, I'll try to check that ASAP.

Comment 40

[Mike Lee [:mlee]]

1.3+, blocks existing blocker Bug 967277.

Kevin, please update the whiteboard ETA field.

Comment 41

[Chris Lee [:clee]]

Looks r+'ed.  What else are we waiting for here?

Comment 42

[Kevin Grandon :kgrandon]

(In reply to Chris Lee [:clee] from comment #41)
> Looks r+'ed.  What else are we waiting for here?

We are just waiting on a R? from Arnau here I think.

Comment 43

[Kevin Grandon :kgrandon]

(Specifically for CSS/style changes)

Comment 44

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8341369 [details] [review]
Pull request - Use position:sticky for fixed headers

Sorry again for the delay. Works awesome!
There's one pixel where you can see the content, between the sticky header and the main header.
But don't worry I could fix this in my next patch for contacts.
Thanks Kevin, nice work.

Comment 45

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

https://github.com/mozilla-b2g/gaia/commit/b8e6475fc45395580dd17e8b5d44c65f1b6f6045

Comment 46

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 b8e6475fc45395580dd17e8b5d44c65f1b6f6045
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 47

[Kevin Grandon :kgrandon]

Landed in v1.3: https://github.com/mozilla-b2g/gaia/commit/fb0a3ee1b364aaf23637a72c32947b64b0df4889