Closed Bug 950993 Opened 7 years ago Closed 7 years ago

position:fixed CSS rule fails to keep marketplace header at top of browser

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: nhirata, Assigned: cwiiis)

References

Details

(Whiteboard: [apz_testrun])

Attachments

(1 file)

Description:



Steps to Reproduce:

1. install APZ build and turn APZ on
2. launch market place
3. pan down

Expected Results: header stays at the top
Actual Results: header can move and drag along

Additional Notes:
Similar to bug 811950

Environmental Variables:
Build : 
Gaia      504cf9a988cd84760b4040706ccc41e508a97fd2
Gecko     55b75ce2263e7d2a8cbfdabc7e5206d9caa89f98
BuildID   20131216140918
Version   28.0a2
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013
RIL Type : Mozilla RIL
OS Version: 1.3
Device: Buri
I think the uglyness is more or less expected. Milan can you confirm, that we should fix the marketplace directly and that the current behavior is the behavior for fixed header in the mobile world ? (or is there a regression somewhere) ?
Flags: needinfo?(milan)
I'll let Kats answer.
Flags: needinfo?(milan) → needinfo?(bugmail.mozilla)
I'm hitting bug 950540 when I try to repro so I can't see the current behaviour until that is fixed or has a workaround. Leaving needinfo flag for now.
Does this reproduce with APZC not turned on?
Keywords: qawanted
This bug does not reproduce when the "Enable APZ" option is turned off.

Device: Buri 1.3 MOZ
BuildID: 20131216140918
Gaia: 504cf9a988cd84760b4040706ccc41e508a97fd2
Gecko: 55b75ce2263e7d2a8cbfdabc7e5206d9caa89f98
Version: 28.0a2
Firmware Version: V1.2_20131115
QA Contact: nkhristoforov
Removing qawanted as per comment 5.
Keywords: qawanted
Okay - that implies this isn't a marketplace bug - it's a regression from turning APZC on across the board.
Blocks: gaia-apzc
Component: Gaia → Panning and Zooming
Product: Firefox OS → Core
Is the marketplace app code available somewhere? From the behaviour I'm seeing it doesn't look like position:fixed in CSS, it looks like JS is moving the bar based on scroll events.

If it is done in CSS then it might be related to bug 946502.
Flags: needinfo?(bugmail.mozilla)
But to answer the question, we should be able to have a much better experience on position:fixed elements. In Fennec the behaviour is much better even though we do async panning there. We have code that is supposed to move position:fixed elements in the compositor to keep them in sync with async scrolling.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Is the marketplace app code available somewhere? From the behaviour I'm
> seeing it doesn't look like position:fixed in CSS, it looks like JS is
> moving the bar based on scroll events.
> 
> If it is done in CSS then it might be related to bug 946502.

Putting needinfo on cvan to help address this question.
Flags: needinfo?(cvan)
(In reply to Jason Smith [:jsmith] from comment #10)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> > Is the marketplace app code available somewhere? From the behaviour I'm
> > seeing it doesn't look like position:fixed in CSS, it looks like JS is
> > moving the bar based on scroll events.
> > 
> > If it is done in CSS then it might be related to bug 946502.
> 
> Putting needinfo on cvan to help address this question.

That's incorrect - we're not using JavaScript anywhere to force the position of the header. Our header is `position: fixed`: https://github.com/mozilla/fireplace/blob/master/hearth/media/css/header.styl#L33-L35
Flags: needinfo?(cvan)
Ok, thanks for the link!

Chris, do you know what the current state of fixed-position content is with async scrolling? I recall various regressions recently but not sure if they've all been fixed or if there is anything outstanding beyond bug 946502.
Flags: needinfo?(chrislord.net)
As far as I know they were all fixed, bar background-attachment:fixed, but this is a question better answered by roc.
Flags: needinfo?(chrislord.net) → needinfo?(roc)
Blocks: 950488
Blocking a blocker.
blocking-b2g: 1.3? → 1.3+
It's worth noting for this that this only applies to fixed position content that *isn't* in the browser (where things behave correctly), so I would suspect that this is because in those cases, the 'root' scrollable frame is actually a sub-frame.
As far as I know, the position:fixed regressions were fixed.
Flags: needinfo?(roc)
The fixes were
https://hg.mozilla.org/mozilla-central/rev/040f7055cab9
https://hg.mozilla.org/mozilla-central/rev/576dcc232795
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8523e7bddac
Then again, it's possible, even likely, that APZC for the marketplace app never worked in any Gecko build.

A MOZ_DUMP_PAINT_LIST display item/layer tree dump for the marketplace app ought to help.
Assignee: nobody → bugmail.mozilla
According to https://bugzilla.mozilla.org/show_bug.cgi?id=950488#c8 this is happening because layers aren't getting marked as fixed-position any more? Chris, do you have more details?
Flags: needinfo?(chrislord.net)
Assignee: bugmail.mozilla → bgirard
No longer blocks: 950488
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> The fixes were
> https://hg.mozilla.org/mozilla-central/rev/040f7055cab9
> https://hg.mozilla.org/mozilla-central/rev/576dcc232795
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e8523e7bddac
> Then again, it's possible, even likely, that APZC for the marketplace app
> never worked in any Gecko build.
> 
> A MOZ_DUMP_PAINT_LIST display item/layer tree dump for the marketplace app
> ought to help.

I don't have anything beyond this - I don't see fixed position content working in any non-browser APZC-enabled use-case, it would appear it's broken for all subframe scrolling (as I said in comment #15). Dumping the display-list and layer tree would help to debug this, which I could do on Monday.

I see that this is assigned to BenWa - are you working on this, or is it ok for me to take it?
Flags: needinfo?(chrislord.net) → needinfo?(bgirard)
(In reply to Chris Lord [:cwiiis] from comment #19)
> I see that this is assigned to BenWa - are you working on this, or is it ok
> for me to take it?

I though I fixed my other APZC blocker but I haven't. It would be great if you can take this so I can focus on bug 950488.
Flags: needinfo?(bgirard)
Assignee: bgirard → chrislord.net
Blocks: 950488
Usefully, exporting MOZ_DUMP_PAINT_LIST causes the marketplace to crash on load, before it shows the main screen... Likely OOMing, I'll work on a reduced test-case.
(In reply to Chris Lord [:cwiiis] from comment #21)
> Usefully, exporting MOZ_DUMP_PAINT_LIST causes the marketplace to crash on
> load, before it shows the main screen... Likely OOMing, I'll work on a
> reduced test-case.

It's possible you're running into bug 946879, it happened to me when trying to do display list dumping as well (should have remembered to mention that in daily, sorry). I worked around it by commenting out some code, I can post a patch if you'd like.
(In reply to Botond Ballo [:botond] from comment #22)
> (In reply to Chris Lord [:cwiiis] from comment #21)
> > Usefully, exporting MOZ_DUMP_PAINT_LIST causes the marketplace to crash on
> > load, before it shows the main screen... Likely OOMing, I'll work on a
> > reduced test-case.
> 
> It's possible you're running into bug 946879, it happened to me when trying
> to do display list dumping as well (should have remembered to mention that
> in daily, sorry). I worked around it by commenting out some code, I can post
> a patch if you'd like.

I posted my workaround patches at bug 946879.
(In reply to Botond Ballo [:botond] from comment #23)
> (In reply to Botond Ballo [:botond] from comment #22)
> > (In reply to Chris Lord [:cwiiis] from comment #21)
> > > Usefully, exporting MOZ_DUMP_PAINT_LIST causes the marketplace to crash on
> > > load, before it shows the main screen... Likely OOMing, I'll work on a
> > > reduced test-case.
> > 
> > It's possible you're running into bug 946879, it happened to me when trying
> > to do display list dumping as well (should have remembered to mention that
> > in daily, sorry). I worked around it by commenting out some code, I can post
> > a patch if you'd like.
> 
> I posted my workaround patches at bug 946879.

This is handy, but I'm just dumping the paint list, not painting itself.

I tried to produce a simple test-case, but it seems position:fixed works fine in my test case... So now I need to have a look at what the marketplace is doing exactly.
Oh, ok, the marketplace is just quitting on load, regardless of MOZ_DUMP_PAINT_LIST... I guess I'm also using a debug build now vs. earlier, perhaps that's affecting things... Not useful.
(In reply to Chris Lord [:cwiiis] from comment #24)
> (In reply to Botond Ballo [:botond] from comment #23)
> > (In reply to Botond Ballo [:botond] from comment #22)
> > > (In reply to Chris Lord [:cwiiis] from comment #21)
> > > > Usefully, exporting MOZ_DUMP_PAINT_LIST causes the marketplace to crash on
> > > > load, before it shows the main screen... Likely OOMing, I'll work on a
> > > > reduced test-case.
> > > 
> > > It's possible you're running into bug 946879, it happened to me when trying
> > > to do display list dumping as well (should have remembered to mention that
> > > in daily, sorry). I worked around it by commenting out some code, I can post
> > > a patch if you'd like.
> > 
> > I posted my workaround patches at bug 946879.
> 
> This is handy, but I'm just dumping the paint list, not painting itself.

Ah, I didn't realize there was a difference! I also just wanted to dump the paint list, so a simpler workaround in my case would have been to define MOZ_DUMP_PAINT_LIST instead of MOZ_DUMP_PAINT...

(In reply to Chris Lord [:cwiiis] from comment #25)
> Oh, ok, the marketplace is just quitting on load, regardless of
> MOZ_DUMP_PAINT_LIST... I guess I'm also using a debug build now vs. earlier,
> perhaps that's affecting things... Not useful.

Another bug I've run into when running a debug build of B2G is bug 820716. My workaround for that is to comment out http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#2496.
ok, so:

Works:
- position:fixed div in browser
- position:fixed div in wrapper
- position:fixed div as installed app
- Firefox Marketplace in browser

Doesn't work:
- Firefox marketplace as installed app

The layer tree for the marketplace, I think:
ClientLayerManager (0x404250d0)
  ClientContainerLayer (0x4383b000) [visible=< (x=0, y=0, w=540, h=930); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=360.000000, h=620.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=360.000000, h=620.000000) scrollId=2 }]
    ClientThebesLayer (0x432f90c0) [not visible] [opaqueContent]
    ClientContainerLayer (0x467f4c00) [clip=(x=0, y=0, w=540, h=930)] [visible=< (x=0, y=0, w=540, h=930); >] [opaqueContent]
      ClientContainerLayer (0x467f5000) [clip=(x=0, y=0, w=540, h=930)] [visible=< (x=0, y=0, w=540, h=3255); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=360.000000, h=620.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=360.000000, h=2170.000000) scrollId=7 }]
        ClientThebesLayer (0x432fbaf0) [visible=< (x=0, y=137, w=540, h=3118); >] [opaqueContent] [valid=< (x=0, y=137, w=540, h=3118); >]
        ClientThebesLayer (0x432fbca0) [transform=[ 1 0; 0 1; 0 72; ]] [visible=< (x=0, y=0, w=540, h=3); >] [isFixedPosition anchor=0.000000,-72.000000 margin=0.000000,0.000000,0.000000,-1.000000] [valid=< (x=0, y=0, w=540, h=32); >]
        ClientThebesLayer (0x432fbe50) [visible=< (x=0, y=72, w=540, h=65); >] [opaqueContent] [valid=< (x=0, y=72, w=540, h=65); >]
        ClientThebesLayer (0x44ce1180) [visible=< (x=0, y=0, w=540, h=72); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000 margin=0.000000,0.000000,0.000000,-1.000000] [valid=< (x=0, y=0, w=540, h=72); >]

Assuming that the fixed-pos header is the last layer in that list, it is marked as fixed position... (the visible region seems about right) So the new question, I suppose, is why it isn't being transformed when it's an app. Will delve further tomorrow.
An interesting thing to note in this layer tree, is that it has two viewports, and the fixed content is a child of the second one.
Having slept on it, comment #28 is almost certainly why this doesn't work - I'm not sure why this app ends up with two viewports, but the scroll offset likely needs to be accumulated when a second viewport is hit like this.

The two possible fixes here are to change the marketplace so it doesn't trigger this behaviour, or to fix the async scroll transform code so that it handles it. I'm going to attempt the latter.
So the problem here is that the root frame has a viewport, and if it does, ApplyAsyncContentTransformToTree gets called and the 'fallback' path, that would do this recursively, doesn't run.

I think, now that we have browser iframes and such, that the fallback path needs to always run, but that AlignFixedAndStickyLayers needs to also stop when it encounters scrollable layers. Something along these lines anyway. I'll write the patch and see if the results make sense.
So, this fixes the issue (and is much simpler than what I was waffling about in the last comment). I'm not sure I have this right, but it seems right to me.

Note, that sometimes the header flickers and disappears when scrolling in the marketplace, I'm not sure if this is because of some weirdness on my Peak (I'm seeing some odd behaviour elsewhere on it that I wouldn't expect), because the untransformed fixed layer ends up falling outside of the displayport, or some other reason. It's likely we'll want to fix this too, and I have a feeling this may need a layout-side fix.
Attachment #8359812 - Flags: review?(roc)
Attachment #8359812 - Flags: feedback?(botond)
Comment on attachment 8359812 [details] [diff] [review]
Fix async scrolling of fixed layers in scrollable sub-layers

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

Looks reasonable to me. (I'm not extensively familiar with this code though.)
Attachment #8359812 - Flags: feedback?(botond) → feedback+
Duplicate of this bug: 959711
Comment on attachment 8359812 [details] [diff] [review]
Fix async scrolling of fixed layers in scrollable sub-layers

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

Looks good, thanks!
Attachment #8359812 - Flags: review?(roc) → review+
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fda811732305
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/fda811732305
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 925026
You need to log in before you can comment on or make changes to this bug.