Closed Bug 1153841 Opened 5 years ago Closed 5 years ago

Navigation bar on ESPN jumps around while panning

Categories

(Firefox for Android :: Toolbar, defect, P1)

Other
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed
fennec 41+ ---

People

(Reporter: blassey, Assigned: kats)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

If it's fixed position, we need to fix this. If not, we should reach out
tracking-fennec: --- → ?
Mike do you have a bug for this other than the github issue? Do you want to close it/dup it?
Flags: needinfo?(miket)
We can dupe either this one or the other--I don't have a preference. By the sound of webcompat@836, their engineers are at least aware of our suggested workaround.

On one hand, position:sticky fixes the issue. On the other hand, Chrome Mobile performs wayyyy better than us with just position:fixed. Your call. :)
So this is not a web compat bug just because they can work around it.

Kevin, can you check quickly if this has regressed in the last 4 releases?
Flags: needinfo?(kbrosnan)
From user feedback on ESPN:
* 37+    - we have reports that the menu bar is jittery.  It manifests worse on some pieces of hardware (cwang's N5 was far worse than my M8 or glind's cheap Samsung)
* 36     - we have a single report of "scrolling issues"
* pre-36 - we have reports that the menu is completely unresponsive and/or renders strangely.  Users are often not able to scroll at all.  Some users report that the page has a ton of excess scrollable space in the right margin.

This could either be correlated to dates or versions, I don't have enough data to make a strong claim.
Here's a link to some of the ESPN data on Input: https://input.mozilla.org/en-US/?q=espn+%2B+%28scroll+bar+menu%29&date_start=2014-01-01&product=Firefox%20for%20Android
Assignee: nobody → bugmail.mozilla
tracking-fennec: ? → +
Priority: -- → P1
Regression in 30.0a1

23:29.97 LOG: MainThread Bisector INFO Narrowed nightly regression window from [2014-02-25, 2014-02-28] (3 days) to [2014-02-27, 2014-02-28] (1 days) (~0 steps left)
23:29.97 LOG: MainThread Bisector INFO Got as far as we can go bisecting nightlies...
23:29.97 LOG: MainThread Bisector INFO Last good revision: a98a1d78817f
23:29.97 LOG: MainThread Bisector INFO First bad revision: 58eca03214a6
23:29.97 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a98a1d78817f&tochange=58eca03214a6
Tried bisecting via hg but I don't seem to have a working 30.0a1 build environment. If it is really needed I can spend some time on this.
Flags: needinfo?(kbrosnan)
kats, is it useful to bisect this further?
Flags: needinfo?(bugmail.mozilla)
Probably not. It's old enough that the code is going to be quite different now.
Flags: needinfo?(bugmail.mozilla)
I took a quick look at this; there is a 3D transform on the fixed-pos item which is causing the problem (because our code in AsyncCompositionManager that deals with keeping fixed-pos items fixed doesn't deal with 3D transforms). The CSS for the page has a perspective:800px on the fixed-pos item, and if I remove that it works fine.
I don't really see any reason to restrict the handling to 2D transforms, and dealing with 3D transforms as well fixes the issue on this page.
Attachment #8624838 - Flags: review?(matt.woodrow)
Attachment #8624838 - Flags: review?(bgirard)
Comment on attachment 8624838 [details] [diff] [review]
Deal with 3D transforms for fixed-pos layers as well

Review of attachment 8624838 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a good candidate to add a test. I can see us regressing this quite easily. Consider adding a test if-and-only-if it's pragmatic.

Patch looks good but I'm a bit surprised there are not any tricky cases to handle for 3D. This patch just removes the 2d checks essentially.
Attachment #8624838 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #12)
> This looks like a good candidate to add a test. I can see us regressing this
> quite easily. Consider adding a test if-and-only-if it's pragmatic.

So I thought it would be pretty straightforward to write a test for this. I copied layout/reftests/async-scrolling/position-fixed-1*.html to -3*.html and modified it so that the fixed-pos element had a 3D transform. On desktop however, the layout.scroll.root-frame-containers pref is set to false by default and on Fennec it's true (until bug 1158392 lands anyway), and that affects the structure of the layer tree that results. On desktop the fixed-pos layer ends up being a sibling of the async-scrolled layer whereas on Fennec it's a child, and that's relevant to the bug. This also implies that fixing bug 1158392 will fix this issue as well.

Also, the 3D transform that I induced caused assertion failures in layout (which is bad, I filed bug 1176355 for it) and I'm not sure how else to induce a 3D transform on layer. I tried copying the CSS properties from the ESPN page but it didn't work - I must be missing something (and I don't really understand the world of 3D CSS transforms enough to figure it out).

> Patch looks good but I'm a bit surprised there are not any tricky cases to
> handle for 3D. This patch just removes the 2d checks essentially.

I'm sure there are some tricky cases, but again I don't know enough about the world of 3D to really say. Hopefully mattwoodrow can chime in on that.
Attachment #8624838 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/07e102889997
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Is this something we want to uplift?
tracking-fennec: + → ?
tracking-fennec: ? → 41+
You need to log in before you can comment on or make changes to this bug.