Attachment #223539 for bug #128421

View Details Raw Unified Return to Bug 128421

| Differences between



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 NOBODY (OOPS!).

        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  Roger Fong  <roger_fong@apple.com>

        CGContextGetUserSpaceToDeviceSpaceTransform returns the wrong value on Windows.



    setSelection(newSelection, UserTriggered | DoNotRevealSelection | CloseTyping | ClearTypingStyle, AlignCursorOnScrollIfNeeded, 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))


    if (!m_frame) {
        m_selection = s;
        return false;
    }

    // <http://bugs.webkit.org/show_bug.cgi?id=23464>: Infinite recursion at FrameSelection::setSelection

            // the frame is about to be destroyed. If this is the case, clear our selection.
            if (guard->hasOneRef() && !m_selection.isNonOrphanedCaretOrRange())
                clear();
            return false;
        }
    }


    if (m_selection == s) {
        // Even if selection was not changed, selection offsets may have been changed.
        updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);
        return false;
    }

    VisibleSelection oldSelection = m_selection;

    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;



        revealSelection(alignment, RevealExtent);
    }

    notifyAccessibilityForSelectionChange();
#endif
    m_frame->document()->enqueueDocumentEvent(Event::create(eventNames().selectionchangeEvent, false, false));
}

static bool removingNodeRemovesPosition(Node* node, const Position& position)

    if (view)
        view->clearSelection();

    setSelectionWithoutUpdatingAppearance(VisibleSelection(), CloseTyping | ClearTypingStyle,
        AlignCursorOnScrollIfNeeded, CharacterGranularity, NotUserTriggered);
    m_previousCaretNode.clear();
}


Lines 124-131 public: a/Source/WebCore/editing/FrameSelection.h_sec1
- a/Source/WebCore/editing/FrameSelection.h -2 / +5 lines
124	SpellCorrectionTriggered = 1 << 3,	124	SpellCorrectionTriggered = 1 << 3,
125	DoNotSetFocus = 1 << 4,	125	DoNotSetFocus = 1 << 4,
126	DictationTriggered = 1 << 5,	126	DictationTriggered = 1 << 5,
127	DoNotUpdateAppearance = 1 << 6,	127	DoNotRevealSelection = 1 << 6,
128	DoNotRevealSelection = 1 << 7,
129	};	128	};
130	typedef unsigned SetSelectionOptions; // Union of values in SetSelectionOption and EUserTriggered	129	typedef unsigned SetSelectionOptions; // Union of values in SetSelectionOption and EUserTriggered
131	static inline EUserTriggered selectionOptionsToUserTriggered(SetSelectionOptions options)	130	static inline EUserTriggered selectionOptionsToUserTriggered(SetSelectionOptions options)
Lines 280-285 public: a/Source/WebCore/editing/FrameSelection.h_sec2
280	private:	279	private:
281	enum EPositionType { START, END, BASE, EXTENT };	280	enum EPositionType { START, END, BASE, EXTENT };
282		281
		282	bool setSelectionWithoutUpdatingAppearance(const VisibleSelection&, SetSelectionOptions, CursorAlignOnScroll, TextGranularity, EUserTriggered);
		283
283	void respondToNodeModification(Node*, bool baseRemoved, bool extentRemoved, bool startRemoved, bool endRemoved);	284	void respondToNodeModification(Node*, bool baseRemoved, bool extentRemoved, bool startRemoved, bool endRemoved);
284	TextDirection directionOfEnclosingBlock();	285	TextDirection directionOfEnclosingBlock();
285	TextDirection directionOfSelection();	286	TextDirection directionOfSelection();
Lines 302-307 private: a/Source/WebCore/editing/FrameSelection.h_sec3
302		303
303	#if HAVE(ACCESSIBILITY)	304	#if HAVE(ACCESSIBILITY)
304	void notifyAccessibilityForSelectionChange();	305	void notifyAccessibilityForSelectionChange();
		306	#else
		307	void notifyAccessibilityForSelectionChange() { }
305	#endif	308	#endif
306		309
307	void updateSelectionCachesIfSelectionIsInsideTextFormControl(EUserTriggered);	310	void updateSelectionCachesIfSelectionIsInsideTextFormControl(EUserTriggered);

Return to Bug 128421

Lines 250-260 void FrameSelection::setSelectionByMouseIfDifferent(const VisibleSelection& pass a/Source/WebCore/editing/FrameSelection.cpp_sec1
- a/Source/WebCore/editing/FrameSelection.cpp -18 / +25 lines
250	setSelection(newSelection, UserTriggered \| DoNotRevealSelection \| CloseTyping \| ClearTypingStyle, AlignCursorOnScrollIfNeeded, granularity);	250	setSelection(newSelection, UserTriggered \| DoNotRevealSelection \| CloseTyping \| ClearTypingStyle, AlignCursorOnScrollIfNeeded, granularity);
251	}	251	}
252		252
253	void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity)	253	bool FrameSelection::setSelectionWithoutUpdatingAppearance(const VisibleSelection& newSelection, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity, EUserTriggered userTriggered)
254	{	254	{
255	bool closeTyping = options & CloseTyping;	255	bool closeTyping = options & CloseTyping;
256	bool shouldClearTypingStyle = options & ClearTypingStyle;	256	bool shouldClearTypingStyle = options & ClearTypingStyle;
257	EUserTriggered userTriggered = selectionOptionsToUserTriggered(options);
258		257
259	VisibleSelection s = newSelection;	258	VisibleSelection s = newSelection;
260	if (shouldAlwaysUseDirectionalSelection(m_frame))	259	if (shouldAlwaysUseDirectionalSelection(m_frame))
Lines 262-268 void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec a/Source/WebCore/editing/FrameSelection.cpp_sec2
262		261
263	if (!m_frame) {	262	if (!m_frame) {
264	m_selection = s;	263	m_selection = s;
265	return;	264	return false;
266	}	265	}
267		266
268	// <http://bugs.webkit.org/show_bug.cgi?id=23464>: Infinite recursion at FrameSelection::setSelection	267	// <http://bugs.webkit.org/show_bug.cgi?id=23464>: Infinite recursion at FrameSelection::setSelection
Lines 277-283 void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec a/Source/WebCore/editing/FrameSelection.cpp_sec3
277	// the frame is about to be destroyed. If this is the case, clear our selection.	276	// the frame is about to be destroyed. If this is the case, clear our selection.
278	if (guard->hasOneRef() && !m_selection.isNonOrphanedCaretOrRange())	277	if (guard->hasOneRef() && !m_selection.isNonOrphanedCaretOrRange())
279	clear();	278	clear();
280	return;	279	return false;
281	}	280	}
282	}	281	}
283		282
Lines 296-302 void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec a/Source/WebCore/editing/FrameSelection.cpp_sec4
296	if (m_selection == s) {	295	if (m_selection == s) {
297	// Even if selection was not changed, selection offsets may have been changed.	296	// Even if selection was not changed, selection offsets may have been changed.
298	updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);	297	updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);
299	return;	298	return false;
300	}	299	}
301		300
302	VisibleSelection oldSelection = m_selection;	301	VisibleSelection oldSelection = m_selection;
Lines 307-327 void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec a/Source/WebCore/editing/FrameSelection.cpp_sec5
307	if (!s.isNone() && !(options & DoNotSetFocus))	306	if (!s.isNone() && !(options & DoNotSetFocus))
308	setFocusedElementIfNeeded();	307	setFocusedElementIfNeeded();
309		308
310	if (!(options & DoNotUpdateAppearance)) {
311	#if ENABLE(TEXT_CARET)
312	m_frame->document()->updateLayoutIgnorePendingStylesheets();
313	#else
314	m_frame->document()->updateStyleIfNeeded();
315	#endif
316	updateAppearance();
317	}
318
319	// Always clear the x position used for vertical arrow navigation.	309	// Always clear the x position used for vertical arrow navigation.
320	// It will be restored by the vertical arrow navigation code if necessary.	310	// It will be restored by the vertical arrow navigation code if necessary.
321	m_xPosForVerticalArrowNavigation = NoXPosForVerticalArrowNavigation();	311	m_xPosForVerticalArrowNavigation = NoXPosForVerticalArrowNavigation();
322	selectFrameElementInParentIfFullySelected();	312	selectFrameElementInParentIfFullySelected();
323	updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);	313	updateSelectionCachesIfSelectionIsInsideTextFormControl(userTriggered);
324	m_frame->editor().respondToChangedSelection(oldSelection, options);	314	m_frame->editor().respondToChangedSelection(oldSelection, options);
		315	m_frame->document()->enqueueDocumentEvent(Event::create(eventNames().selectionchangeEvent, false, false));
		316
		317	return true;
		318	}
		319
		320	void FrameSelection::setSelection(const VisibleSelection& selection, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity)
		321	{
		322	EUserTriggered userTriggered = selectionOptionsToUserTriggered(options);
		323	if (!setSelectionWithoutUpdatingAppearance(selection, options, align, granularity, userTriggered))
		324	return;
		325
		326	#if ENABLE(TEXT_CARET)
		327	m_frame->document()->updateLayoutIgnorePendingStylesheets();
		328	#else
		329	m_frame->document()->updateStyleIfNeeded();
		330	#endif
		331	updateAppearance();
		332
325	if (userTriggered == UserTriggered && !(options & DoNotRevealSelection)) {	333	if (userTriggered == UserTriggered && !(options & DoNotRevealSelection)) {
326	ScrollAlignment alignment;	334	ScrollAlignment alignment;
327		335
Lines 332-341 void FrameSelection::setSelection(const VisibleSelection& newSelection, SetSelec a/Source/WebCore/editing/FrameSelection.cpp_sec6
332		340
333	revealSelection(alignment, RevealExtent);	341	revealSelection(alignment, RevealExtent);
334	}	342	}
335	#if HAVE(ACCESSIBILITY)	343
336	notifyAccessibilityForSelectionChange();	344	notifyAccessibilityForSelectionChange();
337	#endif
338	m_frame->document()->enqueueDocumentEvent(Event::create(eventNames().selectionchangeEvent, false, false));
339	}	345	}
340		346
341	static bool removingNodeRemovesPosition(Node* node, const Position& position)	347	static bool removingNodeRemovesPosition(Node* node, const Position& position)
Lines 1237-1243 void FrameSelection::prepareForDestruction() a/Source/WebCore/editing/FrameSelection.cpp_sec7
1237	if (view)	1243	if (view)
1238	view->clearSelection();	1244	view->clearSelection();
1239		1245
1240	setSelection(VisibleSelection(), CloseTyping \| ClearTypingStyle \| DoNotUpdateAppearance);	1246	setSelectionWithoutUpdatingAppearance(VisibleSelection(), CloseTyping \| ClearTypingStyle,
		1247	AlignCursorOnScrollIfNeeded, CharacterGranularity, NotUserTriggered);
1241	m_previousCaretNode.clear();	1248	m_previousCaretNode.clear();
1242	}	1249	}
1243		1250

Lines 1-3 a/Source/WebCore/ChangeLog_sec1
- a/Source/WebCore/ChangeLog +24 lines
		1	2014-02-07 Ryosuke Niwa <rniwa@webkit.org>
		2
		3	FrameSelection's destructor shouldn't notify accessibility
		4	https://bugs.webkit.org/show_bug.cgi?id=128421
		5
		6	Reviewed by NOBODY (OOPS!).
		7
		8	Extracted setSelectionWithoutUpdatingAppearance out of setSelection and called it in prepareForDestruction
		9	instead of setting DoNotUpdateAppearance option. This new function doesn't reveal selection or notify
		10	accessibility code in addition to not updating appearance.
		11
		12	Note that all implementations of notifyAccessibilityForSelectionChange in FrameSelectionAtk.cpp and
		13	FrameSelectionMac.mm exit early when the selection type is not caret or either start or end is null,
		14	which is already the case inside FrameSelection's destructor. In addition, revealSelection is never called
		15	when selection change was not triggered by user so there should be no behavioral change from this patch.
		16
		17	* editing/FrameSelection.cpp:
		18	(WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance):
		19	(WebCore::FrameSelection::setSelection):
		20	(WebCore::FrameSelection::prepareForDestruction):
		21	* editing/FrameSelection.h:
		22	(WebCore::FrameSelection::notifyAccessibilityForSelectionChange): Added the trivial implementation in the case
		23	accessibility is disabled to tidy up call sites.
		24
1	2014-02-07 Roger Fong <roger_fong@apple.com>	25	2014-02-07 Roger Fong <roger_fong@apple.com>
2		26
3	CGContextGetUserSpaceToDeviceSpaceTransform returns the wrong value on Windows.	27	CGContextGetUserSpaceToDeviceSpaceTransform returns the wrong value on Windows.