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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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

Comment 1

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

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) ?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll let Kats answer.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Jason Smith [:jsmith]]

Does this reproduce with APZC not turned on?

Comment 5

[Nikolai Khristoforov]

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

Comment 6

[Nikolai Khristoforov]

Removing qawanted as per comment 5.

Comment 7

[Jason Smith [:jsmith]]

Okay - that implies this isn't a marketplace bug - it's a regression from turning APZC on across the board.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 11

[Christopher Van Wiemeersch [:cvan]]

(In reply to Jason Smith [:jsmith] from comment #10)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 13

[Chris Lord [:cwiiis]]

As far as I know they were all fixed, bar background-attachment:fixed, but this is a question better answered by roc.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Blocking a blocker.

Comment 15

[Chris Lord [:cwiiis]]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

As far as I know, the position:fixed regressions were fixed.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 19

[Chris Lord [:cwiiis]]

(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?

Comment 20

[Benoit Girard (:BenWa)]

(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.

Comment 21

[Chris Lord [:cwiiis]]

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.

Comment 22

[Botond Ballo [:botond]]

(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.

Comment 23

[Botond Ballo [:botond]]

(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.

Comment 24

[Chris Lord [:cwiiis]]

(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.

Comment 25

[Chris Lord [:cwiiis]]

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.

Comment 26

[Botond Ballo [:botond]]

(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.

Comment 27

[Chris Lord [:cwiiis]]

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.

Comment 28

[Chris Lord [:cwiiis]]

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.

Comment 29

[Chris Lord [:cwiiis]]

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.

Comment 30

[Chris Lord [:cwiiis]]

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.

Comment 31

[Chris Lord [:cwiiis]]

Attachment: Fix async scrolling of fixed layers in scrollable sub-layers

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.

Comment 32

[Botond Ballo [: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.)

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 35

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fda811732305

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fda811732305

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/71c4b1a2ec28