Holes for find matches that span multiple lines are completely wrong
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Aug 2015 18:12:08 +0000 (18:12 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Aug 2015 18:12:08 +0000 (18:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=148065
<rdar://problem/5305002>

Reviewed by Simon Fraser.

Test: fast/text/mark-matches-broken-line-rendering.html

Two big problems with find holes:
    - RenderedDocumentMarker only has one rect, but a marker can be painted
      by multiple text runs, each with their own rendered rect
    - paintTextMatchMarker does std::max((unsigned)a, (unsigned)0), which is
      obviously an overflow problem/not going to work if a is negative

The combination of these issues causes the holes for find matches to be
very broken: text that isn't part of the match is highlighted, and some
text that is part of the match isn't highlighted.

To fix, make RenderedDocumentMarker support multiple rects, and use signed
math (like paintDocumentMarker already did) when computing positions in paintTextMatchMarker.

* dom/DocumentMarkerController.cpp:
(WebCore::DocumentMarkerController::addTextMatchMarker):
(WebCore::DocumentMarkerController::renderedRectsForMarkers):
* dom/RenderedDocumentMarker.h:
(WebCore::RenderedDocumentMarker::RenderedDocumentMarker):
(WebCore::RenderedDocumentMarker::isRendered):
(WebCore::RenderedDocumentMarker::contains):
(WebCore::RenderedDocumentMarker::addRenderedRect):
(WebCore::RenderedDocumentMarker::renderedRects):
(WebCore::RenderedDocumentMarker::invalidate):
(WebCore::RenderedDocumentMarker::setRenderedRect): Deleted.
(WebCore::RenderedDocumentMarker::renderedRect): Deleted.
(WebCore::RenderedDocumentMarker::invalidMarkerRect): Deleted.
* rendering/InlineTextBox.cpp:
(WebCore::InlineTextBox::paintDocumentMarker):
(WebCore::InlineTextBox::paintTextMatchMarker):
(WebCore::InlineTextBox::computeRectForReplacementMarker):
* rendering/svg/SVGInlineFlowBox.cpp:
(WebCore::SVGInlineFlowBox::computeTextMatchMarkerRectForRenderer):

* fast/text/mark-matches-broken-line-rendering-expected.html: Added.
* fast/text/mark-matches-broken-line-rendering.html: Added.
Add a test that ensures that we correctly mark test matches that cross
line breaks.

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

LayoutTests/ChangeLog
LayoutTests/fast/text/mark-matches-broken-line-rendering-expected.html [new file with mode: 0644]
LayoutTests/fast/text/mark-matches-broken-line-rendering.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/DocumentMarkerController.cpp
Source/WebCore/dom/RenderedDocumentMarker.h
Source/WebCore/rendering/InlineTextBox.cpp
Source/WebCore/rendering/svg/SVGInlineFlowBox.cpp

index 211b474..f70f68a 100644 (file)
@@ -1,3 +1,16 @@
+2015-08-17  Timothy Horton  <timothy_horton@apple.com>
+
+        Holes for find matches that span multiple lines are completely wrong
+        https://bugs.webkit.org/show_bug.cgi?id=148065
+        <rdar://problem/5305002>
+
+        Reviewed by Simon Fraser.
+
+        * fast/text/mark-matches-broken-line-rendering-expected.html: Added.
+        * fast/text/mark-matches-broken-line-rendering.html: Added.
+        Add a test that ensures that we correctly mark test matches that cross
+        line breaks.
+
 2015-08-17  Chris Dumez  <cdumez@apple.com>
 
         Accessing HTMLCollection.length is slow
diff --git a/LayoutTests/fast/text/mark-matches-broken-line-rendering-expected.html b/LayoutTests/fast/text/mark-matches-broken-line-rendering-expected.html
new file mode 100644 (file)
index 0000000..afd8a57
--- /dev/null
@@ -0,0 +1,14 @@
+<style>
+p {
+    display: inline-block;
+    background-color: blue;
+    width: 60px;
+}
+
+span {
+    background-color: yellow;
+}
+</style>
+<p>
+<span>Quo</span> <span>usque</span> <span>tandem</span> <span>abutere,</span> <span>Catilina,</span> <span>patientia</span> <span>nostra?</span>
+</p>
diff --git a/LayoutTests/fast/text/mark-matches-broken-line-rendering.html b/LayoutTests/fast/text/mark-matches-broken-line-rendering.html
new file mode 100644 (file)
index 0000000..ab3756b
--- /dev/null
@@ -0,0 +1,16 @@
+<style>
+p {
+    display: inline-block;
+    background-color: blue;
+    width: 60px;
+}
+</style>
+<p>
+Quo usque tandem abutere, Catilina, patientia nostra?
+</p>
+<script>
+if (window.internals) {
+    internals.setMarkedTextMatchesAreHighlighted(true);
+    internals.countMatchesForText("Quo usque tandem abutere, Catilina, patientia nostra?", 0, "mark");
+}
+</script>
index 70f32bc..97ed3c8 100644 (file)
@@ -1,5 +1,48 @@
 2015-08-17  Timothy Horton  <timothy_horton@apple.com>
 
+        Holes for find matches that span multiple lines are completely wrong
+        https://bugs.webkit.org/show_bug.cgi?id=148065
+        <rdar://problem/5305002>
+
+        Reviewed by Simon Fraser.
+
+        Test: fast/text/mark-matches-broken-line-rendering.html
+
+        Two big problems with find holes:
+            - RenderedDocumentMarker only has one rect, but a marker can be painted
+              by multiple text runs, each with their own rendered rect
+            - paintTextMatchMarker does std::max((unsigned)a, (unsigned)0), which is
+              obviously an overflow problem/not going to work if a is negative
+
+        The combination of these issues causes the holes for find matches to be
+        very broken: text that isn't part of the match is highlighted, and some
+        text that is part of the match isn't highlighted.
+
+        To fix, make RenderedDocumentMarker support multiple rects, and use signed
+        math (like paintDocumentMarker already did) when computing positions in paintTextMatchMarker.
+
+        * dom/DocumentMarkerController.cpp:
+        (WebCore::DocumentMarkerController::addTextMatchMarker):
+        (WebCore::DocumentMarkerController::renderedRectsForMarkers):
+        * dom/RenderedDocumentMarker.h:
+        (WebCore::RenderedDocumentMarker::RenderedDocumentMarker):
+        (WebCore::RenderedDocumentMarker::isRendered):
+        (WebCore::RenderedDocumentMarker::contains):
+        (WebCore::RenderedDocumentMarker::addRenderedRect):
+        (WebCore::RenderedDocumentMarker::renderedRects):
+        (WebCore::RenderedDocumentMarker::invalidate):
+        (WebCore::RenderedDocumentMarker::setRenderedRect): Deleted.
+        (WebCore::RenderedDocumentMarker::renderedRect): Deleted.
+        (WebCore::RenderedDocumentMarker::invalidMarkerRect): Deleted.
+        * rendering/InlineTextBox.cpp:
+        (WebCore::InlineTextBox::paintDocumentMarker):
+        (WebCore::InlineTextBox::paintTextMatchMarker):
+        (WebCore::InlineTextBox::computeRectForReplacementMarker):
+        * rendering/svg/SVGInlineFlowBox.cpp:
+        (WebCore::SVGInlineFlowBox::computeTextMatchMarkerRectForRenderer):
+
+2015-08-17  Timothy Horton  <timothy_horton@apple.com>
+
         Adopt shrink-wrapping for TextIndicators on Mac
         https://bugs.webkit.org/show_bug.cgi?id=148064
 
index 1d23049..810943d 100644 (file)
@@ -102,7 +102,7 @@ void DocumentMarkerController::addTextMatchMarker(const Range* range, bool activ
             // matches off-screen are (that haven't been painted yet).
             Node* node = textPiece->startContainer();
             Vector<RenderedDocumentMarker*> markers = markersFor(node);
-            markers[markers.size() - 1]->setRenderedRect(range->absoluteBoundingBox());
+            markers[markers.size() - 1]->addRenderedRect(range->absoluteBoundingBox());
         }
     }
 }
@@ -439,10 +439,7 @@ Vector<IntRect> DocumentMarkerController::renderedRectsForMarkers(DocumentMarker
             if (marker.type() != markerType)
                 continue;
 
-            if (!marker.isRendered())
-                continue;
-
-            result.append(marker.renderedRect());
+            result.appendVector(marker.renderedRects());
         }
     }
 
index 7d48115..8998c66 100644 (file)
 #define RenderedDocumentMarker_h
 
 #include "DocumentMarker.h"
+#include <wtf/Vector.h>
 
 namespace WebCore {
 
 class RenderedDocumentMarker : public DocumentMarker {
 public:
-
     explicit RenderedDocumentMarker(const DocumentMarker& marker)
-        : DocumentMarker(marker), m_renderedRect(invalidMarkerRect())
+        : DocumentMarker(marker)
     {
     }
 
-    bool isRendered() const { return invalidMarkerRect() != m_renderedRect; }
-    bool contains(const LayoutPoint& point) const { return isRendered() && m_renderedRect.contains(point); }
-    void setRenderedRect(const LayoutRect& r) { m_renderedRect = r; }
-    const LayoutRect& renderedRect() const { return m_renderedRect; }
-    void invalidate(const LayoutRect&);
-    void invalidate() { m_renderedRect = invalidMarkerRect(); }
+    bool contains(const LayoutPoint& point) const
+    {
+        for (const auto& rect : m_renderedRects) {
+            if (rect.contains(point))
+                return true;
+        }
+        return false;
+    }
 
-private:
-    static const LayoutRect& invalidMarkerRect()
+    void addRenderedRect(const LayoutRect& r) { m_renderedRects.append(r); }
+    const Vector<LayoutRect, 1>& renderedRects() const { return m_renderedRects; }
+    void invalidate(const LayoutRect& r)
     {
-        static const LayoutRect rect = LayoutRect(-1, -1, -1, -1);
-        return rect;
+        for (const auto& rect : m_renderedRects) {
+            if (rect.intersects(r)) {
+                invalidate();
+                return;
+            }
+        }
     }
+    void invalidate() { m_renderedRects.clear(); }
 
-    LayoutRect m_renderedRect;
+private:
+    Vector<LayoutRect, 1> m_renderedRects;
 };
 
-inline void RenderedDocumentMarker::invalidate(const LayoutRect& r)
-{
-    if (m_renderedRect.intersects(r))
-        invalidate();
-}
 
 } // namespace
 
index 40b4d2e..9e6d2ce 100644 (file)
@@ -1104,7 +1104,7 @@ void InlineTextBox::paintDocumentMarker(GraphicsContext& context, const FloatPoi
         if (grammar || isDictationMarker) {
             markerRect.move(-boxOrigin.x(), -boxOrigin.y());
             markerRect = renderer().localToAbsoluteQuad(FloatRect(markerRect)).enclosingBoundingBox();
-            marker.setRenderedRect(markerRect);
+            marker.addRenderedRect(markerRect);
         }
     }
     
@@ -1132,8 +1132,8 @@ void InlineTextBox::paintTextMatchMarker(GraphicsContext& context, const FloatPo
 {
     LayoutUnit selectionHeight = this->selectionHeight();
 
-    int sPos = std::max(marker.startOffset() - m_start, (unsigned)0);
-    int ePos = std::min(marker.endOffset() - m_start, (unsigned)m_len);
+    int sPos = std::max<int>(marker.startOffset() - m_start, 0);
+    int ePos = std::min<int>(marker.endOffset() - m_start, m_len);
     TextRun run = constructTextRun(style, font);
 
     // Always compute and store the rect associated with this marker. The computed rect is in absolute coordinates.
@@ -1142,7 +1142,7 @@ void InlineTextBox::paintTextMatchMarker(GraphicsContext& context, const FloatPo
     font.adjustSelectionRectForText(run, renderedRect, sPos, ePos);
     IntRect markerRect = enclosingIntRect(renderedRect);
     markerRect = renderer().localToAbsoluteQuad(FloatQuad(markerRect)).enclosingBoundingBox();
-    marker.setRenderedRect(markerRect);
+    marker.addRenderedRect(markerRect);
     
     // Optionally highlight the text
     if (renderer().frame().editor().markedTextMatchesAreHighlighted()) {
@@ -1174,7 +1174,7 @@ void InlineTextBox::computeRectForReplacementMarker(RenderedDocumentMarker& mark
     font.adjustSelectionRectForText(run, selectionRect, sPos, ePos);
     IntRect markerRect = enclosingIntRect(selectionRect);
     markerRect = renderer().localToAbsoluteQuad(FloatRect(markerRect)).enclosingBoundingBox();
-    marker.setRenderedRect(markerRect);
+    marker.addRenderedRect(markerRect);
 }
     
 void InlineTextBox::paintDocumentMarkers(GraphicsContext& context, const FloatPoint& boxOrigin, const RenderStyle& style, const FontCascade& font, bool background)
index ad345f0..d1f4c5c 100644 (file)
@@ -124,7 +124,7 @@ void SVGInlineFlowBox::computeTextMatchMarkerRectForRenderer(RenderSVGInlineText
             }
         }
 
-        marker->setRenderedRect(textRenderer->localToAbsoluteQuad(markerRect).enclosingBoundingBox());
+        marker->addRenderedRect(textRenderer->localToAbsoluteQuad(markerRect).enclosingBoundingBox());
     }
 }