Drag and Drop preview image for Twitter link is the wrong shape
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jul 2017 03:01:51 +0000 (03:01 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jul 2017 03:01:51 +0000 (03:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174731
<rdar://problem/33335616>

Reviewed by Zalan Bujtas.

TextIndicator uses Range::borderAndTextQuads and ::absoluteTextRects
in order to get the rects of the indicated text. Currently, these
functions do not respect clipping, so clipped-out text (e.g. as seen
inside links on Twitter) generates lots of meaningless indicated rects.

* page/TextIndicator.cpp:
(WebCore::estimatedBackgroundColorForRange):
(WebCore::hasAnyIllegibleColors):
Change adjustTextIndicatorDataOptionsForEstimatedColorsIfNecessary
to instead be hasAnyIllegibleColors, and referred to in the same place
as hasNonInlineOrReplacedElements, so that it follows the same
upgrade path (leaving textRects empty, so that it is later filled in
with the absoluteBoundingRect). This was a mistake in r219033, which
instead would end up painting all content, but filling in textRects
with the actual individual text rects.

This alone changes the behavior on Twitter from lots of jagged misplaced
rects to a too-large bounding rect. Combined with the following changes,
the bounding rect is reduced to the right size:

(WebCore::initializeIndicator):
Adopt the new Range::borderAndTextQuads and ::absoluteTextRects parameter
and opt-in to respecting clipping for text rects.

* dom/DOMRectList.cpp:
(WebCore::DOMRectList::DOMRectList):
* dom/DOMRectList.h:
(WebCore::DOMRectList::create):
Add a DOMRectList constructor and create() that take FloatRects, similar
to the one that takes FloatQuads, but without the boundingRect() calls.

* dom/Document.h:
* dom/Document.cpp:
(WebCore::Document::convertAbsoluteToClientRects):
Add convertAbsoluteToClientRects, similar to covertAbsoluteToClientQuads,
except acting on rects instead of quads.

* dom/Range.cpp:
(WebCore::Range::absoluteRectsForRangeInText):
(WebCore::Range::absoluteTextRects):
(WebCore::Range::getClientRects):
(WebCore::Range::borderAndTextRects):
(WebCore::Range::boundingRect):
(WebCore::Range::absoluteBoundingRect):
(WebCore::Range::borderAndTextQuads): Deleted.
* dom/Range.h:
Replace borderAndTextQuads with borderAndTextRects, because all callers
just ended up calling boundingBox() on the quads.

Factor absoluteRectsForRangeInText out of absoluteTextRects and
borderAndTextQuads, and teach it to optionally intersect the text rects
with their renderer's absoluteClippedOverflowRect.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219756 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/DOMRectList.cpp
Source/WebCore/dom/DOMRectList.h
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/dom/Range.cpp
Source/WebCore/dom/Range.h
Source/WebCore/page/TextIndicator.cpp

index 11708e7..2facacf 100644 (file)
@@ -1,3 +1,64 @@
+2017-07-21  Timothy Horton  <timothy_horton@apple.com>
+
+        Drag and Drop preview image for Twitter link is the wrong shape
+        https://bugs.webkit.org/show_bug.cgi?id=174731
+        <rdar://problem/33335616>
+
+        Reviewed by Zalan Bujtas.
+
+        TextIndicator uses Range::borderAndTextQuads and ::absoluteTextRects
+        in order to get the rects of the indicated text. Currently, these
+        functions do not respect clipping, so clipped-out text (e.g. as seen
+        inside links on Twitter) generates lots of meaningless indicated rects.
+
+        * page/TextIndicator.cpp:
+        (WebCore::estimatedBackgroundColorForRange):
+        (WebCore::hasAnyIllegibleColors):
+        Change adjustTextIndicatorDataOptionsForEstimatedColorsIfNecessary
+        to instead be hasAnyIllegibleColors, and referred to in the same place
+        as hasNonInlineOrReplacedElements, so that it follows the same
+        upgrade path (leaving textRects empty, so that it is later filled in
+        with the absoluteBoundingRect). This was a mistake in r219033, which
+        instead would end up painting all content, but filling in textRects
+        with the actual individual text rects.
+
+        This alone changes the behavior on Twitter from lots of jagged misplaced
+        rects to a too-large bounding rect. Combined with the following changes,
+        the bounding rect is reduced to the right size:
+
+        (WebCore::initializeIndicator):
+        Adopt the new Range::borderAndTextQuads and ::absoluteTextRects parameter
+        and opt-in to respecting clipping for text rects.
+
+        * dom/DOMRectList.cpp:
+        (WebCore::DOMRectList::DOMRectList):
+        * dom/DOMRectList.h:
+        (WebCore::DOMRectList::create):
+        Add a DOMRectList constructor and create() that take FloatRects, similar
+        to the one that takes FloatQuads, but without the boundingRect() calls.
+
+        * dom/Document.h:
+        * dom/Document.cpp:
+        (WebCore::Document::convertAbsoluteToClientRects):
+        Add convertAbsoluteToClientRects, similar to covertAbsoluteToClientQuads,
+        except acting on rects instead of quads.
+
+        * dom/Range.cpp:
+        (WebCore::Range::absoluteRectsForRangeInText):
+        (WebCore::Range::absoluteTextRects):
+        (WebCore::Range::getClientRects):
+        (WebCore::Range::borderAndTextRects):
+        (WebCore::Range::boundingRect):
+        (WebCore::Range::absoluteBoundingRect):
+        (WebCore::Range::borderAndTextQuads): Deleted.
+        * dom/Range.h:
+        Replace borderAndTextQuads with borderAndTextRects, because all callers
+        just ended up calling boundingBox() on the quads.
+
+        Factor absoluteRectsForRangeInText out of absoluteTextRects and
+        borderAndTextQuads, and teach it to optionally intersect the text rects
+        with their renderer's absoluteClippedOverflowRect.
+
 2017-07-21  Per Arne Vollan  <pvollan@apple.com>
 
         Round-tripping stroke-width styles through getComputedStyle cause the text to gain a stroke.
index 2d33d29..36b99d1 100644 (file)
@@ -37,6 +37,13 @@ DOMRectList::DOMRectList(const Vector<FloatQuad>& quads)
         m_items.uncheckedAppend(DOMRect::create(quad.boundingBox()));
 }
 
+DOMRectList::DOMRectList(const Vector<FloatRect>& rects)
+{
+    m_items.reserveInitialCapacity(rects.size());
+    for (auto& rect : rects)
+        m_items.uncheckedAppend(DOMRect::create(rect));
+}
+
 DOMRectList::~DOMRectList()
 {
 }
index 4e46ab3..abae681 100644 (file)
@@ -36,6 +36,7 @@ class DOMRect;
 class DOMRectList : public RefCounted<DOMRectList> {
 public:
     static Ref<DOMRectList> create(const Vector<FloatQuad>& quads) { return adoptRef(*new DOMRectList(quads)); }
+    static Ref<DOMRectList> create(const Vector<FloatRect>& rects) { return adoptRef(*new DOMRectList(rects)); }
     static Ref<DOMRectList> create() { return adoptRef(*new DOMRectList()); }
     WEBCORE_EXPORT ~DOMRectList();
 
@@ -44,6 +45,7 @@ public:
 
 private:
     WEBCORE_EXPORT explicit DOMRectList(const Vector<FloatQuad>& quads);
+    WEBCORE_EXPORT explicit DOMRectList(const Vector<FloatRect>& rects);
     DOMRectList() = default;
 
     Vector<Ref<DOMRect>> m_items;
index 68e4c33..13fc77e 100644 (file)
@@ -6554,6 +6554,23 @@ void Document::convertAbsoluteToClientQuads(Vector<FloatQuad>& quads, const Rend
         quad.move(documentToClientOffset);
     }
 }
+    
+void Document::convertAbsoluteToClientRects(Vector<FloatRect>& rects, const RenderStyle& style)
+{
+    if (!view())
+        return;
+    
+    auto& frameView = *view();
+    float inverseFrameScale = frameView.absoluteToDocumentScaleFactor(style.effectiveZoom());
+    auto documentToClientOffset = frameView.documentToClientOffset();
+    
+    for (auto& rect : rects) {
+        if (inverseFrameScale != 1)
+            rect.scale(inverseFrameScale);
+        
+        rect.move(documentToClientOffset);
+    }
+}
 
 void Document::convertAbsoluteToClientRect(FloatRect& rect, const RenderStyle& style)
 {
index 046ba0d..46ad0c8 100644 (file)
@@ -1224,6 +1224,7 @@ public:
 #endif
 
     void convertAbsoluteToClientQuads(Vector<FloatQuad>&, const RenderStyle&);
+    void convertAbsoluteToClientRects(Vector<FloatRect>&, const RenderStyle&);
     void convertAbsoluteToClientRect(FloatRect&, const RenderStyle&);
 
     bool hasActiveParser();
index 2748aac..865a54b 100644 (file)
@@ -1166,8 +1166,38 @@ IntRect Range::absoluteBoundingBox() const
     return result;
 }
 
-void Range::absoluteTextRects(Vector<IntRect>& rects, bool useSelectionHeight, RangeInFixedPosition* inFixed) const
+Vector<FloatRect> Range::absoluteRectsForRangeInText(Node* node, RenderText& renderText, bool useSelectionHeight, bool& isFixed, RespectClippingForTextRects respectClippingForTextRects) const
 {
+    unsigned startOffset = node == &startContainer() ? m_start.offset() : 0;
+    unsigned endOffset = node == &endContainer() ? m_end.offset() : std::numeric_limits<unsigned>::max();
+
+    auto textQuads = renderText.absoluteQuadsForRange(startOffset, endOffset, useSelectionHeight, &isFixed);
+
+    if (respectClippingForTextRects == RespectClippingForTextRects::Yes) {
+        Vector<FloatRect> clippedRects;
+
+        auto absoluteClippedOverflowRect = renderText.absoluteClippedOverflowRect();
+
+        for (auto& quad : textQuads) {
+            auto clippedRect = intersection(quad.boundingBox(), absoluteClippedOverflowRect);
+            if (!clippedRect.isEmpty())
+                clippedRects.append(clippedRect);
+        }
+
+        return clippedRects;
+    }
+
+    Vector<FloatRect> floatRects;
+    floatRects.reserveInitialCapacity(textQuads.size());
+    for (auto& quad : textQuads)
+        floatRects.append(quad.boundingBox());
+    return floatRects;
+}
+
+void Range::absoluteTextRects(Vector<IntRect>& rects, bool useSelectionHeight, RangeInFixedPosition* inFixed, RespectClippingForTextRects respectClippingForTextRects) const
+{
+    // FIXME: This function should probably return FloatRects.
+
     bool allFixed = true;
     bool someFixed = false;
 
@@ -1180,15 +1210,15 @@ void Range::absoluteTextRects(Vector<IntRect>& rects, bool useSelectionHeight, R
         if (renderer->isBR())
             renderer->absoluteRects(rects, flooredLayoutPoint(renderer->localToAbsolute()));
         else if (is<RenderText>(*renderer)) {
-            unsigned startOffset = node == &startContainer() ? m_start.offset() : 0;
-            unsigned endOffset = node == &endContainer() ? m_end.offset() : std::numeric_limits<unsigned>::max();
-            rects.appendVector(downcast<RenderText>(*renderer).absoluteRectsForRange(startOffset, endOffset, useSelectionHeight, &isFixed));
+            auto rectsForRenderer = absoluteRectsForRangeInText(node, downcast<RenderText>(*renderer), useSelectionHeight, isFixed, respectClippingForTextRects);
+            for (auto& rect : rectsForRenderer)
+                rects.append(enclosingIntRect(rect));
         } else
             continue;
         allFixed &= isFixed;
         someFixed |= isFixed;
     }
-    
+
     if (inFixed)
         *inFixed = allFixed ? EntirelyFixedPosition : (someFixed ? PartiallyFixedPosition : NotFixedPosition);
 }
@@ -1765,7 +1795,7 @@ ExceptionOr<void> Range::expand(const String& unit)
 
 Ref<DOMRectList> Range::getClientRects() const
 {
-    return DOMRectList::create(borderAndTextQuads(CoordinateSpace::Client));
+    return DOMRectList::create(borderAndTextRects(CoordinateSpace::Client));
 }
 
 Ref<DOMRect> Range::getBoundingClientRect() const
@@ -1773,9 +1803,9 @@ Ref<DOMRect> Range::getBoundingClientRect() const
     return DOMRect::create(boundingRect(CoordinateSpace::Client));
 }
 
-Vector<FloatQuad> Range::borderAndTextQuads(CoordinateSpace space) const
+Vector<FloatRect> Range::borderAndTextRects(CoordinateSpace space, RespectClippingForTextRects respectClippingForTextRects) const
 {
-    Vector<FloatQuad> quads;
+    Vector<FloatRect> rects;
 
     ownerDocument().updateLayoutIgnorePendingStylesheets();
 
@@ -1799,36 +1829,38 @@ Vector<FloatQuad> Range::borderAndTextQuads(CoordinateSpace space) const
                 renderer->absoluteQuads(elementQuads);
                 if (space == CoordinateSpace::Client)
                     node->document().convertAbsoluteToClientQuads(elementQuads, renderer->style());
-                quads.appendVector(elementQuads);
+                
+                for (auto& quad : elementQuads)
+                    rects.append(quad.boundingBox());
             }
         } else if (is<Text>(*node)) {
             if (auto* renderer = downcast<Text>(*node).renderer()) {
-                unsigned startOffset = node == &startContainer() ? m_start.offset() : 0;
-                unsigned endOffset = node == &endContainer() ? m_end.offset() : std::numeric_limits<unsigned>::max();
-                auto textQuads = renderer->absoluteQuadsForRange(startOffset, endOffset);
+                bool isFixed;
+                auto clippedRects = absoluteRectsForRangeInText(node, *renderer, false, isFixed, respectClippingForTextRects);
                 if (space == CoordinateSpace::Client)
-                    node->document().convertAbsoluteToClientQuads(textQuads, renderer->style());
-                quads.appendVector(textQuads);
+                    node->document().convertAbsoluteToClientRects(clippedRects, renderer->style());
+
+                rects.appendVector(clippedRects);
             }
         }
     }
 
-    return quads;
+    return rects;
 }
 
-FloatRect Range::boundingRect(CoordinateSpace space) const
+FloatRect Range::boundingRect(CoordinateSpace space, RespectClippingForTextRects respectClippingForTextRects) const
 {
     FloatRect result;
-    for (auto& quad : borderAndTextQuads(space))
-        result.unite(quad.boundingBox());
+    for (auto& rect : borderAndTextRects(space, respectClippingForTextRects))
+        result.unite(rect);
     return result;
 }
 
-FloatRect Range::absoluteBoundingRect() const
+FloatRect Range::absoluteBoundingRect(RespectClippingForTextRects respectClippingForTextRects) const
 {
-    return boundingRect(CoordinateSpace::Absolute);
+    return boundingRect(CoordinateSpace::Absolute, respectClippingForTextRects);
 }
-    
+
 TextStream& operator<<(TextStream& ts, const RangeBoundaryPoint& r)
 {
     return ts << r.toPosition();
index 1a5d4a1..f43db39 100644 (file)
@@ -41,6 +41,7 @@ class DocumentFragment;
 class FloatQuad;
 class Node;
 class NodeWithIndex;
+class RenderText;
 class SelectionRect;
 class Text;
 class VisiblePosition;
@@ -116,12 +117,13 @@ public:
     };
 
     // Not transform-friendly
-    WEBCORE_EXPORT void absoluteTextRects(Vector<IntRect>&, bool useSelectionHeight = false, RangeInFixedPosition* = nullptr) const;
+    enum class RespectClippingForTextRects { No, Yes };
+    WEBCORE_EXPORT void absoluteTextRects(Vector<IntRect>&, bool useSelectionHeight = false, RangeInFixedPosition* = nullptr, RespectClippingForTextRects = RespectClippingForTextRects::No) const;
     WEBCORE_EXPORT IntRect absoluteBoundingBox() const;
 
     // Transform-friendly
     WEBCORE_EXPORT void absoluteTextQuads(Vector<FloatQuad>&, bool useSelectionHeight = false, RangeInFixedPosition* = nullptr) const;
-    WEBCORE_EXPORT FloatRect absoluteBoundingRect() const;
+    WEBCORE_EXPORT FloatRect absoluteBoundingRect(RespectClippingForTextRects = RespectClippingForTextRects::No) const;
 #if PLATFORM(IOS)
     WEBCORE_EXPORT void collectSelectionRects(Vector<SelectionRect>&) const;
     WEBCORE_EXPORT int collectSelectionRectsWithoutUnionInteriorLines(Vector<SelectionRect>&) const;
@@ -162,8 +164,10 @@ private:
     ExceptionOr<RefPtr<DocumentFragment>> processContents(ActionType);
 
     enum class CoordinateSpace { Absolute, Client };
-    Vector<FloatQuad> borderAndTextQuads(CoordinateSpace) const;
-    FloatRect boundingRect(CoordinateSpace) const;
+    Vector<FloatRect> borderAndTextRects(CoordinateSpace, RespectClippingForTextRects = RespectClippingForTextRects::No) const;
+    FloatRect boundingRect(CoordinateSpace, RespectClippingForTextRects = RespectClippingForTextRects::No) const;
+
+    Vector<FloatRect> absoluteRectsForRangeInText(Node*, RenderText&, bool useSelectionHeight, bool& isFixed, RespectClippingForTextRects) const;
 
     Ref<Document> m_ownerDocument;
     RangeBoundaryPoint m_start;
index 7630e20..18d00ff 100644 (file)
@@ -237,7 +237,7 @@ static Color estimatedBackgroundColorForRange(const Range& range, const Frame& f
         commonAncestor = commonAncestor->parentOrShadowHostElement();
     }
 
-    auto boundingRectForRange = enclosingIntRect(range.absoluteBoundingRect());
+    auto boundingRectForRange = enclosingIntRect(range.absoluteBoundingRect(Range::RespectClippingForTextRects::Yes));
     Vector<Color> parentRendererBackgroundColors;
     for (; !!renderer; renderer = renderer->parent()) {
         auto absoluteBoundingBox = renderer->absoluteBoundingBoxRect();
@@ -259,13 +259,16 @@ static Color estimatedBackgroundColorForRange(const Range& range, const Frame& f
     return estimatedBackgroundColor;
 }
 
-static void adjustTextIndicatorDataOptionsForEstimatedColorsIfNecessary(TextIndicatorData& data, const Color& backgroundColor, HashSet<Color>&& textColors)
+static bool hasAnyIllegibleColors(TextIndicatorData& data, const Color& backgroundColor, HashSet<Color>&& textColors)
 {
     if (data.options & TextIndicatorOptionPaintAllContent)
-        return;
+        return false;
 
     if (!(data.options & TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges))
-        return;
+        return false;
+
+    if (!(data.options & TextIndicatorOptionComputeEstimatedBackgroundColor))
+        return false;
 
     bool hasOnlyLegibleTextColors = true;
     if (data.options & TextIndicatorOptionRespectTextColor) {
@@ -277,11 +280,7 @@ static void adjustTextIndicatorDataOptionsForEstimatedColorsIfNecessary(TextIndi
     } else
         hasOnlyLegibleTextColors = textColorIsLegibleAgainstBackgroundColor(Color::black, backgroundColor);
 
-    if (!hasOnlyLegibleTextColors || !textColors.size()) {
-        // If the text color is not legible against the estimated color, force all content to be painted.
-        data.options &= ~TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges;
-        data.options |= TextIndicatorOptionPaintAllContent;
-    }
+    return !hasOnlyLegibleTextColors || textColors.isEmpty();
 }
 
 static bool initializeIndicator(TextIndicatorData& data, Frame& frame, const Range& range, FloatSize margin, bool indicatesCurrentSelection)
@@ -289,9 +288,10 @@ static bool initializeIndicator(TextIndicatorData& data, Frame& frame, const Ran
     if (auto* document = frame.document())
         document->updateLayoutIgnorePendingStylesheets();
 
+    bool treatRangeAsComplexDueToIllegibleTextColors = false;
     if (data.options & TextIndicatorOptionComputeEstimatedBackgroundColor) {
         data.estimatedBackgroundColor = estimatedBackgroundColorForRange(range, frame);
-        adjustTextIndicatorDataOptionsForEstimatedColorsIfNecessary(data, data.estimatedBackgroundColor, estimatedTextColorsForRange(range));
+        treatRangeAsComplexDueToIllegibleTextColors = hasAnyIllegibleColors(data, data.estimatedBackgroundColor, estimatedTextColorsForRange(range));
     }
 
     Vector<FloatRect> textRects;
@@ -304,17 +304,22 @@ static bool initializeIndicator(TextIndicatorData& data, Frame& frame, const Ran
 
     FrameSelection::TextRectangleHeight textRectHeight = (data.options & TextIndicatorOptionTightlyFitContent) ? FrameSelection::TextRectangleHeight::TextHeight : FrameSelection::TextRectangleHeight::SelectionHeight;
 
-    if ((data.options & TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges) && hasNonInlineOrReplacedElements(range))
+    if ((data.options & TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges) && (hasNonInlineOrReplacedElements(range) || treatRangeAsComplexDueToIllegibleTextColors))
         data.options |= TextIndicatorOptionPaintAllContent;
 #if PLATFORM(IOS)
     else if (data.options & TextIndicatorOptionUseSelectionRectForSizing)
         getSelectionRectsForRange(textRects, range);
 #endif
-    else
-        frame.selection().getTextRectangles(textRects, textRectHeight);
+    else {
+        Vector<IntRect> absoluteTextRects;
+        range.absoluteTextRects(absoluteTextRects, textRectHeight == FrameSelection::TextRectangleHeight::SelectionHeight, nullptr, Range::RespectClippingForTextRects::Yes);
+
+        for (auto& rect : absoluteTextRects)
+            textRects.append(rect);
+    }
 
     if (textRects.isEmpty())
-        textRects.append(range.absoluteBoundingRect());
+        textRects.append(range.absoluteBoundingRect(Range::RespectClippingForTextRects::Yes));
 
     auto frameView = frame.view();