744939 - Make animatedZoomTo operate in CSS pixels instead of device pixels

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make animatedZoomTo operate in CSS pixels instead of device pixels

Working CSS pixels makes more sense. e.g When ever we zoom out to the full page we use the same width instead of a width relative to the current zoom.

Comment 1

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

Comment on attachment 614549 [details] [diff] [review]
Make animatedZoomTo operate in CSS pixels instead of device pixels

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

This.. doesn't seem right. The main thing is I fail to see how this will provide any benefit. A lot of the values you end up using are still zoom-transformed in some way, it's just a different set of values that are zoom-transformed. It seems to me that this will still result in slightly-variable zoom amounts but under different conditions.

::: mobile/android/chrome/content/browser.js
@@ -2487,2 @@
>  
>        let overlap = vRect.intersect(bRect);

Taking out this line alone seems wrong because now the intersect on the next line is intersecting the viewport (in device pixels) with the block rect (in CSS pixels) and the overlap calculation is garbage.

Comment 2

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

(In reply to Kartikaya Gupta (:kats) from comment #1)
> >  
> >        let overlap = vRect.intersect(bRect);
> 
> Taking out this line alone seems wrong because now the intersect on the next
> line is intersecting the viewport (in device pixels) with the block rect (in
> CSS pixels) and the overlap calculation is garbage.

Stupid splinter. The line I was referring to was the "bRect.scale(zoom, zoom);" that you took out.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make animatedZoomTo operate in CSS pixels instead of device pixels v2

This version takes a slightly more robust approach and operates entirely in css pixels.

Comment 4

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

Comment on attachment 615063 [details] [diff] [review]
Make animatedZoomTo operate in CSS pixels instead of device pixels v2

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

Better, but I still feel like there's multiplications and divisions that will mess things up. I would feel more comfortable if we first came up with a set of rules of when to use which coordinates systems and how to deal with rounding, and then used that to drive the code modifications.

::: mobile/android/base/ui/PanZoomController.java
@@ +183,3 @@
>  
>                  RectF viewableRect = mController.getViewport();
>                  float y = viewableRect.top;

y is in device pixels here, you want it in CSS pixels.

@@ +952,3 @@
>  
>          ViewportMetrics finalMetrics = new ViewportMetrics(mController.getViewportMetrics());
> +        finalMetrics.setOrigin(new PointF(zoomToRect.left*finalMetrics.getZoomFactor(), zoomToRect.top*finalMetrics.getZoomFactor()));

Again, it seems to me like doing this multiplication here may introduce rounding errors and I'm not fully happy with this.

::: mobile/android/chrome/content/browser.js
@@ +1735,4 @@
>  
>      let doc = this.browser.contentDocument;
>      if (doc != null) {
> +      let [pageWidth, pageHeight] = this.getPageSize(doc, viewport.cssWidth, viewport.cssHeight);

Why did you change this line to viewport.cssWidth and .cssHeight from .width and .height? (I'm not sure it's wrong, but I'm not sure it's right either)

Comment 5

[JP Rosevear [:jpr]]

Please do a complete risk assessment of all three pieces before landing, it seems like we could break a lot here.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 615063 [details] [diff] [review]
Make animatedZoomTo operate in CSS pixels instead of device pixels v2

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

::: mobile/android/chrome/content/browser.js
@@ +1735,4 @@
>  
>      let doc = this.browser.contentDocument;
>      if (doc != null) {
> +      let [pageWidth, pageHeight] = this.getPageSize(doc, viewport.cssWidth, viewport.cssHeight);

The code was wrong before. Reading the code below this hunk, it's clear that it assumed that pageWidth and pageHeight were in CSS pixels, since they're immediately transformed to device pixels using the zoom factor. I guess doc always has a .body/.documentElement.

Comment 7

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

(In reply to Joe Drew (:JOEDREW!) from comment #6)
> The code was wrong before. Reading the code below this hunk, it's clear that
> it assumed that pageWidth and pageHeight were in CSS pixels, since they're
> immediately transformed to device pixels using the zoom factor. I guess doc
> always has a .body/.documentElement.

Sort of. Yes, it is assumed that the pageWidth and pageHeight that come out of getPageSize are in CSS pixels. However, also note that the thing that is returned from getPageSize is zoom-independent. That is, you could be viewing the page at 1.0 or 4.0 zoom, and getPageSize would return the same thing. In order to maintain that invariant, the default values passed in also need to be zoom-independent. Passing in viewport.cssWidth (which will vary depending on your zoom) violates this.

The scenario where this affects behaviour is when the page hasn't loaded yet. If we use viewport.width and viewport.height as it is now, then you'll basically see a white "page" which is the size of the screen when you are at zoom 1.0. You can zoom in and out, and the white "page" will shrink and expand as the user expects.

However, if you pass in viewport.cssWidth and viewport.cssHeight, that behaviour changes. Instead of being able to make the "page" shrink and expand like a normal page, it will stay roughly screen-sized no matter what the user is doing. Whether or not this change in behaviour is desirable (or even if anybody cares) I don't know.

Comment 8

[Joe Drew (not getting mail)]

Attachment: Make animatedZoomTo operate in CSS pixels instead of device pixels v3

This is working now. I think we should let the behaviour change go, and see what happens.

Comment 9

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

Comment on attachment 616810 [details] [diff] [review]
Make animatedZoomTo operate in CSS pixels instead of device pixels v3

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

Looks good; a few nits but nothing serious enough to block landing.

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +74,5 @@
> +        RectF cssViewport = getViewport();
> +        cssViewport.bottom /= zoomFactor;
> +        cssViewport.top /= zoomFactor;
> +        cssViewport.left /= zoomFactor;
> +        cssViewport.right /= zoomFactor;

RectUtils.scale is your friend.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +126,5 @@
> +        RectF cssViewport = new RectF(mViewportRect);
> +        cssViewport.bottom /= mZoomFactor;
> +        cssViewport.top /= mZoomFactor;
> +        cssViewport.left /= mZoomFactor;
> +        cssViewport.right /= mZoomFactor;

RectUtils.scale really wants to be your friend.

::: mobile/android/base/ui/PanZoomController.java
@@ +978,5 @@
>          GeckoEvent e = GeckoEvent.createBroadcastEvent("Gesture:CancelTouch", "");
>          GeckoAppShell.sendEventToGecko(e);
>      }
>  
> +    // Zoom to a specified rect in CSS pixels.

Turn this into a javadoc-style comment, starting with /**, maybe use ALL CAPS to emphasize the CSS pixels part of it. Pretty much everything in Java-land is done in device pixels so this exception should be pointed out clearly.

@@ +1011,3 @@
>  
>          ViewportMetrics finalMetrics = new ViewportMetrics(mController.getViewportMetrics());
> +        finalMetrics.setOrigin(new PointF(zoomToRect.left*finalMetrics.getZoomFactor(), zoomToRect.top*finalMetrics.getZoomFactor()));

Add some spaces around the * operators, maybe split this into two lines.

Comment 10

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1d0f6ebe70

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3a1d0f6ebe70