811950 - position:fixed CSS rule fails to keep marketplace header at top of browser, but works fine in an app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Build

Device - Unagi
Date - 11/14/2012

Steps

1. Go to marketplace in the browser
2. Scroll up and down the marketplace

Expected

The marketplace header should stay at the top.

Actual

The marketplace header experiences some evidence of chasing the top of the screen as you scroll up. This is not see while in the app, which gets me the feeling that this is a browser api bug.

This is a spin off from bug 781375. The comments in there imply this has something to do with position:fixed.

Comment 1

[Jason Smith [:jsmith]]

My first guess is that this has something to do with the browser api, but I could be wrong. Justin - Any ideas why this issue only shows up in the browser, but works fine in an app?

Comment 2

[Justin Lebar (not reading bugmail)]

> Any ideas why this issue only shows up in the browser, but works fine in an app?

I have just one idea: Change TabChild::IsRootContentDocument to return true unconditionally.  Does that change the behavior here?

Comment 3

[Jason Smith [:jsmith]]

(In reply to Justin Lebar [:jlebar] from comment #2)
> > Any ideas why this issue only shows up in the browser, but works fine in an app?
> 
> I have just one idea: Change TabChild::IsRootContentDocument to return true
> unconditionally.  Does that change the behavior here?

Should we try putting a patch together for that and testing that on device to see what happens then?

http://dxr.mozilla.org/mozilla-central/dom/ipc/TabChild.cpp.html#l1021

Comment 4

[Justin Lebar (not reading bugmail)]

> Should we try putting a patch together for that and testing that on device to see what 
> happens then?

Yes, please, if you wouldn't mind.

Comment 5

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

(In reply to Jason Smith [:jsmith] from comment #1)
> My first guess is that this has something to do with the browser api, but I
> could be wrong. Justin - Any ideas why this issue only shows up in the
> browser, but works fine in an app?

Apps / Browser use a different rendering path afaik.

Comment 6

[Justin Lebar (not reading bugmail)]

> Apps / Browser use a different rendering path afaik.

I believe it's specifically decided by TabChild::IsRootContentDocument.  At least, that's the only difference I've noticed wrt rendering.

Comment 7

[Justin Lebar (not reading bugmail)]

Maybe Chris can give a more informed hypothesis about this.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I've been looking at this with Matt Woodrow and he suggested it might be a problem with CompositorParent::TransformShadowTree() doing the wrong thing. The problem does not occur on desktop or android.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't think that fennec or apzc implement position:fixed in an expected way.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: 811950 - Fix async scrolling of position:fixed; r=cjones

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix async scrolling of position:fixed v2

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 693234 [details] [diff] [review]
Fix async scrolling of position:fixed v2

In the future, please split the addition of general helpers,
refactorings, and new functionality into separate patches as much as
possible.  That makes reviews go faster, which means happier reviewers
and faster reviews ;).

>diff --git a/gfx/2d/BasePoint.h b/gfx/2d/BasePoint.h

>+  Sub operator*(Sub aScale) const {

I'm not sure that Point * Point = Point makes geometric sense.  I
think you want Point * Size = Point.

But do whatever roc says ;).

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>-    shadow->SetShadowTransform(newTransform);
>+    gfx3DMatrix transform(gfx3DMatrix(treeTransform) * aLayer->GetTransform());
>+    // The transform already takes the resolution scale into account.  Since we
>+    // will apply the resolution scale again when computing the effective
>+    // transform, we must apply the inverse resolution scale here.
>+    transform.Scale(1.0F/container->GetPreXScale(),
>+                    1.0F/container->GetPreYScale(),

Nit s/F/f/

>diff --git a/gfx/layers/ipc/ShadowLayersParent.cpp b/gfx/layers/ipc/ShadowLayersParent.cpp

>-      static bool fixedPositionLayersEnabled = getenv("MOZ_ENABLE_FIXED_POSITION_LAYERS") != 0;
>-      if (fixedPositionLayersEnabled) {
>-        layer->SetIsFixedPosition(common.isFixedPosition());
>-        layer->SetFixedPositionAnchor(common.fixedPositionAnchor());
>-      }
>+      layer->SetIsFixedPosition(common.isFixedPosition());
>+      layer->SetFixedPositionAnchor(common.fixedPositionAnchor());

I see another consumer of MOZ_ENABLE_FIXED_POSITION_LAYERS in
nsDisplayList.  Did it not need to be changed too?

>diff --git a/gfx/thebes/gfxPoint.h b/gfx/thebes/gfxPoint.h

>@@ -37,14 +37,36 @@ struct THEBES_API gfxPoint : public mozilla::gfx::BasePoint<gfxFloat, gfxPoint>
>     // And if you need similar method which is using NS_round(), you should
>     // create new |RoundAwayFromZero()| method.

^^^ probably want to nuke that comment

Clearing request pending answer to question above.  sr to roc for gfx changes.

Comment 13

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

Comment on attachment 693234 [details] [diff] [review]
Fix async scrolling of position:fixed v2

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

Yes, as Chris says, this patch should be broken up.

::: gfx/2d/BasePoint.h
@@ +63,5 @@
> +    return Sub(x * aScale.x, y * aScale.y);
> +  }
> +  Sub operator/(Sub aScale) const {
> +    return Sub(x / aScale.x, y / aScale.y);
> +  }

Yes, these should take a size.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix async scrolling of position:fixed v3

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3: Added scaling operators to BaseSize and gfxPoint

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 4: Refactoring to use gfxPoint and gfxSize more

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Remove fixed position environment variable

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix async scrolling of position:fixed v4

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=f5c2355ce402

Comment 20

[Mozilla RelEng Bot]

Try run for f5c2355ce402 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f5c2355ce402
Results (out of 101 total builds):
    exception: 3
    success: 73
    warnings: 13
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f5c2355ce402

Comment 21

[Mozilla RelEng Bot]

Try run for 919786c0b3c4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=919786c0b3c4
Results (out of 99 total builds):
    exception: 1
    success: 85
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-919786c0b3c4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1310b5aab1f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/955a8b88d796
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e5744e4e2f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9dc5d37fac

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1310b5aab1f4
https://hg.mozilla.org/mozilla-central/rev/955a8b88d796
https://hg.mozilla.org/mozilla-central/rev/0e5744e4e2f6
https://hg.mozilla.org/mozilla-central/rev/cb9dc5d37fac

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 693738 [details] [diff] [review]
Part 4: Refactoring to use gfxPoint and gfxSize more

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

Huh. Wasn't gfx::Point the new hotness?

Comment 25

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to :Ms2ger from comment #24)
> Huh. Wasn't gfx::Point the new hotness?

Perhaps. I guess it depends on how much you consider Azure to be a separate thing. It also depends on how much code you want to change at any given point.

Comment 26

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 693737 [details] [diff] [review]
Part 3: Added scaling operators to BaseSize and gfxPoint

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
positions:fixed has never worked on async scrolling
 
User impact if declined:
position:fixed things float across the screen when scrolling. This makes sites that use position:fixed look nasty on b2g.

Testing completed:
Has been in m-c since christmas.

Risk to taking this patch (and alternatives if risky): 
May conflict with other code that sets a shadow transform. One such bug has been found and fixed. This bug also requires 825808 828249 to land on b2g18.

String or UUID changes made by this patch:
None.

Comment 27

[Alex Keybl [:akeybl]]

We'll fix this web regression for v1.0.0, since our marketplace may get linked to directly.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/816ed33e790f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d43f324b618b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e22778e3dc31
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a4bfb86d0e64

These didn't apply very cleanly to b2g18, so please look the changesets over carefully to make sure no issues crept in during unbitrotting.

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0

Comment 30

[Sam Shipley]

Issue doesn't reproduce on Unagi device:

Build ID: 20130214070203December 5th KernelGaia: 6544fdb8dddc56f1aefe94482402488c89eeec49Gecko http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e

marketplace header stays on top of the marketplace browser