Yelp phone number highlights often disappear
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Aug 2014 01:26:14 +0000 (01:26 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Aug 2014 01:26:14 +0000 (01:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=135789
<rdar://problem/17971057>

Reviewed by Brady Eidson.

Since selectedTelephoneNumberRangesChanged doesn't provide an associated
Frame, an incoming selectedTelephoneNumberRangesChanged from a subframe
would overwrite ServicesOverlayController's cached (and potentially active)
telephone number highlights.

This happens a lot on Yelp, because they have many subframes which are
doing layout on a regular basis.

* WebProcess/WebCoreSupport/WebEditorClient.cpp:
(WebKit::WebEditorClient::selectedTelephoneNumberRangesChanged):
* WebProcess/WebCoreSupport/WebEditorClient.h:
Adjust to the new (lack of) arguments.

* WebProcess/WebPage/ServicesOverlayController.h:
* WebProcess/WebPage/mac/ServicesOverlayController.mm:
(WebKit::ServicesOverlayController::selectedTelephoneNumberRangesChanged):
Adjust logging; we can revisit it later.

(WebKit::ServicesOverlayController::buildPhoneNumberHighlights):
When building phone number highlights, walk the Frame tree and retrieve
them from all of the Editors.

(WebKit::ServicesOverlayController::didRebuildPotentialHighlights):
(WebKit::ServicesOverlayController::telephoneNumberRangesForFocusedFrame):
(WebKit::ServicesOverlayController::determineActiveHighlight):
(WebKit::ServicesOverlayController::handleClick):
Retrieve the detected telephone number ranges from the focused frame
when combining telephone numbers with selection services.
This ensures that we don't show combined phone number highlights from other frames.

* editing/Editor.cpp:
(WebCore::Editor::scanSelectionForTelephoneNumbers):
(WebCore::Editor::clearDataDetectedTelephoneNumbers): Deleted.
* editing/Editor.h:
(WebCore::Editor::detectedTelephoneNumberRanges):
* page/EditorClient.h:
(WebCore::EditorClient::selectedTelephoneNumberRangesChanged):
Cache and expose detected telephone number ranges on Editor.
Change selectedTelephoneNumberRangesChanged to take no arguments; it's
just a notification now.

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

Source/WebCore/ChangeLog
Source/WebCore/editing/Editor.cpp
Source/WebCore/editing/Editor.h
Source/WebCore/page/EditorClient.h
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp
Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h
Source/WebKit2/WebProcess/WebPage/ServicesOverlayController.h
Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm

index b6ab661..4fc4005 100644 (file)
@@ -1,3 +1,22 @@
+2014-08-10  Tim Horton  <timothy_horton@apple.com>
+
+        Yelp phone number highlights often disappear
+        https://bugs.webkit.org/show_bug.cgi?id=135789
+        <rdar://problem/17971057>
+
+        Reviewed by Brady Eidson.
+
+        * editing/Editor.cpp:
+        (WebCore::Editor::scanSelectionForTelephoneNumbers):
+        (WebCore::Editor::clearDataDetectedTelephoneNumbers): Deleted.
+        * editing/Editor.h:
+        (WebCore::Editor::detectedTelephoneNumberRanges):
+        * page/EditorClient.h:
+        (WebCore::EditorClient::selectedTelephoneNumberRangesChanged):
+        Cache and expose detected telephone number ranges on Editor.
+        Change selectedTelephoneNumberRangesChanged to take no arguments; it's
+        just a notification now.
+
 2014-08-09  Zalan Bujtas  <zalan@apple.com>
 
         Subpixel rendering: Transforms on non-compositing layers leave bits behind when the box boundaries changes.
index a3da9a2..7425f63 100644 (file)
@@ -3362,11 +3362,13 @@ void Editor::scanSelectionForTelephoneNumbers()
     if (!shouldDetectTelephoneNumbers() || !client())
         return;
 
+    m_detectedTelephoneNumberRanges.clear();
+
     Vector<RefPtr<Range>> markedRanges;
 
     FrameSelection& frameSelection = m_frame.selection();
     if (!frameSelection.isRange()) {
-        client()->selectedTelephoneNumberRangesChanged(markedRanges);
+        client()->selectedTelephoneNumberRangesChanged();
         return;
     }
     RefPtr<Range> selectedRange = frameSelection.toNormalizedRange();
@@ -3394,20 +3396,19 @@ void Editor::scanSelectionForTelephoneNumbers()
     RefPtr<Range> extendedRange = extendedSelection.toNormalizedRange();
 
     if (!extendedRange) {
-        client()->selectedTelephoneNumberRangesChanged(markedRanges);
+        client()->selectedTelephoneNumberRangesChanged();
         return;
     }
 
     scanRangeForTelephoneNumbers(*extendedRange, extendedRange->text(), markedRanges);
 
     // Only consider ranges with a detected telephone number if they overlap with the actual selection range.
-    Vector<RefPtr<Range>> markedRangesIntersectingSelection;
     for (auto& range : markedRanges) {
         if (rangesOverlap(range.get(), selectedRange.get()))
-            markedRangesIntersectingSelection.append(range);
+            m_detectedTelephoneNumberRanges.append(range);
     }
 
-    client()->selectedTelephoneNumberRangesChanged(markedRangesIntersectingSelection);
+    client()->selectedTelephoneNumberRangesChanged();
 }
 
 void Editor::scanRangeForTelephoneNumbers(Range& range, const StringView& stringView, Vector<RefPtr<Range>>& markedRanges)
@@ -3448,13 +3449,6 @@ void Editor::scanRangeForTelephoneNumbers(Range& range, const StringView& string
     }
 }
 
-void Editor::clearDataDetectedTelephoneNumbers()
-{
-    document().markers().removeMarkers(DocumentMarker::TelephoneNumber);
-
-    // FIXME: Do other UI cleanup here once we have other UI.
-}
-
 #endif // ENABLE(TELEPHONE_NUMBER_DETECTION) && !PLATFORM(IOS)
 
 void Editor::updateEditorUINowIfScheduled()
index a0b2ed3..8118b9d 100644 (file)
@@ -450,6 +450,7 @@ public:
 
 #if ENABLE(TELEPHONE_NUMBER_DETECTION) && !PLATFORM(IOS)
     void scanSelectionForTelephoneNumbers();
+    const Vector<RefPtr<Range>>& detectedTelephoneNumberRanges() const { return m_detectedTelephoneNumberRanges; }
 #endif
 
 private:
@@ -521,9 +522,9 @@ private:
     bool shouldDetectTelephoneNumbers();
     void scanSelectionForTelephoneNumbers(Timer<Editor>&);
     void scanRangeForTelephoneNumbers(Range&, const StringView&, Vector<RefPtr<Range>>& markedRanges);
-    void clearDataDetectedTelephoneNumbers();
 
     Timer<Editor> m_telephoneNumberDetectionUpdateTimer;
+    Vector<RefPtr<Range>> m_detectedTelephoneNumberRanges;
 #endif
 };
 
index de72bd0..2be3011 100644 (file)
@@ -182,7 +182,7 @@ public:
     virtual void setInputMethodState(bool enabled) = 0;
 
 #if ENABLE(SERVICE_CONTROLS) || ENABLE(TELEPHONE_NUMBER_DETECTION)
-    virtual void selectedTelephoneNumberRangesChanged(const Vector<RefPtr<Range>>&) { }
+    virtual void selectedTelephoneNumberRangesChanged() { }
     virtual void selectionRectsDidChange(const Vector<LayoutRect>&, const Vector<GapRects>&, bool) { }
 #endif
 
index 48913eb..67b4271 100644 (file)
@@ -1,5 +1,43 @@
 2014-08-10  Tim Horton  <timothy_horton@apple.com>
 
+        Yelp phone number highlights often disappear
+        https://bugs.webkit.org/show_bug.cgi?id=135789
+        <rdar://problem/17971057>
+
+        Reviewed by Brady Eidson.
+
+        Since selectedTelephoneNumberRangesChanged doesn't provide an associated
+        Frame, an incoming selectedTelephoneNumberRangesChanged from a subframe
+        would overwrite ServicesOverlayController's cached (and potentially active)
+        telephone number highlights.
+
+        This happens a lot on Yelp, because they have many subframes which are
+        doing layout on a regular basis.
+
+        * WebProcess/WebCoreSupport/WebEditorClient.cpp:
+        (WebKit::WebEditorClient::selectedTelephoneNumberRangesChanged):
+        * WebProcess/WebCoreSupport/WebEditorClient.h:
+        Adjust to the new (lack of) arguments.
+
+        * WebProcess/WebPage/ServicesOverlayController.h:
+        * WebProcess/WebPage/mac/ServicesOverlayController.mm:
+        (WebKit::ServicesOverlayController::selectedTelephoneNumberRangesChanged):
+        Adjust logging; we can revisit it later.
+
+        (WebKit::ServicesOverlayController::buildPhoneNumberHighlights):
+        When building phone number highlights, walk the Frame tree and retrieve
+        them from all of the Editors.
+
+        (WebKit::ServicesOverlayController::didRebuildPotentialHighlights):
+        (WebKit::ServicesOverlayController::telephoneNumberRangesForFocusedFrame):
+        (WebKit::ServicesOverlayController::determineActiveHighlight):
+        (WebKit::ServicesOverlayController::handleClick):
+        Retrieve the detected telephone number ranges from the focused frame
+        when combining telephone numbers with selection services.
+        This ensures that we don't show combined phone number highlights from other frames.
+
+2014-08-10  Tim Horton  <timothy_horton@apple.com>
+
         Refactor ServiceOverlayController in preparation for fading between highlights
         https://bugs.webkit.org/show_bug.cgi?id=135787
         <rdar://problem/17935736>
index 57e19ee..bff76f2 100644 (file)
@@ -532,10 +532,10 @@ bool WebEditorClient::supportsGlobalSelection()
 }
 
 #if ENABLE(SERVICE_CONTROLS) || ENABLE(TELEPHONE_NUMBER_DETECTION)
-void WebEditorClient::selectedTelephoneNumberRangesChanged(const Vector<RefPtr<Range>>& ranges)
+void WebEditorClient::selectedTelephoneNumberRangesChanged()
 {
 #if PLATFORM(MAC)
-    m_page->servicesOverlayController().selectedTelephoneNumberRangesChanged(ranges);
+    m_page->servicesOverlayController().selectedTelephoneNumberRangesChanged();
 #endif
 }
 void WebEditorClient::selectionRectsDidChange(const Vector<LayoutRect>& rects, const Vector<GapRects>& gapRects, bool isTextOnly)
index a33f406..f34a9eb 100644 (file)
@@ -169,7 +169,7 @@ private:
     virtual bool supportsGlobalSelection() override;
 
 #if ENABLE(TELEPHONE_NUMBER_DETECTION) || ENABLE(SERVICE_CONTROLS)
-    virtual void selectedTelephoneNumberRangesChanged(const Vector<RefPtr<WebCore::Range>>&) override;
+    virtual void selectedTelephoneNumberRangesChanged() override;
     virtual void selectionRectsDidChange(const Vector<WebCore::LayoutRect>&, const Vector<WebCore::GapRects>&, bool isTextOnly) override;
 #endif
 
index d8618be..1b73f76 100644 (file)
@@ -29,6 +29,7 @@
 #if ENABLE(SERVICE_CONTROLS) || ENABLE(TELEPHONE_NUMBER_DETECTION)
 
 #include "PageOverlay.h"
+#include "WebFrame.h"
 #include <WebCore/Range.h>
 #include <WebCore/Timer.h>
 #include <wtf/RefCounted.h>
@@ -50,7 +51,7 @@ public:
     ServicesOverlayController(WebPage&);
     ~ServicesOverlayController();
 
-    void selectedTelephoneNumberRangesChanged(const Vector<RefPtr<WebCore::Range>>&);
+    void selectedTelephoneNumberRangesChanged();
     void selectionRectsDidChange(const Vector<WebCore::LayoutRect>&, const Vector<WebCore::GapRects>&, bool isTextOnly);
 
 private:
@@ -112,13 +113,14 @@ private:
 
     static bool highlightsAreEquivalent(const Highlight* a, const Highlight* b);
 
+    Vector<RefPtr<WebCore::Range>> telephoneNumberRangesForFocusedFrame();
+
     WebPage& m_webPage;
     PageOverlay* m_servicesOverlay;
 
     RefPtr<Highlight> m_activeHighlight;
     HashSet<RefPtr<Highlight>> m_potentialHighlights;
 
-    Vector<RefPtr<WebCore::Range>> m_currentTelephoneNumberRanges;
     Vector<WebCore::LayoutRect> m_currentSelectionRects;
     bool m_isTextOnly;
 
index 8d639e2..14dbb05 100644 (file)
@@ -33,6 +33,7 @@
 #import "WebProcess.h"
 #import <WebCore/Document.h>
 #import <WebCore/FloatQuad.h>
+#import <WebCore/FocusController.h>
 #import <WebCore/FrameView.h>
 #import <WebCore/GapRects.h>
 #import <WebCore/GraphicsContext.h>
@@ -224,12 +225,10 @@ void ServicesOverlayController::selectionRectsDidChange(const Vector<LayoutRect>
 #endif
 }
 
-void ServicesOverlayController::selectedTelephoneNumberRangesChanged(const Vector<RefPtr<Range>>& ranges)
+void ServicesOverlayController::selectedTelephoneNumberRangesChanged()
 {
 #if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED > 1090
-    LOG(Services, "ServicesOverlayController - Telephone number ranges changed - Had %lu, now have %lu\n", m_currentTelephoneNumberRanges.size(), ranges.size());
-    m_currentTelephoneNumberRanges = ranges;
-
+    LOG(Services, "ServicesOverlayController - Telephone number ranges changed\n");
     buildPhoneNumberHighlights();
 #else
     UNUSED_PARAM(ranges);
@@ -323,25 +322,30 @@ void ServicesOverlayController::buildPhoneNumberHighlights()
 {
     removeAllPotentialHighlightsOfType(Highlight::Type::TelephoneNumber);
 
-    for (unsigned i = 0; i < m_currentTelephoneNumberRanges.size(); ++i) {
-        // FIXME: This will choke if the range wraps around the edge of the view.
-        // What should we do in that case?
-        IntRect rect = textQuadsToBoundingRectForRange(*m_currentTelephoneNumberRanges[i]);
-
-        // Convert to the main document's coordinate space.
-        // FIXME: It's a little crazy to call contentsToWindow and then windowToContents in order to get the right coordinate space.
-        // We should consider adding conversion functions to ScrollView for contentsToDocument(). Right now, contentsToRootView() is
-        // not equivalent to what we need when you have a topContentInset or a header banner.
-        FrameView* viewForRange = m_currentTelephoneNumberRanges[i]->ownerDocument().view();
-        if (!viewForRange)
-            continue;
-        FrameView& mainFrameView = *m_webPage.corePage()->mainFrame().view();
-        rect.setLocation(mainFrameView.windowToContents(viewForRange->contentsToWindow(rect.location())));
+    Frame* mainFrame = m_webPage.mainFrame();
+    FrameView& mainFrameView = *mainFrame->view();
+
+    for (Frame* frame = mainFrame; frame; frame = frame->tree().traverseNext()) {
+        auto& ranges = frame->editor().detectedTelephoneNumberRanges();
+        for (auto& range : ranges) {
+            // FIXME: This will choke if the range wraps around the edge of the view.
+            // What should we do in that case?
+            IntRect rect = textQuadsToBoundingRectForRange(*range);
+
+            // Convert to the main document's coordinate space.
+            // FIXME: It's a little crazy to call contentsToWindow and then windowToContents in order to get the right coordinate space.
+            // We should consider adding conversion functions to ScrollView for contentsToDocument(). Right now, contentsToRootView() is
+            // not equivalent to what we need when you have a topContentInset or a header banner.
+            FrameView* viewForRange = range->ownerDocument().view();
+            if (!viewForRange)
+                continue;
+            rect.setLocation(mainFrameView.windowToContents(viewForRange->contentsToWindow(rect.location())));
 
-        CGRect cgRect = rect;
-        RetainPtr<DDHighlightRef> ddHighlight = adoptCF(DDHighlightCreateWithRectsInVisibleRectWithStyleAndDirection(nullptr, &cgRect, 1, mainFrameView.visibleContentRect(), DDHighlightOutlineWithArrow, YES, NSWritingDirectionNatural, NO, YES));
+            CGRect cgRect = rect;
+            RetainPtr<DDHighlightRef> ddHighlight = adoptCF(DDHighlightCreateWithRectsInVisibleRectWithStyleAndDirection(nullptr, &cgRect, 1, mainFrameView.visibleContentRect(), DDHighlightOutlineWithArrow, YES, NSWritingDirectionNatural, NO, YES));
 
-        m_potentialHighlights.add(Highlight::createForTelephoneNumber(ddHighlight, m_currentTelephoneNumberRanges[i]));
+            m_potentialHighlights.add(Highlight::createForTelephoneNumber(ddHighlight, range));
+        }
     }
 
     didRebuildPotentialHighlights();
@@ -380,7 +384,7 @@ void ServicesOverlayController::didRebuildPotentialHighlights()
         return;
     }
 
-    if (m_currentTelephoneNumberRanges.isEmpty() && !hasRelevantSelectionServices())
+    if (telephoneNumberRangesForFocusedFrame().isEmpty() && !hasRelevantSelectionServices())
         return;
 
     createOverlayIfNeeded();
@@ -399,6 +403,15 @@ void ServicesOverlayController::createOverlayIfNeeded()
     m_servicesOverlay->setNeedsDisplay();
 }
 
+Vector<RefPtr<Range>> ServicesOverlayController::telephoneNumberRangesForFocusedFrame()
+{
+    Page* page = m_webPage.corePage();
+    if (!page)
+        return Vector<RefPtr<Range>>();
+
+    return page->focusController().focusedOrMainFrame().editor().detectedTelephoneNumberRanges();
+}
+
 bool ServicesOverlayController::highlightsAreEquivalent(const Highlight* a, const Highlight* b)
 {
     if (a == b)
@@ -427,7 +440,7 @@ void ServicesOverlayController::determineActiveHighlight(bool& mouseIsOverActive
                 continue;
 
             // If this highlight has no compatible services, it can't be active, unless we have telephone number highlights to show in the combined menu.
-            if (m_currentTelephoneNumberRanges.isEmpty() && !hasRelevantSelectionServices())
+            if (telephoneNumberRangesForFocusedFrame().isEmpty() && !hasRelevantSelectionServices())
                 continue;
         }
 
@@ -505,9 +518,10 @@ void ServicesOverlayController::handleClick(const WebCore::IntPoint& clickPoint,
     IntPoint windowPoint = frameView->contentsToWindow(clickPoint);
 
     if (highlight.type() == Highlight::Type::Selection) {
+        auto telephoneNumberRanges = telephoneNumberRangesForFocusedFrame();
         Vector<String> selectedTelephoneNumbers;
-        selectedTelephoneNumbers.reserveCapacity(m_currentTelephoneNumberRanges.size());
-        for (auto& range : m_currentTelephoneNumberRanges)
+        selectedTelephoneNumbers.reserveCapacity(telephoneNumberRanges.size());
+        for (auto& range : telephoneNumberRanges)
             selectedTelephoneNumbers.append(range->text());
 
         m_webPage.handleSelectionServiceClick(m_webPage.corePage()->mainFrame().selection(), selectedTelephoneNumbers, windowPoint);