Closed Bug 811950 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

VERIFIED FIXED
B2G C3 (12dec-1jan)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: jsmith, Assigned: ajones)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
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?
> 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?
(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
> 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.
(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.
> 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.
Maybe Chris can give a more informed hypothesis about this.
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.
Assignee: nobody → ajones
I don't think that fennec or apzc implement position:fixed in an expected way.
Status: NEW → ASSIGNED
Attachment #692805 - Attachment is obsolete: true
Attachment #693234 - Attachment description: 811950 - Fix async scrolling of position:fixed; r=cjones → Fix async scrolling of position:fixed v2
Attachment #693234 - Flags: review?(jones.chris.g)
Summary: postition:fixed CSS rule fails to keep marketplace header at top of browser, but works fine in an app → position:fixed CSS rule fails to keep marketplace header at top of browser, but works fine in an app
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.
Attachment #693234 - Flags: review?(jones.chris.g) → superreview?(roc)
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.
Attachment #693234 - Attachment is obsolete: true
Attachment #693234 - Flags: superreview?(roc)
Attachment #693737 - Attachment description: Added scaling operators to BaseSize and gfxPoint → Part 3: Added scaling operators to BaseSize and gfxPoint
Attachment #693738 - Attachment description: Refactoring to use gfxPoint and gfxSize more → Part 4: Refactoring to use gfxPoint and gfxSize more
Attachment #693738 - Flags: review?(jones.chris.g)
Attachment #693736 - Attachment is obsolete: true
Attachment #694108 - Flags: review?(jones.chris.g)
Attachment #693738 - Flags: review?(jones.chris.g) → review+
Attachment #694108 - Flags: review?(jones.chris.g) → review+
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/ajones@mozilla.com-f5c2355ce402
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/ajones@mozilla.com-919786c0b3c4
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?
(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 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.
Attachment #693737 - Flags: approval-mozilla-b2g18?
blocking-b2g: --- → tef?
Attachment #693737 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Whiteboard: [bug 825808 needs to land first]
blocking-b2g: tef? → tef+
We'll fix this web regression for v1.0.0, since our marketplace may get linked to directly.
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.
Whiteboard: [bug 825808 needs to land first]
Target Milestone: --- → B2G C3 (12dec-1jan)
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.