Skip to content
Permalink
Browse files
[WK2] Data interaction tests occasionally hit assertions in debug builds
https://bugs.webkit.org/show_bug.cgi?id=169002
<rdar://problem/30994806>

Reviewed by Tim Horton.

Source/WebCore:

Data interaction unit tests occasionally fail due to the UI process expecting the latest received EditorState to
contain post layout data, but finding that it does not in -[WKContentView selectedTextRange]. The incomplete
EditorStates in question are sent while performing a data interaction operation, due to transient changes in the
frame selection. The UI process does not need to (and should not) be informed of these selection changes at all.

We can fix this by preventing the editor client from responding to selection changes during data interaction
operation. This patch also renames setIgnoreCompositionSelectionChange to setIgnoreSelectionChanges to better
reflect the fact that it is used outside of the context of holding selection change updates during IME. We
already use this affordance in various places, such as TextIndicator (while taking a snapshot on iOS), in
FindController on iOS, and when replacing selected or dictated text. Additionally, there is no logic in
setIgnoreCompositionSelectionChange that limits its use to composition.

* editing/Editor.cpp:
(WebCore::Editor::cancelCompositionIfSelectionIsInvalid):
(WebCore::Editor::setComposition):
(WebCore::Editor::revealSelectionAfterEditingOperation):
(WebCore::Editor::setIgnoreSelectionChanges):
(WebCore::Editor::changeSelectionAfterCommand):
(WebCore::Editor::respondToChangedSelection):
(WebCore::Editor::setIgnoreCompositionSelectionChange): Deleted.
* editing/Editor.h:
(WebCore::Editor::ignoreSelectionChanges):
(WebCore::Editor::ignoreCompositionSelectionChange): Deleted.
* editing/mac/EditorMac.mm:
(WebCore::Editor::selectionWillChange):
* page/TextIndicator.cpp:
(WebCore::TextIndicator::createWithRange):

Source/WebKit/mac:

Renames setIgnoreCompositionSelectionChange to setIgnoreSelectionChanges. See WebCore ChangeLog for more details.

* WebView/WebHTMLView.mm:
(-[WebHTMLView _updateSelectionForInputManager]):
* WebView/WebView.mm:
(-[WebView updateTextTouchBar]):

Source/WebKit2:

Renames setIgnoreCompositionSelectionChange to setIgnoreSelectionChanges. See WebCore ChangeLog for more details.

* Shared/EditorState.cpp:
(WebKit::EditorState::encode):
(WebKit::EditorState::decode):
* Shared/EditorState.h:
* UIProcess/gtk/WebPageProxyGtk.cpp:
* UIProcess/ios/WebPageProxyIOS.mm:
(WebKit::WebPageProxy::editorStateChanged):
* UIProcess/mac/WebPageProxyMac.mm:
(WebKit::WebPageProxy::editorStateChanged):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::editorState):
(WebKit::WebPage::performDragControllerAction):
(WebKit::WebPage::setComposition):
(WebKit::WebPage::didChangeSelection):
* WebProcess/WebPage/ios/FindControllerIOS.mm:
(WebKit::setSelectionChangeUpdatesEnabledInAllFrames):
(WebKit::FindController::willFindString):
(WebKit::FindController::didFailToFindString):
(WebKit::FindController::didHideFindIndicator):
(WebKit::setCompositionSelectionChangeEnabledInAllFrames): Deleted.
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::updateSelectionAppearance):
(WebKit::WebPage::replaceSelectedText):
(WebKit::WebPage::replaceDictatedText):

Tools:

Reenables and refactors data interaction tests.

* TestWebKitAPI/Tests/ios/DataInteractionTests.mm:
* TestWebKitAPI/ios/DataInteractionSimulator.h:
* TestWebKitAPI/ios/DataInteractionSimulator.mm:
(-[DataInteractionSimulator _resetSimulatedState]):
(-[DataInteractionSimulator runFrom:to:]):
(-[DataInteractionSimulator _advanceProgress]):


Canonical link: https://commits.webkit.org/186596@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@213902 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
whsieh committed Mar 14, 2017
1 parent 2854d12 commit f92a531bb8ea6a61293f63adb6ee3b70ddfbdbc9
Show file tree
Hide file tree
Showing 21 changed files with 160 additions and 81 deletions.
@@ -1,3 +1,39 @@
2017-03-14 Wenson Hsieh <wenson_hsieh@apple.com>

[WK2] Data interaction tests occasionally hit assertions in debug builds
https://bugs.webkit.org/show_bug.cgi?id=169002
<rdar://problem/30994806>

Reviewed by Tim Horton.

Data interaction unit tests occasionally fail due to the UI process expecting the latest received EditorState to
contain post layout data, but finding that it does not in -[WKContentView selectedTextRange]. The incomplete
EditorStates in question are sent while performing a data interaction operation, due to transient changes in the
frame selection. The UI process does not need to (and should not) be informed of these selection changes at all.

We can fix this by preventing the editor client from responding to selection changes during data interaction
operation. This patch also renames setIgnoreCompositionSelectionChange to setIgnoreSelectionChanges to better
reflect the fact that it is used outside of the context of holding selection change updates during IME. We
already use this affordance in various places, such as TextIndicator (while taking a snapshot on iOS), in
FindController on iOS, and when replacing selected or dictated text. Additionally, there is no logic in
setIgnoreCompositionSelectionChange that limits its use to composition.

* editing/Editor.cpp:
(WebCore::Editor::cancelCompositionIfSelectionIsInvalid):
(WebCore::Editor::setComposition):
(WebCore::Editor::revealSelectionAfterEditingOperation):
(WebCore::Editor::setIgnoreSelectionChanges):
(WebCore::Editor::changeSelectionAfterCommand):
(WebCore::Editor::respondToChangedSelection):
(WebCore::Editor::setIgnoreCompositionSelectionChange): Deleted.
* editing/Editor.h:
(WebCore::Editor::ignoreSelectionChanges):
(WebCore::Editor::ignoreCompositionSelectionChange): Deleted.
* editing/mac/EditorMac.mm:
(WebCore::Editor::selectionWillChange):
* page/TextIndicator.cpp:
(WebCore::TextIndicator::createWithRange):

2017-03-14 Antoine Quint <graouts@apple.com>

[Modern Media Controls] iOS may attempt to load fullscreen icon variants
@@ -1683,7 +1683,7 @@ bool Editor::cancelCompositionIfSelectionIsInvalid()
{
unsigned start;
unsigned end;
if (!hasComposition() || ignoreCompositionSelectionChange() || getCompositionSelection(start, end))
if (!hasComposition() || ignoreSelectionChanges() || getCompositionSelection(start, end))
return false;

cancelComposition();
@@ -1700,7 +1700,7 @@ void Editor::setComposition(const String& text, SetCompositionMode mode)
ASSERT(mode == ConfirmComposition || mode == CancelComposition);
UserTypingGestureIndicator typingGestureIndicator(m_frame);

setIgnoreCompositionSelectionChange(true);
setIgnoreSelectionChanges(true);

if (mode == CancelComposition)
ASSERT(text == emptyString());
@@ -1711,7 +1711,7 @@ void Editor::setComposition(const String& text, SetCompositionMode mode)
m_customCompositionUnderlines.clear();

if (m_frame.selection().isNone()) {
setIgnoreCompositionSelectionChange(false);
setIgnoreSelectionChanges(false);
return;
}

@@ -1731,7 +1731,7 @@ void Editor::setComposition(const String& text, SetCompositionMode mode)
TypingCommand::closeTyping(&m_frame);
}

setIgnoreCompositionSelectionChange(false);
setIgnoreSelectionChanges(false);
}

void Editor::setComposition(const String& text, const Vector<CompositionUnderline>& underlines, unsigned selectionStart, unsigned selectionEnd)
@@ -1740,7 +1740,7 @@ void Editor::setComposition(const String& text, const Vector<CompositionUnderlin

UserTypingGestureIndicator typingGestureIndicator(m_frame);

setIgnoreCompositionSelectionChange(true);
setIgnoreSelectionChanges(true);

// Updates styles before setting selection for composition to prevent
// inserting the previous composition text into text nodes oddly.
@@ -1750,7 +1750,7 @@ void Editor::setComposition(const String& text, const Vector<CompositionUnderlin
selectComposition();

if (m_frame.selection().isNone()) {
setIgnoreCompositionSelectionChange(false);
setIgnoreSelectionChanges(false);
return;
}

@@ -1843,7 +1843,7 @@ void Editor::setComposition(const String& text, const Vector<CompositionUnderlin
}
}

setIgnoreCompositionSelectionChange(false);
setIgnoreSelectionChanges(false);

#if PLATFORM(IOS)
client()->stopDelayingAndCoalescingContentChangeNotifications();
@@ -2775,7 +2775,7 @@ PassRefPtr<Range> Editor::rangeForPoint(const IntPoint& windowPoint)

void Editor::revealSelectionAfterEditingOperation(const ScrollAlignment& alignment, RevealExtentOption revealExtentOption)
{
if (m_ignoreCompositionSelectionChange)
if (m_ignoreSelectionChanges)
return;

#if PLATFORM(IOS)
@@ -2787,12 +2787,12 @@ void Editor::revealSelectionAfterEditingOperation(const ScrollAlignment& alignme
m_frame.selection().revealSelection(revealMode, alignment, revealExtentOption);
}

void Editor::setIgnoreCompositionSelectionChange(bool ignore, RevealSelection shouldRevealExistingSelection)
void Editor::setIgnoreSelectionChanges(bool ignore, RevealSelection shouldRevealExistingSelection)
{
if (m_ignoreCompositionSelectionChange == ignore)
if (m_ignoreSelectionChanges == ignore)
return;

m_ignoreCompositionSelectionChange = ignore;
m_ignoreSelectionChanges = ignore;
#if PLATFORM(IOS)
// FIXME: Should suppress selection change notifications during a composition change <https://webkit.org/b/38830>
if (!ignore)
@@ -2944,7 +2944,7 @@ void Editor::changeSelectionAfterCommand(const VisibleSelection& newSelection, F
// starts a new kill ring sequence, but we want to do these things (matches AppKit).
#if PLATFORM(IOS)
// FIXME: Should suppress selection change notifications during a composition change <https://webkit.org/b/38830>
if (m_ignoreCompositionSelectionChange)
if (m_ignoreSelectionChanges)
return;
#endif
if (selectionDidNotChangeDOMPosition && client())
@@ -3286,7 +3286,7 @@ void Editor::respondToChangedSelection(const VisibleSelection&, FrameSelection::
{
#if PLATFORM(IOS)
// FIXME: Should suppress selection change notifications during a composition change <https://webkit.org/b/38830>
if (m_ignoreCompositionSelectionChange)
if (m_ignoreSelectionChanges)
return;
#endif

@@ -334,8 +334,8 @@ class Editor {
const Vector<CompositionUnderline>& customCompositionUnderlines() const { return m_customCompositionUnderlines; }

enum class RevealSelection { No, Yes };
WEBCORE_EXPORT void setIgnoreCompositionSelectionChange(bool, RevealSelection shouldRevealExistingSelection = RevealSelection::Yes);
bool ignoreCompositionSelectionChange() const { return m_ignoreCompositionSelectionChange; }
WEBCORE_EXPORT void setIgnoreSelectionChanges(bool, RevealSelection shouldRevealExistingSelection = RevealSelection::Yes);
bool ignoreSelectionChanges() const { return m_ignoreSelectionChanges; }

WEBCORE_EXPORT PassRefPtr<Range> rangeForPoint(const IntPoint& windowPoint);

@@ -544,7 +544,7 @@ class Editor {
unsigned m_compositionStart;
unsigned m_compositionEnd;
Vector<CompositionUnderline> m_customCompositionUnderlines;
bool m_ignoreCompositionSelectionChange { false };
bool m_ignoreSelectionChanges { false };
bool m_shouldStartNewKillRingSequence { false };
bool m_shouldStyleWithCSS { false };
const std::unique_ptr<KillRing> m_killRing;
@@ -258,7 +258,7 @@ static void getImage(Element& imageElement, RefPtr<Image>& image, CachedImage*&

void Editor::selectionWillChange()
{
if (!hasComposition() || ignoreCompositionSelectionChange() || m_frame.selection().isNone())
if (!hasComposition() || ignoreSelectionChanges() || m_frame.selection().isNone())
return;

cancelComposition();
@@ -71,7 +71,7 @@ RefPtr<TextIndicator> TextIndicator::createWithRange(const Range& range, TextInd
Ref<Frame> protector(*frame);

#if PLATFORM(IOS)
frame->editor().setIgnoreCompositionSelectionChange(true);
frame->editor().setIgnoreSelectionChanges(true);
frame->selection().setUpdateAppearanceEnabled(true);
#endif

@@ -93,7 +93,7 @@ RefPtr<TextIndicator> TextIndicator::createWithRange(const Range& range, TextInd
frame->selection().setSelection(oldSelection);

#if PLATFORM(IOS)
frame->editor().setIgnoreCompositionSelectionChange(false, Editor::RevealSelection::No);
frame->editor().setIgnoreSelectionChanges(false, Editor::RevealSelection::No);
frame->selection().setUpdateAppearanceEnabled(false);
#endif

@@ -1,3 +1,18 @@
2017-03-14 Wenson Hsieh <wenson_hsieh@apple.com>

[WK2] Data interaction tests occasionally hit assertions in debug builds
https://bugs.webkit.org/show_bug.cgi?id=169002
<rdar://problem/30994806>

Reviewed by Tim Horton.

Renames setIgnoreCompositionSelectionChange to setIgnoreSelectionChanges. See WebCore ChangeLog for more details.

* WebView/WebHTMLView.mm:
(-[WebHTMLView _updateSelectionForInputManager]):
* WebView/WebView.mm:
(-[WebView updateTextTouchBar]):

2017-03-13 Anders Carlsson <andersca@apple.com>

Fix build warnings.
@@ -7211,7 +7211,7 @@ - (void)_updateSelectionForInputManager

[self _updateSecureInputState];

if (!coreFrame->editor().hasComposition() || coreFrame->editor().ignoreCompositionSelectionChange())
if (!coreFrame->editor().hasComposition() || coreFrame->editor().ignoreSelectionChanges())
return;

unsigned start;
@@ -9616,7 +9616,7 @@ - (void)updateTextTouchBar
}

if ([NSSpellChecker isAutomaticTextCompletionEnabled] && !_private->_isCustomizingTouchBar) {
BOOL shouldShowCandidateList = !coreFrame->selection().selection().isRange() || coreFrame->editor().ignoreCompositionSelectionChange();
BOOL shouldShowCandidateList = !coreFrame->selection().selection().isRange() || coreFrame->editor().ignoreSelectionChanges();
[self.candidateList updateWithInsertionPointVisibility:shouldShowCandidateList];
}

@@ -1,3 +1,38 @@
2017-03-14 Wenson Hsieh <wenson_hsieh@apple.com>

[WK2] Data interaction tests occasionally hit assertions in debug builds
https://bugs.webkit.org/show_bug.cgi?id=169002
<rdar://problem/30994806>

Reviewed by Tim Horton.

Renames setIgnoreCompositionSelectionChange to setIgnoreSelectionChanges. See WebCore ChangeLog for more details.

* Shared/EditorState.cpp:
(WebKit::EditorState::encode):
(WebKit::EditorState::decode):
* Shared/EditorState.h:
* UIProcess/gtk/WebPageProxyGtk.cpp:
* UIProcess/ios/WebPageProxyIOS.mm:
(WebKit::WebPageProxy::editorStateChanged):
* UIProcess/mac/WebPageProxyMac.mm:
(WebKit::WebPageProxy::editorStateChanged):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::editorState):
(WebKit::WebPage::performDragControllerAction):
(WebKit::WebPage::setComposition):
(WebKit::WebPage::didChangeSelection):
* WebProcess/WebPage/ios/FindControllerIOS.mm:
(WebKit::setSelectionChangeUpdatesEnabledInAllFrames):
(WebKit::FindController::willFindString):
(WebKit::FindController::didFailToFindString):
(WebKit::FindController::didHideFindIndicator):
(WebKit::setCompositionSelectionChangeEnabledInAllFrames): Deleted.
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::updateSelectionAppearance):
(WebKit::WebPage::replaceSelectedText):
(WebKit::WebPage::replaceDictatedText):

2017-03-14 Carlos Garcia Campos <cgarcia@igalia.com>

Unreviewed. Fix syntax error in GTK+ API docs.
@@ -32,7 +32,7 @@ namespace WebKit {

void EditorState::encode(IPC::Encoder& encoder) const
{
encoder << shouldIgnoreCompositionSelectionChange;
encoder << shouldIgnoreSelectionChanges;
encoder << selectionIsNone;
encoder << selectionIsRange;
encoder << isContentEditable;
@@ -56,7 +56,7 @@ void EditorState::encode(IPC::Encoder& encoder) const

bool EditorState::decode(IPC::Decoder& decoder, EditorState& result)
{
if (!decoder.decode(result.shouldIgnoreCompositionSelectionChange))
if (!decoder.decode(result.shouldIgnoreSelectionChanges))
return false;

if (!decoder.decode(result.selectionIsNone))
@@ -60,7 +60,7 @@ enum ListType {
};

struct EditorState {
bool shouldIgnoreCompositionSelectionChange { false };
bool shouldIgnoreSelectionChanges { false };

bool selectionIsNone { true }; // This will be false when there is a caret selection.
bool selectionIsRange { false };
@@ -79,7 +79,7 @@ void WebPageProxy::editorStateChanged(const EditorState& editorState)
{
m_editorState = editorState;

if (editorState.shouldIgnoreCompositionSelectionChange)
if (editorState.shouldIgnoreSelectionChanges)
return;
if (m_editorState.selectionIsRange)
WebPasteboardProxy::singleton().setPrimarySelectionOwner(focusedFrame());
@@ -1085,7 +1085,7 @@ static bool exceedsRenderTreeSizeSizeThreshold(uint64_t thresholdSize, uint64_t
if (couldChangeSecureInputState && !editorState.selectionIsNone)
m_pageClient.updateSecureInputState();

if (editorState.shouldIgnoreCompositionSelectionChange)
if (editorState.shouldIgnoreSelectionChanges)
return;

// We always need to notify the client on iOS to make sure the selection is redrawn,
@@ -574,7 +574,7 @@ static String webKitBundleVersionString()
if (couldChangeSecureInputState && !editorState.selectionIsNone)
m_pageClient.updateSecureInputState();

if (editorState.shouldIgnoreCompositionSelectionChange)
if (editorState.shouldIgnoreSelectionChanges)
return;

m_pageClient.selectionDidChange();
@@ -831,7 +831,7 @@ EditorState WebPage::editorState(IncludePostLayoutDataHint shouldIncludePostLayo
result.isContentRichlyEditable = selection.isContentRichlyEditable();
result.isInPasswordField = selection.isInPasswordField();
result.hasComposition = frame.editor().hasComposition();
result.shouldIgnoreCompositionSelectionChange = frame.editor().ignoreCompositionSelectionChange();
result.shouldIgnoreSelectionChanges = frame.editor().ignoreSelectionChanges();

#if PLATFORM(COCOA)
if (shouldIncludePostLayoutData == IncludePostLayoutDataHint::Yes && result.isContentEditable) {
@@ -3525,7 +3525,10 @@ void WebPage::performDragControllerAction(uint64_t action, const WebCore::DragDa
m_pendingDropExtensionsForFileUpload.append(extension);
}

auto& frame = m_page->focusController().focusedOrMainFrame();
frame.editor().setIgnoreSelectionChanges(true);
m_page->dragController().performDragOperation(dragData);
frame.editor().setIgnoreSelectionChanges(false);

// If we started loading a local file, the sandbox extension tracker would have adopted this
// pending drop sandbox extension. If not, we'll play it safe and clear it.
@@ -4861,9 +4864,9 @@ void WebPage::setComposition(const String& text, const Vector<CompositionUnderli

Element* scope = targetFrame->selection().selection().rootEditableElement();
RefPtr<Range> replacementRange = TextIterator::rangeFromLocationAndLength(scope, replacementStart, replacementLength);
targetFrame->editor().setIgnoreCompositionSelectionChange(true);
targetFrame->editor().setIgnoreSelectionChanges(true);
targetFrame->selection().setSelection(VisibleSelection(*replacementRange, SEL_DEFAULT_AFFINITY));
targetFrame->editor().setIgnoreCompositionSelectionChange(false);
targetFrame->editor().setIgnoreSelectionChanges(false);
}

targetFrame->editor().setComposition(text, underlines, selectionStart, selectionStart + selectionLength);
@@ -4957,7 +4960,7 @@ void WebPage::didChangeSelection()
// FIXME: This logic should be in WebCore.
// FIXME: Many changes that affect composition node do not go through didChangeSelection(). We need to do something when DOM manipulation affects the composition, because otherwise input method's idea about it will be different from Editor's.
// FIXME: We can't cancel composition when selection changes to NoSelection, but we probably should.
if (frame.editor().hasComposition() && !frame.editor().ignoreCompositionSelectionChange() && !frame.selection().isNone()) {
if (frame.editor().hasComposition() && !frame.editor().ignoreSelectionChanges() && !frame.selection().isNone()) {
frame.editor().cancelComposition();
discardedComposition();
} else
@@ -131,15 +131,15 @@ static Color highlightColor()
didHideFindIndicator();
}

static void setCompositionSelectionChangeEnabledInAllFrames(WebPage& page, bool enabled)
static void setSelectionChangeUpdatesEnabledInAllFrames(WebPage& page, bool enabled)
{
for (Frame* coreFrame = page.mainFrame(); coreFrame; coreFrame = coreFrame->tree().traverseNext())
coreFrame->editor().setIgnoreCompositionSelectionChange(enabled);
coreFrame->editor().setIgnoreSelectionChanges(enabled);
}

void FindController::willFindString()
{
setCompositionSelectionChangeEnabledInAllFrames(*m_webPage, true);
setSelectionChangeUpdatesEnabledInAllFrames(*m_webPage, true);
}

void FindController::didFindString()
@@ -157,12 +157,12 @@ static void setCompositionSelectionChangeEnabledInAllFrames(WebPage& page, bool

void FindController::didFailToFindString()
{
setCompositionSelectionChangeEnabledInAllFrames(*m_webPage, false);
setSelectionChangeUpdatesEnabledInAllFrames(*m_webPage, false);
}

void FindController::didHideFindIndicator()
{
setCompositionSelectionChangeEnabledInAllFrames(*m_webPage, false);
setSelectionChangeUpdatesEnabledInAllFrames(*m_webPage, false);
}

} // namespace WebKit

0 comments on commit f92a531

Please sign in to comment.