945481 - [Messages] Implement position:sticky for fixed headers

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

We should migrate our Fixed Header javascript to the new position: sticky CSS position.

This is also necessary for smooth headers when APZ is turned on.

Comment 1

[Kevin Grandon :kgrandon]

Already blocks 943849, so cleaning up the dependency chain.

Comment 2

[Kevin Grandon :kgrandon]

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

Comment 3

[Rick Waldron [:rwaldron]]

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

I'd like to get this in ASAP.

Comment 4

[Kevin Grandon :kgrandon]

(In reply to Rick Waldron [:rwaldron] from comment #3)
> Comment on attachment 8341345 [details] [review]
> Pull request - Use position:sticky for fixed headers
> 
> I'd like to get this in ASAP.

Us too :) Unfortunately there's a crazy dependency map of getting APZ working properly and position sticky turned on. We need to leave this un-landed until we handle all of this, but thank you very much for the review!

Comment 5

[Julien Wajsberg [:julienw]]

Is there a bug about enabling position:sticky for B2G? We should make this bug depends on it.

Comment 6

[Julien Wajsberg [:julienw]]

Just tried this again, and this is still not ready for prime time.

Also, the headers in the threads are also made sticky, I don't know if this is due to the patch or to a gecko issue. So I'm removing the review flag for now.

Comment 7

[Kevin Grandon :kgrandon]

Hi Julien. Thanks for taking a look here. I applied the patch again today, and it looks like the behavior has changed since it was originally written. My guess is that this is due to bitrot. Did you apply the patch on top of master, or take only the app from my branch?

In any case I will rebase, get this in working shape again, and ask for another review.

Comment 8

[Julien Wajsberg [:julienw]]

Yeah, I unbitrotted it when I tried it on master. But what I really meant is that the whole position:sticky feature is not ready: the scroll feels more laggy than without the patch.

Note that I tried on a Peak so it might be different on other devices.

Comment 9

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

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Yeah, I unbitrotted it when I tried it on master. But what I really meant is
> that the whole position:sticky feature is not ready: the scroll feels more
> laggy than without the patch.
> 

I would like to have position: sticky in 1.4. Any lagginess can be fixed by the APZC team.

Comment 10

[Kevin Grandon :kgrandon]

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

Hi Julien - this has now been rebased and un-bitrotted. If you have time to give this a quick review it would be helpful. Please note, you will need to enable the layout.css.sticky.enabled preference first. You could either apply the patch of the blocked bug, or with build/config/custom-prefs.js:

user_pref("layout.css.sticky.enabled", true);

Comment 11

[Julien Wajsberg [:julienw]]

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

I find no issue with this patch, except small graphics issues we'll need to file separately when we'll have good STR.

r=me after it's unbitrotted and with a nit.

Of course we need bug 945777 first, so if it takes too long and you need to unbitrot too much, we might need another review before merging.

Comment 12

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/9d135e867cc9cb22adbe41f481de3aa9657e11a2

Comment 13

[Mike Lee [:mlee]]

1.3+, blocked existing 1.3 blocker Bug 943849.

Comment 14

[Julien Wajsberg [:julienw]]

Are we sure all the fixes for position:sticky landed in 1.3?

Comment 15

[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 9d135e867cc9cb22adbe41f481de3aa9657e11a2
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 16

[Mike Lee [:mlee]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Are we sure all the fixes for position:sticky landed in 1.3?

Kevin please confirm.

Comment 17

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Are we sure all the fixes for position:sticky landed in 1.3?

It should work, but I was hoping to have some of the issues in bug 916315 landed for 1.4. I am a bit concerned about this being available to all third party apps in 1.3 with it not being super polished.

Comment 18

[Kevin Grandon :kgrandon]

Attachment: Pull Request - to 1.3 branch

Comment 19

[Julien Wajsberg [:julienw]]

I'm also quite concerned about this. I think the risk outweights greatly the benefit here.

Comment 20

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> I'm also quite concerned about this. I think the risk outweights greatly the
> benefit here.

Yeah... apparently they all got flipped to 1.3 due to APZ. Worst case scenario - it's not hard to back these out if needed.

Comment 21

[Kevin Grandon :kgrandon]

Landed in 1.3: https://github.com/mozilla-b2g/gaia/commit/a094ecdcb71bb8fc73afceeb32001fdc616d6945

Comment 22

[Rick Waldron [:rwaldron]]

Julien, Kevin— I can't find the r+ for this

Comment 23

[Julien Wajsberg [:julienw]]

Rick, what do you mean? I can see the r+ on the attachment...

Comment 24

[Rick Waldron [:rwaldron]]

I have no explanation for why I completely missed https://bugzilla.mozilla.org/show_bug.cgi?id=945481#c11 accept that I was reviewing from my phone maybe I scrolled past it? Sorry for the noise!