- Revision
- 217870
- Author
- [email protected]
- Date
- 2017-06-06 19:17:27 -0700 (Tue, 06 Jun 2017)
Log Message
Crash trying to drag the entire text of a long book out of Mail compose view
https://bugs.webkit.org/show_bug.cgi?id=173042
<rdar://problem/32382059>
Reviewed by Simon Fraser.
Source/WebCore:
Creating a TextIndicator that is hundreds of thousands of pixels tall
is not a good idea. We introduced TextIndicatorOptionDoNotClipToVisibleRect
so that (for example) an image that overhangs the edge of the page would
contain the entire image, instead of just the visible fragment.
Instead, rename this option to ExpandClipBeyondVisibleRect, and make it
not unclip *entirely*, but expand the clip significantly (50% in each direction)
beyond the bounds of the current visible rect. Also, make use of exposed
area information for this clipping; otherwise, clients with very large views
inside scrollable areas (like Mail) would still try to create large TextIndicators.
* page/TextIndicator.cpp:
(WebCore::initializeIndicator):
* page/TextIndicator.h:
Source/WebKit2:
* WebProcess/WebPage/ios/FindControllerIOS.mm:
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::didConcludeEditDataInteraction):
Adapt to the new name.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (217869 => 217870)
--- trunk/Source/WebCore/ChangeLog 2017-06-07 00:28:47 UTC (rev 217869)
+++ trunk/Source/WebCore/ChangeLog 2017-06-07 02:17:27 UTC (rev 217870)
@@ -1,3 +1,25 @@
+2017-06-06 Tim Horton <[email protected]>
+
+ Crash trying to drag the entire text of a long book out of Mail compose view
+ https://bugs.webkit.org/show_bug.cgi?id=173042
+ <rdar://problem/32382059>
+
+ Reviewed by Simon Fraser.
+
+ Creating a TextIndicator that is hundreds of thousands of pixels tall
+ is not a good idea. We introduced TextIndicatorOptionDoNotClipToVisibleRect
+ so that (for example) an image that overhangs the edge of the page would
+ contain the entire image, instead of just the visible fragment.
+ Instead, rename this option to ExpandClipBeyondVisibleRect, and make it
+ not unclip *entirely*, but expand the clip significantly (50% in each direction)
+ beyond the bounds of the current visible rect. Also, make use of exposed
+ area information for this clipping; otherwise, clients with very large views
+ inside scrollable areas (like Mail) would still try to create large TextIndicators.
+
+ * page/TextIndicator.cpp:
+ (WebCore::initializeIndicator):
+ * page/TextIndicator.h:
+
2017-06-06 Chris Dumez <[email protected]>
RELEASE_ASSERT(static_cast<size_t>(enumerationValue) < WTF_ARRAY_LENGTH(values)) hit in convertEnumerationToJS<WebCore::History::ScrollRestoration>()
Modified: trunk/Source/WebCore/page/TextIndicator.cpp (217869 => 217870)
--- trunk/Source/WebCore/page/TextIndicator.cpp 2017-06-07 00:28:47 UTC (rev 217869)
+++ trunk/Source/WebCore/page/TextIndicator.cpp 2017-06-07 02:17:27 UTC (rev 217870)
@@ -272,34 +272,42 @@
else if (data.options & TextIndicatorOptionUseSelectionRectForSizing)
getSelectionRectsForRange(textRects, range);
#endif
- else {
- if (data.options & TextIndicatorOptionDoNotClipToVisibleRect)
- frame.selection().getTextRectangles(textRects, textRectHeight);
- else
- frame.selection().getClippedVisibleTextRectangles(textRects, textRectHeight);
- }
+ else
+ frame.selection().getTextRectangles(textRects, textRectHeight);
- if (textRects.isEmpty()) {
- RenderView* renderView = frame.contentRenderer();
- if (!renderView)
- return false;
- FloatRect boundingRect = range.absoluteBoundingRect();
- if (data.options & TextIndicatorOptionDoNotClipToVisibleRect)
- textRects.append(boundingRect);
- else {
- // Clip to the visible rect, just like getClippedVisibleTextRectangles does.
- // FIXME: We really want to clip to the unobscured rect in both cases, I think.
- // (this seems to work on Mac, but maybe not iOS?)
- FloatRect visibleContentRect = frame.view()->visibleContentRect(ScrollableArea::LegacyIOSDocumentVisibleRect);
- textRects.append(intersection(visibleContentRect, boundingRect));
- }
+ if (textRects.isEmpty())
+ textRects.append(range.absoluteBoundingRect());
+
+ auto frameView = frame.view();
+
+ // Use the exposedContentRect/viewExposedRect instead of visibleContentRect to avoid creating a huge indicator for a large view inside a scroll view.
+ IntRect contentsClipRect;
+#if PLATFORM(IOS)
+ contentsClipRect = enclosingIntRect(frameView->exposedContentRect());
+#else
+ if (auto viewExposedRect = frameView->viewExposedRect())
+ contentsClipRect = frameView->viewToContents(enclosingIntRect(*viewExposedRect));
+ else
+ contentsClipRect = frameView->visibleContentRect();
+#endif
+
+ if (data.options & TextIndicatorOptionExpandClipBeyondVisibleRect) {
+ contentsClipRect.inflateX(contentsClipRect.width() / 2);
+ contentsClipRect.inflateY(contentsClipRect.height() / 2);
}
FloatRect textBoundingRectInRootViewCoordinates;
FloatRect textBoundingRectInDocumentCoordinates;
+ Vector<FloatRect> clippedTextRectsInDocumentCoordinates;
Vector<FloatRect> textRectsInRootViewCoordinates;
for (const FloatRect& textRect : textRects) {
- FloatRect textRectInDocumentCoordinatesIncludingMargin = textRect;
+ FloatRect clippedTextRect = intersection(textRect, contentsClipRect);
+ if (clippedTextRect.isEmpty())
+ continue;
+
+ clippedTextRectsInDocumentCoordinates.append(clippedTextRect);
+
+ FloatRect textRectInDocumentCoordinatesIncludingMargin = clippedTextRect;
textRectInDocumentCoordinatesIncludingMargin.inflateX(margin.width());
textRectInDocumentCoordinatesIncludingMargin.inflateY(margin.height());
textBoundingRectInDocumentCoordinates.unite(textRectInDocumentCoordinatesIncludingMargin);
@@ -321,7 +329,7 @@
data.textBoundingRectInRootViewCoordinates = textBoundingRectInRootViewCoordinates;
data.textRectsInBoundingRectCoordinates = textRectsInBoundingRectCoordinates;
- return takeSnapshots(data, frame, enclosingIntRect(textBoundingRectInDocumentCoordinates), textRects);
+ return takeSnapshots(data, frame, enclosingIntRect(textBoundingRectInDocumentCoordinates), clippedTextRectsInDocumentCoordinates);
}
} // namespace WebCore
Modified: trunk/Source/WebCore/page/TextIndicator.h (217869 => 217870)
--- trunk/Source/WebCore/page/TextIndicator.h 2017-06-07 00:28:47 UTC (rev 217869)
+++ trunk/Source/WebCore/page/TextIndicator.h 2017-06-07 02:17:27 UTC (rev 217870)
@@ -78,8 +78,8 @@
TextIndicatorOptionIncludeMarginIfRangeMatchesSelection = 1 << 6,
// By default, TextIndicator clips the indicated rects to the visible content rect.
- // If this option is set, do not clip the indicated rects.
- TextIndicatorOptionDoNotClipToVisibleRect = 1 << 7,
+ // If this option is set, expand the clip rect outward so that slightly offscreen content will be included.
+ TextIndicatorOptionExpandClipBeyondVisibleRect = 1 << 7,
// Include an additional snapshot of everything in view, with the exception of nodes within the currently selected range.
TextIndicatorOptionIncludeSnapshotOfAllVisibleContentWithoutSelection = 1 << 8,
Modified: trunk/Source/WebKit2/ChangeLog (217869 => 217870)
--- trunk/Source/WebKit2/ChangeLog 2017-06-07 00:28:47 UTC (rev 217869)
+++ trunk/Source/WebKit2/ChangeLog 2017-06-07 02:17:27 UTC (rev 217870)
@@ -1,3 +1,16 @@
+2017-06-06 Tim Horton <[email protected]>
+
+ Crash trying to drag the entire text of a long book out of Mail compose view
+ https://bugs.webkit.org/show_bug.cgi?id=173042
+ <rdar://problem/32382059>
+
+ Reviewed by Simon Fraser.
+
+ * WebProcess/WebPage/ios/FindControllerIOS.mm:
+ * WebProcess/WebPage/ios/WebPageIOS.mm:
+ (WebKit::WebPage::didConcludeEditDataInteraction):
+ Adapt to the new name.
+
2017-06-06 Chris Dumez <[email protected]>
RELEASE_ASSERT(static_cast<size_t>(enumerationValue) < WTF_ARRAY_LENGTH(values)) hit in convertEnumerationToJS<WebCore::History::ScrollRestoration>()
Modified: trunk/Source/WebKit2/WebProcess/WebPage/ios/FindControllerIOS.mm (217869 => 217870)
--- trunk/Source/WebKit2/WebProcess/WebPage/ios/FindControllerIOS.mm 2017-06-07 00:28:47 UTC (rev 217869)
+++ trunk/Source/WebKit2/WebProcess/WebPage/ios/FindControllerIOS.mm 2017-06-07 02:17:27 UTC (rev 217870)
@@ -50,7 +50,7 @@
const int totalHorizontalMargin = 2;
const int totalVerticalMargin = 1;
-const TextIndicatorOptions findTextIndicatorOptions = TextIndicatorOptionTightlyFitContent | TextIndicatorOptionIncludeMarginIfRangeMatchesSelection | TextIndicatorOptionDoNotClipToVisibleRect;
+const TextIndicatorOptions findTextIndicatorOptions = TextIndicatorOptionTightlyFitContent | TextIndicatorOptionIncludeMarginIfRangeMatchesSelection | TextIndicatorOptionExpandClipBeyondVisibleRect;
static Color highlightColor()
{
Modified: trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm (217869 => 217870)
--- trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm 2017-06-07 00:28:47 UTC (rev 217869)
+++ trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm 2017-06-07 02:17:27 UTC (rev 217870)
@@ -635,7 +635,7 @@
{
std::optional<TextIndicatorData> textIndicatorData;
- static auto defaultEditDataInteractionTextIndicatorOptions = TextIndicatorOptionIncludeSnapshotOfAllVisibleContentWithoutSelection | TextIndicatorOptionDoNotClipToVisibleRect | TextIndicatorOptionPaintAllContent | TextIndicatorOptionIncludeMarginIfRangeMatchesSelection | TextIndicatorOptionPaintBackgrounds | TextIndicatorOptionComputeEstimatedBackgroundColor| TextIndicatorOptionUseSelectionRectForSizing | TextIndicatorOptionIncludeSnapshotWithSelectionHighlight;
+ static auto defaultEditDataInteractionTextIndicatorOptions = TextIndicatorOptionIncludeSnapshotOfAllVisibleContentWithoutSelection | TextIndicatorOptionExpandClipBeyondVisibleRect | TextIndicatorOptionPaintAllContent | TextIndicatorOptionIncludeMarginIfRangeMatchesSelection | TextIndicatorOptionPaintBackgrounds | TextIndicatorOptionComputeEstimatedBackgroundColor| TextIndicatorOptionUseSelectionRectForSizing | TextIndicatorOptionIncludeSnapshotWithSelectionHighlight;
auto& frame = m_page->focusController().focusedOrMainFrame();
if (auto range = frame.selection().selection().toNormalizedRange()) {
if (auto textIndicator = TextIndicator::createWithRange(*range, defaultEditDataInteractionTextIndicatorOptions, TextIndicatorPresentationTransition::None, FloatSize()))
