1043644 - Things go wonky after zooming in

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect)

Description

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

Latest metro project branch build, if you go to CNN.com or any other website and zoom in, you won't be able to zoom out or pan horizontally.

Looks like the composition bounds get larger as you zoom in, and that screws things up.

Comment 1

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

Regression from bug 1000423.

Comment 2

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> Regression from bug 1000423.

I guess the claim in the comments that "mTransformScale is 1, module CSS transforms", is not true on Metro, after all.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> "mTransformScale is 1, module CSS transforms"

modulo*

Comment 4

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

So in this case there's only one APZC, and for it resolution == cumulative resolution. I'm quite reluctant to change this code again in production. Jim, is there a policy on landing metro-only fixes #ifdef'd onto the metro tree?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> So in this case there's only one APZC, and for it resolution == cumulative
> resolution. I'm quite reluctant to change this code again in production.
> Jim, is there a policy on landing metro-only fixes #ifdef'd onto the metro
> tree?

Feel free to land metro only fixes in the metro repo. In the off chance we do bring the metro browser back, I'd suggest it be best to land stuff you would be comfortable having mass-merged back into mc.

Comment 6

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

Attachment: Patch (option #1)

Comment 7

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

Attachment: Patch (option #2)

Comment 8

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

tn, I'm happy to land option #2 directly on the metro tree. Let me know if option #1 is better and worth going into m-c (like if it fixes other things).

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8462109 [details] [diff] [review]
Patch (option #1)

Things that we can do after this (leaving this here just so I remember, not because you have to do it): get rid of viewbounds altogether, it's the same as frame bounds, it was my mistake to think it would be different. Also we should be able to get rid of the first android ifdef, and maybe the second one (have to test).

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8462109 [details] [diff] [review]
Patch (option #1)

Just as a reminder to myself so I can find it later: this patch does bug 985992, comment 2.

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8462109 [details] [diff] [review]
Patch (option #1)

I forgot that you need to make the same change in two other places: nsLayoutUtils::CalculateRootCompositionSize and nsLayoutUtils::CalculateCompositionSizeForFrame

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: stop using view bounds

I did the first cleanup item. I'll leave the other two for now.

viewbounds are the same as framebounds, so we don't need them. If we are a root scroll frame we'll use widget bounds or content viewer bounds.

Comment 13

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

Attachment: Part 1 - Try using the contentviewer bounds

Updated to also modify the two functions in nsLayoutUtils.cpp

Comment 14

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

Attachment: Part 2 - Stop using view bounds (rebased)

tn, I left you as the author on this patch.

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8463362 [details] [diff] [review]
Part 1 - Try using the contentviewer bounds

Now that I read this with fresh eyes I think that we should be incorporating the resolution up to the parent before considering the content viewer bounds as layer pixels. (It'll be 1 right now, but maybe not in the future.)

You can do it as a separate patch on top of the first two if you'd like.

Comment 16

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

(In reply to Timothy Nikkel (:tn) from comment #15)
> Now that I read this with fresh eyes I think that we should be incorporating
> the resolution up to the parent before considering the content viewer bounds
> as layer pixels. (It'll be 1 right now, but maybe not in the future.)

Not sure which resolution you're talking about here.

Btw, try push from the first two parts is at https://tbpl.mozilla.org/?tree=Try&rev=00615a8ab506 - I fixed the build failures locally by moving frameSize into the ifdef.

Comment 17

[Timothy Nikkel (:tnikkel)]

I think that the content viewer bounds type is layout device pixels in the parent layer. So to convert them to layer pixels (where we do that) we need to multiply them by the cumulative resolution of the parent. Or am I thinking about it wrong?

Comment 18

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

Attachment: Part 1 - Try using the contentviewer bounds

That makes sense. I changed GetContentViewerBounds to take a LayoutDeviceIntRect to make it more clear and propagated some conversions.

Comment 19

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

Attachment: Part 2 - Stop using view bounds (rebased)

Part 2 rebased

Comment 20

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

New try push:

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

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8464089 [details] [diff] [review]
Part 1 - Try using the contentviewer bounds

>@@ -6643,7 +6671,13 @@ nsLayoutUtils::CalculateRootCompositionS
>           }
> #endif
>         } else {
>-          rootCompositionSize = viewSize;
>+          LayoutDeviceIntRect contentBounds;
>+          if (nsLayoutUtils::GetContentViewerBounds(rootPresContext, contentBounds)) {
>+            gfxSize res = rootPresContext->PresShell()->GetCumulativeResolution();
>+            rootCompositionSize = contentBounds.Size() * LayoutDeviceToLayerScale(res.width, res.height);
>+          } else {
>+            rootCompositionSize = viewSize;
>+          }
>         }
>       }
>     }

This should use the cumulative resolution of the parent of rootPresContext, if there is one, or just 1.0 if not.

Comment 22

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

Done and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdde5b95b064
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1b0d6b870c

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdde5b95b064
https://hg.mozilla.org/mozilla-central/rev/0f1b0d6b870c