Changeset 163689 in webkit

Timestamp:

Feb 7, 2014 7:50:22 PM (10 years ago)

Author:

rniwa@webkit.org

Message:

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• editing/FrameSelection.cpp (modified) (8 diffs)
• editing/FrameSelection.h (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/editing/FrameSelection.cpp

@@ -251 +251 @@
 }
 
-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;
@@ -263 +262 @@
     if (!m_frame) {
         m_selection = s;
-        return;
+        return false;
     }
 
@@ -278 +277 @@
             if (guard->hasOneRef() && !m_selection.isNonOrphanedCaretOrRange())
                 clear();
-            return;
+            return false;
         }
     }
@@ -297 +296 @@
         // Even if selection was not changed, selection offsets may have been changed.
         updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);
-        return;
+        return false;
     }
 
@@ -307 +306 @@
     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.
@@ -323 +313 @@
     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;
@@ -333 +341 @@
         revealSelection(alignment, RevealExtent);
     }
-#if HAVE(ACCESSIBILITY)
+
     notifyAccessibilityForSelectionChange();
-#endif
-    m_frame->document()->enqueueDocumentEvent(Event::create(eventNames().selectionchangeEvent, false, false));
 }
 
@@ -1238 +1244 @@
         view->clearSelection();
 
-    setSelection(VisibleSelection(), CloseTyping | ClearTypingStyle | DoNotUpdateAppearance);
+    setSelectionWithoutUpdatingAppearance(VisibleSelection(), CloseTyping | ClearTypingStyle,
+        AlignCursorOnScrollIfNeeded, CharacterGranularity, NotUserTriggered);
     m_previousCaretNode.clear();
 }

trunk/Source/WebCore/editing/FrameSelection.h

@@ -125 +125 @@
         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
@@ -281 +280 @@
     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();
@@ -303 +304 @@
 #if HAVE(ACCESSIBILITY)
     void notifyAccessibilityForSelectionChange();
+#else
+    void notifyAccessibilityForSelectionChange() { }
 #endif