Closed Bug 945481 Opened 8 years ago Closed 7 years ago

[Messages] Implement position:sticky for fixed headers

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

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

Attachments

(2 files)

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.
Already blocks 943849, so cleaning up the dependency chain.
No longer depends on: 916315
Component: Gaia::Dialer → Gaia::SMS
Summary: [Sms] Implement position:sticky for fixed headers → [Messages] Implement position:sticky for fixed headers
Comment on attachment 8341345 [details] [review]
Pull request - Use position:sticky for fixed headers

I'd like to get this in ASAP.
Attachment #8341345 - Flags: review+
(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!
Is there a bug about enabling position:sticky for B2G? We should make this bug depends on it.
Depends on: 945777
Whiteboard: [c=handeye p2= s= u=] → [c=handeye p=2 s= u=]
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.
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.
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.
(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 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);
Attachment #8341345 - Flags: review?(felash)
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.
Attachment #8341345 - Flags: review?(felash) → review+
Landed: https://github.com/mozilla-b2g/gaia/commit/9d135e867cc9cb22adbe41f481de3aa9657e11a2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
1.3+, blocked existing 1.3 blocker Bug 943849.
blocking-b2g: --- → 1.3+
Whiteboard: [c=handeye p=2 s= u=] → [c=handeye p=2 s= u=1.3]
Target Milestone: --- → 1.4 S1 (14feb)
Priority: -- → P1
Are we sure all the fixes for position:sticky landed in 1.3?
Flags: needinfo?(mlee)
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
Flags: needinfo?(kgrandon)
(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.
Flags: needinfo?(mlee)
(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.
Flags: needinfo?(kgrandon)
I'm also quite concerned about this. I think the risk outweights greatly the benefit here.
(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.
Julien, Kevin— I can't find the r+ for this
Rick, what do you mean? I can see the r+ on the attachment...
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!
You need to log in before you can comment on or make changes to this bug.