Closed Bug 1204932 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Panning and Zooming, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → rbarker
Attachment #8661296 - Flags: review?(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 -= ...;
Attachment #8661296 - Flags: review?(botond) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a02adf61c013
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.