FrameSelection's destructor shouldn't notify accessibility
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Feb 2014 03:50:22 +0000 (03:50 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Feb 2014 03:50:22 +0000 (03:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=128421

Reviewed by Benjamin Poulain.

Extracted setSelectionWithoutUpdatingAppearance out of setSelection and called it in prepareForDestruction
instead of setting DoNotUpdateAppearance option. This new function doesn't reveal selection or notify
accessibility code in addition to not updating appearance.

Note that all implementations of notifyAccessibilityForSelectionChange in FrameSelectionAtk.cpp and
FrameSelectionMac.mm exit early when the selection type is not caret or either start or end is null,
which is already the case inside FrameSelection's destructor. In addition, revealSelection is never called
when selection change was not triggered by user so there should be no behavioral change from this patch.

* editing/FrameSelection.cpp:
(WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance):
(WebCore::FrameSelection::setSelection):
(WebCore::FrameSelection::prepareForDestruction):
* editing/FrameSelection.h:
(WebCore::FrameSelection::notifyAccessibilityForSelectionChange): Added the trivial implementation in the case
accessibility is disabled to tidy up call sites.

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

Source/WebCore/ChangeLog
Source/WebCore/editing/FrameSelection.cpp
Source/WebCore/editing/FrameSelection.h

index ba25748..04f58ee 100644 (file)
@@ -1,3 +1,27 @@
+2014-02-07  Ryosuke Niwa  <rniwa@webkit.org>
+
+        FrameSelection's destructor shouldn't notify accessibility
+        https://bugs.webkit.org/show_bug.cgi?id=128421
+
+        Reviewed by Benjamin Poulain.
+
+        Extracted setSelectionWithoutUpdatingAppearance out of setSelection and called it in prepareForDestruction
+        instead of setting DoNotUpdateAppearance option. This new function doesn't reveal selection or notify
+        accessibility code in addition to not updating appearance.
+
+        Note that all implementations of notifyAccessibilityForSelectionChange in FrameSelectionAtk.cpp and
+        FrameSelectionMac.mm exit early when the selection type is not caret or either start or end is null,
+        which is already the case inside FrameSelection's destructor. In addition, revealSelection is never called
+        when selection change was not triggered by user so there should be no behavioral change from this patch.
+
+        * editing/FrameSelection.cpp:
+        (WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance):
+        (WebCore::FrameSelection::setSelection):
+        (WebCore::FrameSelection::prepareForDestruction):
+        * editing/FrameSelection.h:
+        (WebCore::FrameSelection::notifyAccessibilityForSelectionChange): Added the trivial implementation in the case
+        accessibility is disabled to tidy up call sites.
+
 2014-02-07  Martin Robinson  <mrobinson@igalia.com>
 
         Build fix for the GTK+ CMake build
index dd68f17..8427e78 100644 (file)
@@ -250,11 +250,10 @@ void FrameSelection::setSelectionByMouseIfDifferent(const VisibleSelection& pass
     setSelection(newSelection, UserTriggered | DoNotRevealSelection | CloseTyping | ClearTypingStyle, AlignCursorOnScrollIfNeeded, granularity);
 }
 
-void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity)
+bool FrameSelection::setSelectionWithoutUpdatingAppearance(const VisibleSelection& newSelection, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity, EUserTriggered userTriggered)
 {
     bool closeTyping = options & CloseTyping;
     bool shouldClearTypingStyle = options & ClearTypingStyle;
-    EUserTriggered userTriggered = selectionOptionsToUserTriggered(options);
 
     VisibleSelection s = newSelection;
     if (shouldAlwaysUseDirectionalSelection(m_frame))
@@ -262,7 +261,7 @@ void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec
 
     if (!m_frame) {
         m_selection = s;
-        return;
+        return false;
     }
 
     // <http://bugs.webkit.org/show_bug.cgi?id=23464>: Infinite recursion at FrameSelection::setSelection
@@ -277,7 +276,7 @@ void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec
             // the frame is about to be destroyed. If this is the case, clear our selection.
             if (guard->hasOneRef() && !m_selection.isNonOrphanedCaretOrRange())
                 clear();
-            return;
+            return false;
         }
     }
 
@@ -296,7 +295,7 @@ void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec
     if (m_selection == s) {
         // Even if selection was not changed, selection offsets may have been changed.
         updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);
-        return;
+        return false;
     }
 
     VisibleSelection oldSelection = m_selection;
@@ -307,21 +306,30 @@ void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec
     if (!s.isNone() && !(options & DoNotSetFocus))
         setFocusedElementIfNeeded();
 
-    if (!(options & DoNotUpdateAppearance)) {
-#if ENABLE(TEXT_CARET)
-        m_frame->document()->updateLayoutIgnorePendingStylesheets();
-#else
-        m_frame->document()->updateStyleIfNeeded();
-#endif
-        updateAppearance();
-    }
-
     // Always clear the x position used for vertical arrow navigation.
     // It will be restored by the vertical arrow navigation code if necessary.
     m_xPosForVerticalArrowNavigation = NoXPosForVerticalArrowNavigation();
     selectFrameElementInParentIfFullySelected();
     updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);
     m_frame->editor().respondToChangedSelection(oldSelection, options);
+    m_frame->document()->enqueueDocumentEvent(Event::create(eventNames().selectionchangeEvent, false, false));
+
+    return true;
+}
+
+void FrameSelection::setSelection(const VisibleSelection& selection, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity)
+{
+    EUserTriggered userTriggered = selectionOptionsToUserTriggered(options);
+    if (!setSelectionWithoutUpdatingAppearance(selection, options, align, granularity, userTriggered))
+        return;
+
+#if ENABLE(TEXT_CARET)
+    m_frame->document()->updateLayoutIgnorePendingStylesheets();
+#else
+    m_frame->document()->updateStyleIfNeeded();
+#endif
+    updateAppearance();
+
     if (userTriggered == UserTriggered && !(options & DoNotRevealSelection)) {
         ScrollAlignment alignment;
 
@@ -332,10 +340,8 @@ void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec
 
         revealSelection(alignment, RevealExtent);
     }
-#if HAVE(ACCESSIBILITY)
+
     notifyAccessibilityForSelectionChange();
-#endif
-    m_frame->document()->enqueueDocumentEvent(Event::create(eventNames().selectionchangeEvent, false, false));
 }
 
 static bool removingNodeRemovesPosition(Node* node, const Position& position)
@@ -1237,7 +1243,8 @@ void FrameSelection::prepareForDestruction()
     if (view)
         view->clearSelection();
 
-    setSelection(VisibleSelection(), CloseTyping | ClearTypingStyle | DoNotUpdateAppearance);
+    setSelectionWithoutUpdatingAppearance(VisibleSelection(), CloseTyping | ClearTypingStyle,
+        AlignCursorOnScrollIfNeeded, CharacterGranularity, NotUserTriggered);
     m_previousCaretNode.clear();
 }
 
index 0784fb1..f0417a8 100644 (file)
@@ -124,8 +124,7 @@ public:
         SpellCorrectionTriggered = 1 << 3,
         DoNotSetFocus = 1 << 4,
         DictationTriggered = 1 << 5,
-        DoNotUpdateAppearance = 1 << 6,
-        DoNotRevealSelection = 1 << 7,
+        DoNotRevealSelection = 1 << 6,
     };
     typedef unsigned SetSelectionOptions; // Union of values in SetSelectionOption and EUserTriggered
     static inline EUserTriggered selectionOptionsToUserTriggered(SetSelectionOptions options)
@@ -280,6 +279,8 @@ public:
 private:
     enum EPositionType { START, END, BASE, EXTENT };
 
+    bool setSelectionWithoutUpdatingAppearance(const VisibleSelection&, SetSelectionOptions, CursorAlignOnScroll, TextGranularity, EUserTriggered);
+
     void respondToNodeModification(Node*, bool baseRemoved, bool extentRemoved, bool startRemoved, bool endRemoved);
     TextDirection directionOfEnclosingBlock();
     TextDirection directionOfSelection();
@@ -302,6 +303,8 @@ private:
 
 #if HAVE(ACCESSIBILITY)
     void notifyAccessibilityForSelectionChange();
+#else
+    void notifyAccessibilityForSelectionChange() { }
 #endif
 
     void updateSelectionCachesIfSelectionIsInsideTextFormControl(EUserTriggered);