Closed Bug 1204084 Opened 4 years ago Closed 4 years ago

Double tap gesture fails when tapping on a large element contained in an iframe or scrollable div

Categories

(Core :: Panning and Zooming, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

Details

Attachments

(1 file, 6 obsolete files)

4.83 KB, patch
Details | Diff | Splinter Review
When calculating the rect to zoom to in GetBoundingContentRect in DoubleTapToZoom.cpp, it does not take into account the size of the containing scrollable frame. Thus if the tapped element in the scrollable element is very large, the returned bounding rect is not clipped and will trip up the IsRectZoomedIn function because the bounding rect will appear to be much larger than the composited area even though the actual area visible is much smaller.
Assignee: nobody → rbarker
This patch addresses the issue by clipping the returned element bounding rect to the size of the containing scrollable element.
Attachment #8660089 - Flags: review?(botond)
Comment on attachment 8660089 [details] [diff] [review]
0001-Bug-1204084-Double-tap-gesture-fails-when-tapping-on-a-large-element-contained-in-an-iframe-or-scrollable-div-15091113-6c2d352.patch

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

::: gfx/layers/apz/util/DoubleTapToZoom.cpp
@@ +86,5 @@
>              aShell->GetRootFrame(),
>              nsLayoutUtils::RECTS_ACCOUNT_FOR_TRANSFORMS)
>        + aRootScrollFrame->GetScrollPosition());
> +
> +    nsIFrame* primaryFrame = aElement->GetPrimaryFrame();

We already have |aElement->GetPrimaryFrame()|, null-checked, in |frame| - just use that.

@@ +91,5 @@
> +    if (primaryFrame) {
> +      nsIScrollableFrame* scrollFrame = nsLayoutUtils::GetNearestScrollableFrame(primaryFrame);
> +      if (scrollFrame && scrollFrame != aRootScrollFrame) {
> +        FrameMetrics metrics = nsLayoutUtils::CalculateBasicFrameMetrics(scrollFrame);
> +        CSSRect area = CSSRect(metrics.GetScrollOffset(), metrics.CalculateCompositedSizeInCssPixels());

s/area/scrollFrameBounds

@@ +93,5 @@
> +      if (scrollFrame && scrollFrame != aRootScrollFrame) {
> +        FrameMetrics metrics = nsLayoutUtils::CalculateBasicFrameMetrics(scrollFrame);
> +        CSSRect area = CSSRect(metrics.GetScrollOffset(), metrics.CalculateCompositedSizeInCssPixels());
> +        result.width = std::min(result.width, area.width);
> +        result.height = std::min(result.height, area.height);

I don't think clamping the width and height, while leaving the origin unchanged, is correct. 

Consider a case where the element bounds are (0, -1000, 200, 2000), while the scroll frame's bounds are (0, 0, 200, 200).

The current code would make the result (0, -1000, 200, 200), whereas I think we'd want it to be (0, 0, 200, 200).

I think the correct operation here is intersecting the element bounds and the scroll frame bounds.
Attachment #8660089 - Flags: review?(botond)
Comment on attachment 8660878 [details] [diff] [review]
0001-Bug-1204084-Double-tap-gesture-fails-when-tapping-on-a-large-element-contained-in-an-iframe-or-scrollable-div-15091412-ab15025.patch

Updated to correctly use bounding rect of scrollable frame.
Attachment #8660878 - Flags: review?(botond)
Comment on attachment 8660878 [details] [diff] [review]
0001-Bug-1204084-Double-tap-gesture-fails-when-tapping-on-a-large-element-contained-in-an-iframe-or-scrollable-div-15091412-ab15025.patch

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

Thanks!

::: gfx/layers/apz/util/DoubleTapToZoom.cpp
@@ +93,5 @@
> +    nsIScrollableFrame* scrollFrame = nsLayoutUtils::GetNearestScrollableFrame(frame);
> +    if (scrollFrame && scrollFrame != aRootScrollFrame) {
> +      nsIFrame* subFrame = do_QueryFrame(scrollFrame);
> +      if (subFrame) {
> +        CSSRect subFrameRect = CSSRect::FromAppUnits(

// Get the bounds of the scroll frame in the same coordinate space
// as |result|.

@@ +104,5 @@
> +        result = subFrameRect.Intersect(result);
> +      }
> +    }
> +
> +    return result;

Since we're introducing the variable |result|, we might as well declare it at the very top, return it instead of |CSSRect()| at the bottom, and omit this return.
Attachment #8660878 - Flags: review?(botond) → review+
Updated to address review comments. Carry forward r+ from :botond
Attachment #8660878 - Attachment is obsolete: true
Updated to fix case where long narrow div that is not scrollable can still be zoomed.
Attachment #8660888 - Attachment is obsolete: true
Attachment #8661322 - Flags: review?(botond)
Attachment #8661915 - Flags: review?(botond)
Comment on attachment 8661915 [details] [diff] [review]
0001-Bug-1204084-Double-tap-gesture-fails-when-tapping-on-a-large-element-contained-in-an-iframe-or-scrollable-div-15091610-1c59b2f.patch

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

::: gfx/layers/apz/util/DoubleTapToZoom.cpp
@@ +73,5 @@
>  // Calculate the bounding rect of |aElement|, relative to the origin
>  // of the document associated with |aShell|.
>  // |aRootScrollFrame| should be the root scroll frame of the document in
>  // question.
>  // The implementation is adapted from Element::GetBoundingClientRect().

Please update this comment. Suggestion:

// Calculate the bounding rect of |aElement|, relative to the origin
// of the scrolled content of |aRootScrollFrame|.
// The implementation of this calculation is adapted from 
// Element::GetBoundingClientRect().
//
// Where the element is contained inside a scrollable subframe, the
// bounding rect is clipped to the bounds of the subframe.

@@ +99,5 @@
> +      CSSRect subFrameRect = CSSRect::FromAppUnits(
> +          nsLayoutUtils::TransformFrameRectToAncestor(
> +              subFrame,
> +              subFrame->GetRectRelativeToSelf(),
> +              aRootScrollFrame->GetScrolledFrame()));

Maybe extract the two uses of |aRootScrollFrame->GetScrolledFrame()| out into a variable |relativeTo|?

@@ +162,5 @@
>    CSSRect compositedArea(metrics.GetScrollOffset(), metrics.CalculateCompositedSizeInCssPixels());
>    const CSSCoord margin = 15;
> +  CSSRect rect = GetBoundingContentRect(element, rootScrollFrame);
> +
> +  // If the element is taller than the visible page scale the hight of

s/visible page/visible area of the page
s/hight/height

@@ +173,5 @@
> +    if (widthRatio < 0.9 && targetHeight < rect.height) {
> +      const CSSPoint scrollPoint = CSSPoint::FromAppUnits(rootScrollFrame->GetScrollPosition());
> +      float newY = aPoint.y + scrollPoint.y - (targetHeight * 0.5f);
> +      if ((newY + targetHeight) > (rect.y + rect.height)) {
> +        rect.y = rect.y + rect.height - targetHeight;

rect.y += ...;
Attachment #8661915 - Flags: review?(botond) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a404231c514a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.