945777 - Enable position:sticky in gaia

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

As position:sticky is stabilizing we should consider enabling it for gaia. This will help with overall performance and code cleanliness.

This is also needed for the migration to APZ as scroll events will be fired less often.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Pull request - enable position sticky pref

Comment 2

[Kevin Grandon :kgrandon]

We can review this one, but let's not land until we enable APZ.

Comment 3

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

Comment on attachment 8341786 [details] [review]
Pull request - enable position sticky pref

Don't you want to add it to preferences instead? In all cases it does not hurt since sticky is normally enabled by default on nightly build so do we really need it yet ?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

This will enable position:sticky for all apps and web pages, correct?  Just curious if we want to open it up that wide given that there are still known crashers in position:sticky.  See the deps for bug 916315.

Given that there are stability concerns it would seem prudent to enable this at the beginning of a release cycle right after branching the previous release.  That would give us the maximum amount of burn-in time.

Comment 5

[Kevin Grandon :kgrandon]

(In reply to Ben Kelly [:bkelly] from comment #4)
> This will enable position:sticky for all apps and web pages, correct?  Just
> curious if we want to open it up that wide given that there are still known
> crashers in position:sticky.  See the deps for bug 916315.
> 
> Given that there are stability concerns it would seem prudent to enable this
> at the beginning of a release cycle right after branching the previous
> release.  That would give us the maximum amount of burn-in time.

Yes, the current patch will enable it for all apps/web pages. There was some discussion of limiting it to certified apps only in bug 943849. I would recommend that we enable position sticky whenever we land APZ (bug 909877). 

I believe we've committed to APZ in 1.3 already, but I'm not sure how flexible this list is: https://wiki.mozilla.org/B2G/Roadmap#Committed_1.3_Features

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kevin Grandon :kgrandon from comment #5)
> I believe we've committed to APZ in 1.3 already, but I'm not sure how
> flexible this list is:
> https://wiki.mozilla.org/B2G/Roadmap#Committed_1.3_Features

According to the history it was added to the 1.3 committed list the evening of Nov 20, 2013.  That was 12 business days (10 if you count the US Thanksgiving holiday) before 1.3 code freeze.

I don't know what the state of APZ was when that decision was made, but we're now 3 days from code freeze.  We have open crashers against position:sticky.  The APZ code is still being actively worked to remove bugs (for example bug 945277).  From talking with Vivien our current scroll handler strategies don't appear to work well with APZ (we don't receive onscroll during kinetic panning, etc).  I know position:sticky is meant to deal with that for headers, but we have a ton of code that modifies the DOM via onscroll events.

I know all of these problems can and will be resolved, but it seems like we would have a better shot at a quality release if we rode the trains in 1.4 instead of crash landing in 1.3.

Comment 7

[Kevin Grandon :kgrandon]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #3)
> Comment on attachment 8341786 [details] [review]
> Pull request - enable position sticky pref
> 
> Don't you want to add it to preferences instead? In all cases it does not
> hurt since sticky is normally enabled by default on nightly build so do we
> really need it yet ?

I suppose, I'm not really sure what the benefit of either one is yet still. I assumed that we would need to turn this on before position:sticky is enabled for production. 

I've updated the PR, but will wait to see if we want to turn this on early or not depending on when APZ lands.

Comment 8

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

Comment on attachment 8341786 [details] [review]
Pull request - enable position sticky pref

APZC is going to be turned on for 1.3, and there is no chance that it miss 1.4. I'm going to push for sticky in 1.4, let's land that as it will let us is there is some bugs with sticky.

Comment 9

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/4bfe2353566ed2164a97d900649e60f434b0854a

Comment 10

[Mike Lee [:mlee]]

1.3+, blocked existing 1.3 blocker Bug 943849 via Bug 942460.

Comment 11

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

Uplifted 4bfe2353566ed2164a97d900649e60f434b0854a to:
v1.3: 4d1514ab33665d81112479c92626e4addd41a0ba