Closed Bug 876542 Opened 11 years ago Closed 11 years ago

Fullscreen broken for content in gaia browser app

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25
Project Flags:
blocking-b2g -
Tracking Flags:
Tracking Status
firefox25 --- fixed
b2g18 --- affected
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- affected

People

(Reporter: cpearce, Assigned: cwiiis)

References

(
URL
)

Details

(Whiteboard: leorun3 [LeoVB-], burirun3)

Attachments

(10 files, 5 obsolete files)

Photo of fullscreen broken in browser app
379.58 KB, image/jpeg
Details
Photo of fullscreen working in stand alone web app
326.40 KB, image/jpeg
Details
Photo of fullscreen working in latest browser app
30.52 KB, image/png
Details
more comprehensive fix
11.32 KB, patch
cwiiis
: review+
ajones
: feedback+
Details | Diff | Splinter Review
roc's patch revised and rebased on m-c
15.83 KB, patch
roc
: feedback+
ajones
: feedback+
Details | Diff | Splinter Review
Idea for reducing some of the noise
6.56 KB, patch
cwiiis
: feedback-
Details | Diff | Splinter Review
Restore async fixed margin reconciliation
11.76 KB, patch
roc
: review+
kats
: review+
Details | Diff | Splinter Review
Restore async reconciliation during overscroll behaviour
2.93 KB, patch
roc
: review+
Details | Diff | Splinter Review
B2g18 fix
8.97 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
Fix-ups on top of attachment #770195
4.91 KB, patch
kats
: review+
Details | Diff | Splinter Review 
Fullscreen from webcontent is broken in the Gaia browser app.

STR:
1. Open http://pearce.org.nz/f in the browser app.
2. Touch the "toggle fullscreen" button, opt to "allow fullscreen" when prompted.
3. Observe that the blue div with grey background does not take up the whole screen, and that the white background can be sort of scrolled/panned.

Note that if you click the "Install as web app" button on that page you can run the app outside of the browser, and fullscreen works there.

The browser app must be interfering with the fullscreen styles somehow.
Photo of http://pearce.org.nz/f having requested fullscreen in the gaia browser app. This is incorrect behaviour
Photo of http://pearce.org.nz/f running as an installed web app in fullscreen mode. This is the correct behaviour.
This bug occurs in v1.0.1 and v1.1.
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → affected
status-b2g18-v1.0.1: --- → affected
The same bug occurs in Fennec, I filed bug 876562 to track that. It may very well be a bug in the async pan zoom controller and fixed position content.
How bad is this if we don't fix this for 1.01? Do we need to block on this?
I don't think it's worth blocking on, this only affects content in the browser, fullscreen in other webapps works fine.
Okay. This probably isn't a Browser app bug - probably something in the Browser API specifically in relation to the DOM.
status-b2g18-v1.0.0: affected → wontfix
status-b2g18-v1.0.1: affected → wontfix
status-firefox24: --- → affected
Component: Gaia::Browser → DOM
Product: Boot2Gecko → Core
Version: unspecified → Trunk
(In reply to Chris Pearce (:cpearce) from comment #0)
> 3. Observe that the blue div with grey background does not take up the whole
> screen, and that the white background can be sort of scrolled/panned.

This is exactly the behaviour I observed in bug 874905 - the fullscreen content does not take up the full screen and the content in the background is pannable.

In addition to that, the video didn't render, as you described in https://bugzilla.mozilla.org/show_bug.cgi?id=826130#c2

I think this is a duplicate of bug 874905.

As far as I know the browser app has no control over these styles. Given that the main difference between the rendering of browser/wrapper content vs. installed apps is the AZPC I think that's a likely culprit, though I thought Firefox OS used different AZPC code to Fennec where it is also reproducible.

In bug 874905 you said http://pearce.org.nz/fullscreen works in Fennec 20, but in bug 876562 you say the same page doesn't work in Fennec Nightly so is this a regression in Gecko?
Chris Lord: Can you take a look at this? You know more about AZPC and layers than I.
Flags: needinfo?(chrislord.net)
There's an extremely simple fullscreen testcase in attachment 756531 [details].
I opened http://people.mozilla.com/~tmielczarek/fullscreen.html on my inari and was able to enter fullscreen mode after tapping 'allow'.

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/11b55d3ada71
Gaia   ac293ce59acc3bede083fad1b973794fa8bf0253
BuildID 20130529230209
Version 18.0
I just commented in bug 876562 that I think this could be a regression from the core changes in bug 716403, which landed in Gecko 22.

(Also, FYI, Chris Lord is on vacation, so he probably won't get to this for a while.)
Here's a reduced testcase that mimics the CSS styles applied to fullscreen and fullscreen-ancestor elements, but doesn't actually use the fullscreen API:

http://bit.ly/19ecjil

The behaviour on desktop is correct, behaviour on mobile should match desktop's behaviour, but doesn't.
URL: http://bit.ly/19ecjil
Whiteboard: leorun3
http://pearce.org.nz/f looks correct on b2g trunk and gaia master (w/ unagi).
Testing environment:

m-c: 36da3cb92193
gaia: 3e9090894daaa1c7f894a1dcc1026b21f889eadc
Note: it' can't be scrolled after fullscreen..
(In reply to Kan-Ru Chen [:kanru](Mozilla Corporation) from comment #17)
> Created attachment 763995 [details]
> Photo of fullscreen working in latest browser app
> 
> Note: it' can't be scrolled after fullscreen..

This is intended behaviour. If you want fullscreen content to be scrollable, request fullscreen on the document.documentElement.
It's almost certainly a bug in CompositorParent involving the interaction of positioning of fixed-pos elements with scaling. Zooming in or out makes the bug better/worse.
A comment in bug 880587 says that this is fixed in m-c, and I recall kats having either worked on or reviewed a patch that may have fixed this, so adding needinfo.
Flags: needinfo?(chrislord.net) → needinfo?(bugmail.mozilla)
The only patch I reviewed related to this was tn's patch for Fennec in bug 876562. I don't think that would affect B2G so if this is fixed in m-c then we should track down the fix window.
Flags: needinfo?(bugmail.mozilla)
I have a fix for this.
Assignee: nobody → roc
Attached patch fix (18 branch) (obsolete) — — Splinter Review 
I do not like this code at all.

The call from TransformScrollableLayer to TransformFixedLayers probably needs to be changed to fix Android. However we don't care about Android on the 18 branch, and I don't understand the code here, so I'm leaving it alone.
Attachment #766540 - Flags: review?
Attached patch fix v2 (obsolete) — — Splinter Review 
Oops, removing debug spew.

Please solicit additional review as needed.

It looks like the early return in TransformFixedLayers was in the wrong place, so I moved it.
Attachment #766541 - Flags: review?(ajones)
I've filed bug 886298 for hopefully making this code better on m-c.
Attachment #766541 - Flags: review?(ajones) → review+
Need some kind of approval to land this patch.
blocking-b2g: --- → leo?
blocking-b2g: leo? → leo+
With comment 27, should status-b2g18 be set to fixed?
This was waiting on bug 886298 - I'll forward-port the patch here.
A straight port shows that fixed position content is positioned incorrectly by the compositor - I assume this is just because of the TransformFixedLayers change, will sort that out and attach the revised patch.
Only change is the scaleDiff / userZoom.scale, and I added a couple of comments.
Attachment #768305 - Flags: review?(ajones)
Attachment #768305 - Flags: review?(ajones) → review+
dang, so I was working on top of this to fix bug 886298 and I've come to the realisation that this isn't quite right - I think at this point it's easier to just roll these into one patch and fix it there. Marking as dependent.
Depends on: 886298
Just to clarify what I think isn't quite right (it's more than likely I've missed something..);

TransformFixedLayers does multiple things (which I think is very unclear (and my fault, because I think I wrote it...));
- It translates all layers by a given 2d point (aTranslation) - this point is considered to be relative to the 'root' layer*
- It makes sure fixed layers are offset correctly based on a currently applied scale (aScaleDiff) and their own fixed-position-anchor
- It reconciles the difference between the given margins (aFixedLayerMargins) and the fixed-position-margins of the layer, taking into account the scale (aScaleDiff)

It doesn't seem to me that the attached patch is right - I don't understand why you'd only accumulate 2d zoom transforms, will this just break when a layer is rotated slightly? It just seems to me that the value passed in as aScaleDiff for b2g was wrong, as opposed to needing to accumulate anything.

As a side-note, this code will likely break (pre and post patch) with nested APZC's.
Coming to the conclusion that the original code is right, the b2g path just passes in a wrong aScaleDiff.
This patch is wrong, and I should probably back it out to fix bug 887326, but I don't think the original patch was right either. We definitely need to modify aScaleDiff as we descend through layers that have a scaling transform set on them (including pre or post scale).
I've backed out the patch for b2g18, to fix bug 887326. We need to re-fix this properly.
status-b2g18: fixed → affected
(In reply to Chris Lord [:cwiiis] from comment #34)
> TransformFixedLayers does multiple things (which I think is very unclear
> (and my fault, because I think I wrote it...));
> - It translates all layers by a given 2d point (aTranslation) - this point
> is considered to be relative to the 'root' layer*
> - It makes sure fixed layers are offset correctly based on a currently
> applied scale (aScaleDiff) and their own fixed-position-anchor

My understanding is that the only purpose of these effects is to ensure that the anchor point of the fixed layer remains stationary after CompositorParent::ApplyAsyncContentTransformToTree applies its treeTransform to its aLayer. Right?

> - It reconciles the difference between the given margins
> (aFixedLayerMargins) and the fixed-position-margins of the layer, taking
> into account the scale (aScaleDiff)

This doesn't exist on b2g18 so I'm going to ignore it for now.

> It doesn't seem to me that the attached patch is right - I don't understand
> why you'd only accumulate 2d zoom transforms, will this just break when a
> layer is rotated slightly?

If TransformFixedLayers is (recursively) invoked on a layer which has a rotation transform, then everything TransformFixedLayers does to that layer (or its descendants) makes no sense --- in general, in the presence of rotation, keeping the anchor point stationary will require more than just inserting a translation somewhere.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> (In reply to Chris Lord [:cwiiis] from comment #34)
> > TransformFixedLayers does multiple things (which I think is very unclear
> > (and my fault, because I think I wrote it...));
> > - It translates all layers by a given 2d point (aTranslation) - this point
> > is considered to be relative to the 'root' layer*
> > - It makes sure fixed layers are offset correctly based on a currently
> > applied scale (aScaleDiff) and their own fixed-position-anchor
> 
> My understanding is that the only purpose of these effects is to ensure that
> the anchor point of the fixed layer remains stationary after
> CompositorParent::ApplyAsyncContentTransformToTree applies its treeTransform
> to its aLayer. Right?

Assuming this is the case, then it makes sense for TransformFixedLayers to take into account all scaling that is applied after the translation induced by TransformFixedLayers.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> in the presence of
> rotation, keeping the anchor point stationary will require more than just
> inserting a translation somewhere.

I guess that's not true. I expect that as long as everything is nonsingular we can find a translationthat keeps the anchor point stationary. Things could get weird though.
So I think we can fix this by accumulating the transforms as we recurse down layers, applying the inverse of that matrix to the translation matrix we want and then post-applying that to the layer transform matrix - or at least, something along those lines.

The current method will alter the meaning of existing transforms on the layer so that rotation and scaling will have incorrect focus points, as you point out.

Assuming this is correct, I'll fix this as part of bug 886298 and the back-port should be easy (which I will also do).
This patch reworks TransformFixedLayers so we don't try to pass around a translation or aScaleDiff anymore. Instead we explicitly make it responsible for preserving the position of the anchor point.

This worked first time for the fixed-pos testcases I've been looking at. It feels like a much cleaner and more understandable approach to me. It also seems more robust --- it punts on 3D transforms and singular transforms, but other than that, it should handle any case.

I'm not sure how it would impact Android. For b2g18, that doesn't really matter. The fact that we don't use 'offset' and 'scaleDiff' computed by TransformScrollableLayer is a little alarming, but maybe they're just not needed with this approach. I have to admit I don't understand why they're currently needed.
Attachment #766541 - Attachment is obsolete: true
Attachment #768305 - Attachment is obsolete: true
Attachment #769624 - Flags: review?
Attachment #769624 - Flags: feedback?(ajones)
Attachment #769624 - Flags: review? → review?(chrislord.net)
Comment on attachment 769624 [details] [diff] [review]
more comprehensive fix

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

So this is good for b2g, but this will cause some regressions for fennec (which I don't think matters, so r+ for this with the comments addressed).

On fennec, aScaleDiff and offset aren't used, so the behaviour of fixed layers during overscroll and pinch-zoom overscroll (i.e. zooming out far enough so that the page becomes smaller than the screen) will change. These transformations probably ought to be applied separately though, it'd certainly make things easier to understand.

Is there any reason this patch restricts these transformations to 2D transformed layers only? It seems the logic would work for 3D transforms too.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +601,3 @@
>        !aLayer->GetParent()->GetIsFixedPosition()) {
> +    // Insert a translation so that the position of the anchor point is the same
> +    // before and after the change to the transform of aTransformedSubtreeRoot.

I think add an extra comment here noting that this currently only works for 2D transforms.

@@ +610,5 @@
> +        return;
> +      }
> +      ancestorTransform = ancestorTransform.Multiply(l2D);
> +    }
> +    gfxMatrix oldRootTransform;

Let's add a new line before this, help readability.

@@ +616,5 @@
> +    if (!aPreviousTransformForRoot.Is2D(&oldRootTransform) ||
> +        !aTransformedSubtreeRoot->GetLocalTransform().Is2D(&newRootTransform)) {
> +      return;
> +    }
> +    gfxMatrix oldCumulativeTransform = ancestorTransform.Multiply(oldRootTransform);

And before here too.

@@ +640,5 @@
>      // will apply the resolution scale again when computing the effective
>      // transform, we must apply the inverse resolution scale here.
> +
> +    layerTransform.x0 += translation.x;
> +    layerTransform.y0 += translation.y;

These two translate lines should be above the above comment, and I think they could do with their own comment, just 'Finally, apply the 2D translation' or something along those lines.

::: gfx/layers/ipc/CompositorParent.h
@@ +238,2 @@
>     */
>    void TransformFixedLayers(Layer* aLayer,

Probably time we renamed this function in this case - how about TranslateFixedLayersForAnchorPoint?
Attachment #769624 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #44)
> On fennec, aScaleDiff and offset aren't used, so the behaviour of fixed
> layers during overscroll and pinch-zoom overscroll (i.e. zooming out far
> enough so that the page becomes smaller than the screen) will change. These
> transformations probably ought to be applied separately though, it'd
> certainly make things easier to understand.

I want to find out more about this.

> Is there any reason this patch restricts these transformations to 2D
> transformed layers only? It seems the logic would work for 3D transforms too.

It could, but it's a more complex and more overhead and I don't think we care about the 3D case really.

> ::: gfx/layers/ipc/CompositorParent.h
> @@ +238,2 @@
> >     */
> >    void TransformFixedLayers(Layer* aLayer,
> 
> Probably time we renamed this function in this case - how about
> TranslateFixedLayersForAnchorPoint?

AlignFixedLayersAtAnchorPoint?
We discussed this on IRC, but adding here for public record;

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> (In reply to Chris Lord [:cwiiis] from comment #44)
> > On fennec, aScaleDiff and offset aren't used, so the behaviour of fixed
> > layers during overscroll and pinch-zoom overscroll (i.e. zooming out far
> > enough so that the page becomes smaller than the screen) will change. These
> > transformations probably ought to be applied separately though, it'd
> > certainly make things easier to understand.
> 
> I want to find out more about this.

It might actually end up that this new method covers these situations, for the most part - what it won't cover, having thought about it a bit longer, is the case where, say, the page width fits within the screen with space to spare, but the vertical axis doesn't. The old code, in this case, would apply different offsets to each axis so that fixed position layers only moved on the horizontal axis as necessary.

tbh, I don't think this is important, but I'll see if it's something that can be easily replicated on top of this code when I forward-port it.

> > Is there any reason this patch restricts these transformations to 2D
> > transformed layers only? It seems the logic would work for 3D transforms too.
> 
> It could, but it's a more complex and more overhead and I don't think we
> care about the 3D case really.

I agree, and as you explained on IRC, fixed-pos layers can't be descendents of 3d transforms (for the most part).

> > ::: gfx/layers/ipc/CompositorParent.h
> > @@ +238,2 @@
> > >     */
> > >    void TransformFixedLayers(Layer* aLayer,
> > 
> > Probably time we renamed this function in this case - how about
> > TranslateFixedLayersForAnchorPoint?
> 
> AlignFixedLayersAtAnchorPoint?

Sounds good.
And apparently re-landed with commenting in the bug...
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1384f32d3f99
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/1384f32d3f99

Still assuming something needs to be done on m-c, so leaving open.
status-b2g18: affected → fixed
status-b2g-v1.1hd: affected → fixed
Comment on attachment 769624 [details] [diff] [review]
more comprehensive fix

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

LGTM.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +633,5 @@
> +      layerTransform.Transform(aLayer->GetFixedPositionAnchor());
> +    gfxPoint oldAnchorPositionInNewSpace =
> +      newCumulativeTransformInverse.Transform(
> +        oldCumulativeTransform.Transform(locallyTransformedAnchor));
> +    gfxPoint translation = oldAnchorPositionInNewSpace - locallyTransformedAnchor;

This is a lot of code to do something logically simple. It should therefore be in a separate function, perhaps CalculateTranslationRelativeToAnchor(). Would it be simpler without the 2d-ifying?
Attachment #769624 - Flags: feedback?(ajones) → feedback+
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #49)
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +633,5 @@
> > +      layerTransform.Transform(aLayer->GetFixedPositionAnchor());
> > +    gfxPoint oldAnchorPositionInNewSpace =
> > +      newCumulativeTransformInverse.Transform(
> > +        oldCumulativeTransform.Transform(locallyTransformedAnchor));
> > +    gfxPoint translation = oldAnchorPositionInNewSpace - locallyTransformedAnchor;
> 
> This is a lot of code to do something logically simple. It should therefore
> be in a separate function, perhaps CalculateTranslationRelativeToAnchor().

You're probably right, but I already landed it. Something for the m-c landing.

> Would it be simpler without the 2d-ifying?

Not really, I think.
Comment on attachment 769624 [details] [diff] [review]
more comprehensive fix

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +617,5 @@
> +        !aTransformedSubtreeRoot->GetLocalTransform().Is2D(&newRootTransform)) {
> +      return;
> +    }
> +    gfxMatrix oldCumulativeTransform = ancestorTransform.Multiply(oldRootTransform);
> +    gfxMatrix newCumulativeTransform = ancestorTransform.Multiply(newRootTransform);

So I just spent several hours trying to figure out why this function wasn't doing what I expected, and missed this... You've used Multiply twice on the same matrix here, but Multiply operates on the matrix it's called on. You can use operator* instead, which automatically makes a copy.

I guess this could give the impression of working if the old root transform is always the identity matrix (which I guess it is on b2g?). Anyway, I'll fix this as part of the forward-port.
I'm going to take this bug because bug 886298 depends too heavily on it and I think it's likely I know this code better than others (having written a lot of it).

This is roc's patch with the Multiply fix and the fennec code modified to work with it - I've tested it and it works great :)

Unfortunately, this does not handle the fixed position margins or overscroll like the previous code did, so I'll handle these in two follow-up patches.

I'm assuming this is still r+'d code, but I'd like feedback on the changes I've made (I've added comments, alongside the fennec fixes). I didn't split out the transform relative point bit into a function because all my attempts at doing so made the code harder to read rather than easier. I hope the extra commenting will suffice instead.
Assignee: roc → chrislord.net
Status: NEW → ASSIGNED
Attachment #770195 - Flags: feedback?(roc)
Attachment #770195 - Flags: feedback?(ajones)
(In reply to Chris Lord [:cwiiis] from comment #51)
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +617,5 @@
> > +        !aTransformedSubtreeRoot->GetLocalTransform().Is2D(&newRootTransform)) {
> > +      return;
> > +    }
> > +    gfxMatrix oldCumulativeTransform = ancestorTransform.Multiply(oldRootTransform);
> > +    gfxMatrix newCumulativeTransform = ancestorTransform.Multiply(newRootTransform);
> 
> So I just spent several hours trying to figure out why this function wasn't
> doing what I expected, and missed this... You've used Multiply twice on the
> same matrix here, but Multiply operates on the matrix it's called on. You
> can use operator* instead, which automatically makes a copy.
> 
> I guess this could give the impression of working if the old root transform
> is always the identity matrix (which I guess it is on b2g?). Anyway, I'll
> fix this as part of the forward-port.

Gah! We'd better fix that on 18!
Comment on attachment 770195 [details] [diff] [review]
roc's patch revised and rebased on m-c

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +136,2 @@
>        }
> +      ancestorTransform = ancestorTransform.Multiply(l2D);

The assignment isn't needed, right?
Attachment #770195 - Flags: feedback?(roc) → feedback+
I'm exploring an idea for removing some of the noise out of the code (i.e checking for edge cases). It may not be useful because you still need to check the valid flags at the end. It would also be cleaner if we have a gfxMaybePoint where we could just check if the final translation point is valid. Let me know what you think.
Attachment #770573 - Flags: feedback?(roc)
Attachment #770573 - Flags: feedback?(chrislord.net)
Attachment #770195 - Flags: feedback?(ajones) → feedback+
Comment on attachment 770573 [details] [diff] [review]
Idea for reducing some of the noise

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

I don't think this is worthwhile, personally. If we had a lot more code like this, it would be.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57)
> I don't think this is worthwhile, personally. If we had a lot more code like
> this, it would be.

It was a lot cleaner in my head :-P
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #55)
> Comment on attachment 770195 [details] [diff] [review]
> roc's patch revised and rebased on m-c
> 
> Review of attachment 770195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +136,2 @@
> >        }
> > +      ancestorTransform = ancestorTransform.Multiply(l2D);
> 
> The assignment isn't needed, right?

Another good catch :)
Comment on attachment 770573 [details] [diff] [review]
Idea for reducing some of the noise

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

This doesn't look to me like it saves us very much - I like the idea, but I'd prefer if gfxMatrix just always worked like this rather than have a whole new type for it.
Attachment #770573 - Flags: feedback?(chrislord.net) → feedback-
This restores async fixed margin reconciliation. This does not restore correct behaviour during overscroll/underzoom.
Attachment #772746 - Flags: review?(roc)
Attachment #772746 - Flags: review?(bugmail.mozilla)
Comment on attachment 772746 [details] [diff] [review]
Restore async fixed margin reconciliation

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +155,5 @@
> +                            Layer* aAncestor,
> +                            gfxMatrix& aMatrix)
> +{
> +  // Accumulate the transforms between this layer and the subtree root layer.
> +  for (Layer* l = aLayer; (l != aAncestor) && (l != nullptr);

just "l && l != aAncestor"
Attachment #772746 - Flags: review?(roc) → review+
Comment on attachment 772746 [details] [diff] [review]
Restore async fixed margin reconciliation

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

I admit that this doesn't make 100% sense to me but I looked over roc's patch and this one and the parts that make sense seem correct.

Also in the future please split patches like these into refactoring and substantive changes. bz was right [1] that review time increases non-linearly when I have to verify that code moved wasn't modified at the same time.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/UCio5fB4VJo/KEeyULRD8DAJ
Attachment #772746 - Flags: review?(bugmail.mozilla) → review+
This restores the old behaviour of making sure fixed position layers remain within the page area during overscroll/under-zoom.

After this patch, this series of patches doesn't regress Android and should still fix b2g.
Attachment #773323 - Flags: review?(roc)
Attachment #773323 - Flags: review?(bugmail.mozilla)
Depends on: 891180
Depends on: 891142
The regressions in bug 891180 and 891142 are worse than this bug and I can't fix them in the next day or two. I'm going to back out the b2g18 patches out.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #67)
> Backout: https://hg.mozilla.org/releases/mozilla-b2g18/rev/aaa74e5efaf1

Do we still want this on m-c, given these regressions? I guess so seeing as it isn't shipping and we probably want to do things in this way long-term...
Well now that's irritating - rebasing these patches on latest m-c and something's broken... Will track it down.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #67)
> Backout: https://hg.mozilla.org/releases/mozilla-b2g18/rev/aaa74e5efaf1

Ryan - Can you back this out on 1.1hd?
Flags: needinfo?(ryanvm)
I merge b2g18 to v1.1hd 1-2 times a day. The backout will come over next time I do.
Flags: needinfo?(ryanvm)
Comment on attachment 773323 [details] [diff] [review]
Restore async reconciliation during overscroll behaviour

Dropping review flag for now as it looks like things still need to be figured out here. That being said I did look at this patch and it looks ok, except for the part where you have underZoomScale.width and .height being calculated separately. If those factors are different then the layer will look stretched when it shouldn't.

We're moving towards abolishing separate x- and y-scale factors and just using a single scale factor for both dimensions, so I would prefer making underZoomScale a ScreenToScreenScale instance that is assigned the min of the two values, if that makes sense.
Attachment #773323 - Flags: review?(bugmail.mozilla)
Comment on attachment 770195 [details] [diff] [review]
roc's patch revised and rebased on m-c

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +489,5 @@
>                 "overwriting animated transform!");
> +
> +  // Apply resolution scaling to the old transform - the layer tree as it is
> +  // doesn't have the necessary transform to display correctly.
> +  oldTransform.Scale(geckoZoom.scale, geckoZoom.scale, 1);

Nice, this line isn't needed on m-c - I'm going to assume we're all ok with this :)
Depends on: 891155
(In reply to Chris Lord [:cwiiis] from comment #73)
> Comment on attachment 770195 [details] [diff] [review]
> roc's patch revised and rebased on m-c
> 
> Review of attachment 770195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +489,5 @@
> >                 "overwriting animated transform!");
> > +
> > +  // Apply resolution scaling to the old transform - the layer tree as it is
> > +  // doesn't have the necessary transform to display correctly.
> > +  oldTransform.Scale(geckoZoom.scale, geckoZoom.scale, 1);
> 
> Nice, this line isn't needed on m-c - I'm going to assume we're all ok with
> this :)

Spoke to soon about this - it's not that the line isn't needed, just that it's inaccurate - it just needs to take into account mDevPixelsPerCSSPixel (which makes perfect sense). Again, I assume that we're ok with this as adding that makes this line make more sense in terms of units.
In my limited testing, this series of patches seems fine for fennec and I guess the regressions on b2g will need to be dealt with at some point.

Seeing as my work in bug 886298 will depend on this, should we push it to m-c?
Flags: needinfo?(roc)
I don't think we should push it to m-c while it breaks b2g badly. You're welcome to help me track that down :-)
Flags: needinfo?(roc)
With the b2g18 patch reapplied, I can't reproduce bug 891180 or bug 891142 :-(.
status-b2g-v1.1hd: affected → fixed
OK, after updating B2G18 and Gaia and reapplying the patch I can reproduce bug 891180.
I'm also having a look at this today, between meetings  and such.
This is caused by using GetLocalTransform instead of GetTransform for oldTransform in ApplyAsyncContentTransformToTree - similarly to fennec, the oldTransform for gaia also needs to take into account the resolution.

I'll attach a revised patch for review.
I just spent much longer figuring out the same thing!

But I also think we should make TransformShadowTree on the b2g18 branch never call TransformScrollableLayer. As far as I know, TransformScrollableLayer should never be used by b2g, but we do enter it, for example in bug 891180 when you log into GMail through a cross-process IFRAME hosted by the contacts app. The subdocument gets a FrameMetrics created, but there's no APZC, so we enter the TransformScrollableLayer code to scroll it. That seems bad since that code is not really maintained on the 18 branch.
Hmm, I think GetLocalTransform isn't quite what we want, since we want to use the animated transform if there is one.
Attached patch not-working (obsolete) — — Splinter Review 
This doesn't work yet. It doesn't work on http://people.mozilla.com/~roc/apzc-bug.html for example.

I wonder why my patch that seemed to work before is now wrong...
It looks like the patch doesn't work on that page, for me, because the "oldTransform" is a scale by 3.062500 and the new transform is the identity. So to keep the red fixed-pos element in the same place we translate it right out of the window.
Blocks: 886298
No longer depends on: 886298
Attached patch B2G follow-up (obsolete) — — Splinter Review 
So this on top of attachment 770195 [details] [diff] [review] fixes the issue on b2g for me and oughtn't regress Android either (I need to test that, but it doesn't really change anything there). I've kept them separate to make it easier to backport to b2g18.

Sorry to ping you on this as well kats, but I want to make sure this looks like it makes sense to you too.
Attachment #776377 - Flags: review?(roc)
Attachment #776377 - Flags: review?(bugmail.mozilla)
Punting to 1.2 given the possibility of regression and where we are in the release.
blocking-b2g: leo+ → -
Attached patch B2g18 fix — — Splinter Review 
This is the b2g18 version of the patch.
Attachment #776327 - Attachment is obsolete: true
Attachment #776717 - Flags: review?(chrislord.net)
I did test the patch in comment #88 on all the bugs the previous patch caused, and it seems to fix everything with no regressions.
Comment on attachment 776717 [details] [diff] [review]
B2g18 fix

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

This looks good to me, and makes a lot more sense on the b2g18 branch. I think it's worth getting someone else to look at this though, I'm tagging kats (sorry!), but feel free to pass on to :BenWa or :kentuckyfriedtakahe, or someone like that.
Attachment #776717 - Flags: review?(chrislord.net)
Attachment #776717 - Flags: review?(bugmail.mozilla)
Attachment #776717 - Flags: review+
Comment on attachment 776377 [details] [diff] [review]
B2G follow-up

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

Makes sense. For my own future self, this patch is just making the behaviour of the ApplyAsyncContentTransformToTree path consistent with the TransformScrollableLayer path.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +400,5 @@
>  
> +    // Apply resolution scaling to the old transform - the layer tree as it is
> +    // doesn't have the necessary transform to display correctly.
> +#ifdef MOZ_WIDGET_ANDROID
> +    LayoutDeviceToLayerScale resolution(1.0 / rootTransform.GetXScale(),

I would like a comment just inside the MOZ_WIDGET_ANDROID ifdef either pointing to bug 732971 or the other ifdef in TransformShadowTree for an explanation of why it is needed.
Attachment #776377 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 776717 [details] [diff] [review]
B2g18 fix

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

I'm leery of killing the TransformScrollableLayer because it is used on B2G as well, in cases where there is no APZC. It may not get used often but I've definitely had breakpoints in there get hit and if you remove the code I suspect there will be some cases that break. Is there any reason we can't just leave it?
Attachment #776717 - Flags: review?(bugmail.mozilla)
Comment on attachment 769624 [details] [diff] [review]
more comprehensive fix

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +632,5 @@
> +    gfxPoint locallyTransformedAnchor =
> +      layerTransform.Transform(aLayer->GetFixedPositionAnchor());
> +    gfxPoint oldAnchorPositionInNewSpace =
> +      newCumulativeTransformInverse.Transform(
> +        oldCumulativeTransform.Transform(locallyTransformedAnchor));

I was looking over this code again today as part of reviewing the other patches. I'm not yet convinced it makes sense. At first I thought "oldAnchorPositionInNewSpace" should really be called "newAnchorPositionInOldSpace" because it's getting transformed out of the new (via newCumulativeTransformInverse) and into the old (via oldCumulativeTransform). I'm not 100% sure about that either now. Also I'm not sure that "oldAnchorPositionInNewSpace" and "locallyTransformedAnchor" are in the same coordinate space (in fact I'm almost positive they're not) so I don't think you can just subtract the two like that. More comments and/or explanation here would be good.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #93)
> I'm leery of killing the TransformScrollableLayer because it is used on B2G
> as well, in cases where there is no APZC. It may not get used often but I've
> definitely had breakpoints in there get hit and if you remove the code I
> suspect there will be some cases that break. Is there any reason we can't
> just leave it?

It does get hit, but I don't think it *should* be hit, because AFAIK we don't have any code in B2G that sets up a displayport in the non-APZC case. And I don't trust the TransformScrollableLayer for use by non-Android-Java anyway.
+    gfxMatrix layerTransform;
+    if (!GetBaseTransform2D(aLayer, &layerTransform)) {
+      return;
+    }
+    gfxPoint locallyTransformedAnchor =
+      layerTransform.Transform(aLayer->GetFixedPositionAnchor());
+    gfxPoint oldAnchorPositionInNewSpace =
+      newCumulativeTransformInverse.Transform(
+        oldCumulativeTransform.Transform(locallyTransformedAnchor));
+    gfxPoint translation = oldAnchorPositionInNewSpace - locallyTransformedAnchor;

I believe that oldCumulativeTransform.Transform(locallyTransformedAnchor) returns the point relative to aTransformedSubtreeRoot's parent that aLayer's anchor point was mapped to before aTransformedSubtreeRoot's transform changed. Then newCumulativeTransformInverse.Transform maps that back down to a point relative to aLayer's parent. locallyTransformedAnchor is also relative to aLayer's parent, so subtracting is valid and returns a vector from locallyTransformedAnchor to the point where the anchor needs to be.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #95)
> It does get hit, but I don't think it *should* be hit, because AFAIK we
> don't have any code in B2G that sets up a displayport in the non-APZC case.
> And I don't trust the TransformScrollableLayer for use by non-Android-Java
> anyway.

Ok, I agree that the transform applied in TransformScrollableLayer is never used on the B2G side. But does shadow->SetShadowTransform still need to be called at all (even if it's just with the unmodified pre-existing transform)?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #96)
> I believe that oldCumulativeTransform.Transform(locallyTransformedAnchor)
> returns the point relative to aTransformedSubtreeRoot's parent that aLayer's
> anchor point was mapped to before aTransformedSubtreeRoot's transform
> changed. Then newCumulativeTransformInverse.Transform maps that back down to
> a point relative to aLayer's parent. locallyTransformedAnchor is also
> relative to aLayer's parent, so subtracting is valid and returns a vector
> from locallyTransformedAnchor to the point where the anchor needs to be.

Ok, this makes sense to me now. I think my problem was that I was thinking of transforms backwards. Given a chain of transforms from an ancestor layer S to a descendant layer T, applying the transforms to a layer X will move X from being relative to T to be relative to S. I was thinking it would move X from being relative to S to being relative to T, which is incorrect. Thanks for explanation!
I think we're good to go after this, but would like to get confirmation from kats that the comment I've added reads well enough.

I'll commit this and attachment #770195 [details] [diff] [review] folded, it doesn't make sense to have them separately.
Attachment #776377 - Attachment is obsolete: true
Attachment #778501 - Flags: review?(bugmail.mozilla)
Just thought I'd mention this. I'm testing on the Peak, using version 1.2.0-prerelease. When I go into fullscreen mode, it does seem to work, however the content is scaled down half of what it should be. The Peak has a higher resolution, and if I run it as an app it is scaled correctly (twice as dense).

You can run it here: http://webfighter.paas.allizom.org/.

Not sure if this is just a Peak bug or something real in Gaia.
(In reply to James Long (:jlongster) from comment #101)
> Just thought I'd mention this. I'm testing on the Peak, using version
> 1.2.0-prerelease. When I go into fullscreen mode, it does seem to work,
> however the content is scaled down half of what it should be. The Peak has a
> higher resolution, and if I run it as an app it is scaled correctly (twice
> as dense).
> 
> You can run it here: http://webfighter.paas.allizom.org/.
> 
> Not sure if this is just a Peak bug or something real in Gaia.

Is that with this patch applied? And if so, do you get the same behaviour without the patch?
(In reply to Chris Lord [:cwiiis] from comment #102)
> 
> Is that with this patch applied? And if so, do you get the same behaviour
> without the patch?

No, it is without the patch. I don't the environment to apply the patch. I thought I'd just mention it, and possibly someone with a Peak could try it with the patch.
Comment on attachment 778501 [details] [diff] [review]
Fix-ups on top of attachment #770195 [details] [diff] [review]

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

Comment looks fine to me.
Attachment #778501 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #97)
> Ok, I agree that the transform applied in TransformScrollableLayer is never
> used on the B2G side. But does shadow->SetShadowTransform still need to be
> called at all (even if it's just with the unmodified pre-existing transform)?

No.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #98)
> Ok, this makes sense to me now. I think my problem was that I was thinking
> of transforms backwards. Given a chain of transforms from an ancestor layer
> S to a descendant layer T, applying the transforms to a layer X will move X
> from being relative to T to be relative to S. I was thinking it would move X
> from being relative to S to being relative to T, which is incorrect. Thanks
> for explanation!

If you have any suggestions for making this code easier to read, I'd love to hear them!
Whiteboard: leorun3 → leorun3 [LeoVB+]
Do we want to leave this open until it's fixed in b2g18?
(In reply to Chris Lord [:cwiiis] from comment #109)
> Do we want to leave this open until it's fixed in b2g18?

Nope. This is blocking-, so we don't need to patch this for b2g18.
Whiteboard: leorun3 [LeoVB+] → leorun3
Whiteboard: leorun3 → leorun3 [LeoVB+]
change LeoVB+ to LeoVB-
it means backout from leo repository by bug 891142
Whiteboard: leorun3 [LeoVB+] → leorun3 [LeoVB-]
Dear Mozilla colleagues, mark leo? since the patch of this PR need by v1.1
blocking-b2g: - → leo?
It's too late to fix this in 1.1 release. It's already fixed in 1.2 release, so will ship already in that release.
blocking-b2g: leo? → -
Whiteboard: leorun3 [LeoVB-] → leorun3 [LeoVB-], burirun3
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: