1204932 - When C++APZ is enabled, elements that are zoomed with double tap should be centered

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Randall Barker [:rbarker]]

Currently, the C++APZ aligns the zoomed element from a double tap along the top of the viewport. If the height of the element is short enough, it would be better to center the element vertically. This helps on pages that have a fixed position menu bar so that the zoomed element is less likely to be covered by fixed elements at either the top or bottom of the page.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1204932-When-C-APZ-is-enabled-elements-that-are-zoomed-with-double-tap-should-be-centered-15091508-27ca1ce.patch

Comment 2

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=806ae5c372e2

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8661296 [details] [diff] [review]
0001-Bug-1204932-When-C-APZ-is-enabled-elements-that-are-zoomed-with-double-tap-should-be-centered-15091508-27ca1ce.patch

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

Thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3092,5 @@
>        aRect = aRect.Intersect(cssPageRect);
>        targetZoom = CSSToParentLayerScale(std::min(compositionBounds.width / aRect.width,
>                                                    compositionBounds.height / aRect.height));
>      }
>      // 1. If the rect is empty, request received from browserElementScrolling.js

Since you're touching this code anyways, could you update this comment? "browserElementScrolling.js" is something very old :)

  // 1. If the rect is empty, the content-side logic for handling a double-tap
  //    requested that we zoom out.

@@ +3134,5 @@
>        aRect.x = cssPageRect.width - sizeAfterZoom.width;
>        aRect.x = aRect.x > 0 ? aRect.x : 0;
>      }
>  
> +    // Center the zoomed element in the screen.

// ... vertically.

@@ +3136,5 @@
>      }
>  
> +    // Center the zoomed element in the screen.
> +    if (!zoomOut && (sizeAfterZoom.height > aRect.height)) {
> +      aRect.y = aRect.y - ((sizeAfterZoom.height - aRect.height) * 0.5f);

This can be written:

  aRect.y -= ...;

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1204932-When-C-APZ-is-enabled-elements-that-are-zoomed-with-double-tap-should-be-centered-15091610-1d6cde6.patch

Carry forward r+ from :botond

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a02adf61c013

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a02adf61c013