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

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

Trunk
mozilla43
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
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

Updated

4 years ago
Assignee: nobody → rbarker
Assignee

Updated

4 years ago
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+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a02adf61c013
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.