Closed
Bug 1204084
Opened 9 years ago
Closed 9 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)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: rbarker, Assigned: rbarker)
References
Details
Attachments
(1 file, 6 obsolete files)
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 | ||
Updated•9 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 1•9 years ago
|
||
This patch addresses the issue by clipping the returned element bounding rect to the size of the containing scrollable element.
Assignee | ||
Updated•9 years ago
|
Attachment #8660089 -
Flags: review?(botond)
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8660089 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
Updated to address review comments. Carry forward r+ from :botond
Attachment #8660878 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
Updated to fix case where long narrow div that is not scrollable can still be zoomed.
Assignee | ||
Updated•9 years ago
|
Attachment #8660888 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8661299 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ed18bc17a77
Assignee | ||
Updated•9 years ago
|
Attachment #8661322 -
Flags: review?(botond)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8661322 -
Attachment is obsolete: true
Attachment #8661322 -
Flags: review?(botond)
Assignee | ||
Updated•9 years ago
|
Attachment #8661915 -
Flags: review?(botond)
Comment 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
Address review comments carry forward r+ from :botond
Attachment #8661915 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a404231c514a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a404231c514a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•