From 54f915a8ea7c5b8c46120d5114a73dd6123454e5 Mon Sep 17 00:00:00 2001 From: Wenson Hsieh Date: Tue, 7 May 2024 10:01:11 -0700 Subject: [PATCH] Cherry-pick 6cd1f4e75e31. rdar://127392270 Letters disappear randomly while typing in Stash when writing suggestions are enabled https://bugs.webkit.org/show_bug.cgi?id=273748 rdar://127392270 Reviewed by Richard Robinson. Stash's JavaScript listens for `keydown` events and, in response, replaces text nodes right before the selection in the editable container when writing comments. This interferes with both current implementations of writing suggestions (i.e. inline predictions), which depend on the text node before the user's selection being the same, as the user types. We can detect this case by computing and remembering the last node before the user's selection after each editing keyboard event (where writing suggestions would normally be inserted). If this text node changes in between the last key event and when sending the next editor state after changing the selection, then don't allow writing suggestions. * LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes-expected-mismatch.html: Added. * LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes.html: Added. Add a layout test to exercise the new heuristic. * LayoutTests/platform/mac-wk2/TestExpectations: * Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: Remove the internal `InlinePredictionsInAllEditableElementsEnabled` setting. This was only used to test writing suggestions on the web, before enabling it by default. * Source/WebCore/editing/Editor.h: * Source/WebCore/editing/VisibleSelection.cpp: (WebCore::VisibleSelection::canEnableWritingSuggestions const): Make the `writingsuggestions` state in editable elements follow the enclosing text form control, if applicable. This is needed to keep `WritingSuggestionsWebAPI.DefaultStateWithDisabledAutocomplete` passing after the change to consult `EditorState` in `-[WKContentView _updateTextInputTraits:]`, now that we don't solely depend on the initial focused element information. * Source/WebKit/UIProcess/API/mac/WKWebViewPrivateForTestingMac.h: * Source/WebKit/UIProcess/API/mac/WKWebViewTestingMac.mm: (-[WKWebView _allowsInlinePredictions]): * Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm: (-[WKContentView _updateTextInputTraits:]): Make the iOS codepath honor editor state's writing suggestions enablement flag (which may change on the fly, unlike focused element information). * Source/WebKit/UIProcess/mac/WebViewImpl.h: * Source/WebKit/UIProcess/mac/WebViewImpl.mm: (WebKit::WebViewImpl::allowsInlinePredictions const): Remove logic to honor the (now-removed) internal feature flag, and simplify the logic a bit. * Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm: (WebKit::WebPage::getPlatformEditorStateCommon const): Consult `m_lastNodeBeforeWritingSuggestions` when computing `canEnableWritingSuggestions` on the `EditorState`'s post-layout data. If the current node has changed since the last key event, avoid trying to compute and inject writing suggestions. * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::didCommitLoad): Reset `m_lastNodeBeforeWritingSuggestions`. (WebKit::WebPage::updateLastNodeBeforeWritingSuggestions): Add a helper method to update `m_lastNodeBeforeWritingSuggestions` after a `keydown`. * Source/WebKit/WebProcess/WebPage/WebPage.h: * Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm: (WebKit::WebPage::handleEditingKeyboardEvent): * Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm: (WebKit::WebPage::handleEditingKeyboardEvent): Call the helper method above when handling keyboard editing. * Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm: (WTR::UIScriptControllerMac::setInlinePrediction): Make this a no-op when `-_allowsInlinePredictions` is `NO`, so that we can (somewhat indirectly) test the new logic to avoid writing suggestions in the case where the text node before the selection keeps changing after each key event. Canonical link: https://commits.webkit.org/278407@main --- ...ons-if-text-changes-expected-mismatch.html | 35 +++++++++++++ ...ow-inline-predictions-if-text-changes.html | 50 +++++++++++++++++++ LayoutTests/platform/mac-wk2/TestExpectations | 1 + .../Preferences/UnifiedWebPreferences.yaml | 13 ----- Source/WebCore/editing/Editor.h | 2 +- Source/WebCore/editing/VisibleSelection.cpp | 3 ++ .../API/mac/WKWebViewPrivateForTestingMac.h | 1 + .../UIProcess/API/mac/WKWebViewTestingMac.mm | 9 ++++ .../UIProcess/ios/WKContentViewInteraction.mm | 9 +++- Source/WebKit/UIProcess/mac/WebViewImpl.h | 5 +- Source/WebKit/UIProcess/mac/WebViewImpl.mm | 10 +--- .../WebProcess/WebPage/Cocoa/WebPageCocoa.mm | 10 +++- Source/WebKit/WebProcess/WebPage/WebPage.cpp | 11 ++++ Source/WebKit/WebProcess/WebPage/WebPage.h | 4 ++ .../WebProcess/WebPage/ios/WebPageIOS.mm | 2 + .../WebProcess/WebPage/mac/WebPageMac.mm | 2 + .../mac/UIScriptControllerMac.mm | 3 ++ 17 files changed, 145 insertions(+), 25 deletions(-) create mode 100644 LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes-expected-mismatch.html create mode 100644 LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes.html diff --git a/LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes-expected-mismatch.html b/LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes-expected-mismatch.html new file mode 100644 index 000000000000..b2da0c64e3f0 --- /dev/null +++ b/LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes-expected-mismatch.html @@ -0,0 +1,35 @@ + + + + + + + +
+ + + diff --git a/LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes.html b/LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes.html new file mode 100644 index 000000000000..db2454e7743f --- /dev/null +++ b/LayoutTests/editing/input/mac/do-not-allow-inline-predictions-if-text-changes.html @@ -0,0 +1,50 @@ + + + + + + + +
+ + + diff --git a/LayoutTests/platform/mac-wk2/TestExpectations b/LayoutTests/platform/mac-wk2/TestExpectations index 3e8bb7d0351c..76fdda7660f3 100644 --- a/LayoutTests/platform/mac-wk2/TestExpectations +++ b/LayoutTests/platform/mac-wk2/TestExpectations @@ -1908,6 +1908,7 @@ webkit.org/b/272685 [ Ventura+ x86_64 ] imported/w3c/web-platform-tests/svg/pain # Inline predictions are only supported on Sonoma and later. [ Monterey Ventura ] editing/input/mac/show-inline-prediction-with-adjacent-text.html [ Skip ] [ Monterey Ventura ] editing/input/mac/writing-suggestions-textarea-multiple-lines.html [ Skip ] +[ Monterey Ventura ] editing/input/mac/do-not-allow-inline-predictions-if-text-changes.html [ Skip ] webkit.org/b/273012 [ Ventura+ x86_64 ] imported/w3c/web-platform-tests/svg/painting/reftests/percentage.svg [ ImageOnlyFailure ] diff --git a/Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml b/Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml index 21b4c1aa39db..36bec7bdff90 100644 --- a/Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml +++ b/Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml @@ -3098,19 +3098,6 @@ InlineMediaPlaybackRequiresPlaysInlineAttribute: WebCore: default: false -InlinePredictionsInAllEditableElementsEnabled: - type: bool - status: internal - humanReadableName: "Inline Text Predictions" - humanReadableDescription: "Enable Inline Text Predictions in all editable elements" - defaultValue: - WebKitLegacy: - default: false - WebKit: - default: false - WebCore: - default: false - InputTypeColorEnabled: type: bool status: embedder diff --git a/Source/WebCore/editing/Editor.h b/Source/WebCore/editing/Editor.h index b663a9cddb63..5daaaac2c44a 100644 --- a/Source/WebCore/editing/Editor.h +++ b/Source/WebCore/editing/Editor.h @@ -611,7 +611,7 @@ class Editor final : public CanMakeCheckedPtr { bool isPastingFromMenuOrKeyBinding() const { return m_pastingFromMenuOrKeyBinding; } bool isCopyingFromMenuOrKeyBinding() const { return m_copyingFromMenuOrKeyBinding; } - Node* nodeBeforeWritingSuggestions() const; + WEBCORE_EXPORT Node* nodeBeforeWritingSuggestions() const; Element* writingSuggestionsContainerElement() const; WritingSuggestionData* writingSuggestionData() const { return m_writingSuggestionData.get(); } bool isInsertingTextForWritingSuggestion() const { return m_isInsertingTextForWritingSuggestion; } diff --git a/Source/WebCore/editing/VisibleSelection.cpp b/Source/WebCore/editing/VisibleSelection.cpp index 5dc540e7217e..cee5263ac80e 100644 --- a/Source/WebCore/editing/VisibleSelection.cpp +++ b/Source/WebCore/editing/VisibleSelection.cpp @@ -696,6 +696,9 @@ bool VisibleSelection::isInPasswordField() const bool VisibleSelection::canEnableWritingSuggestions() const { + if (RefPtr formControl = enclosingTextFormControl(start())) + return formControl->isWritingSuggestionsEnabled(); + RefPtr containerNode = start().containerNode(); if (!containerNode) return false; diff --git a/Source/WebKit/UIProcess/API/mac/WKWebViewPrivateForTestingMac.h b/Source/WebKit/UIProcess/API/mac/WKWebViewPrivateForTestingMac.h index 022752323fcc..c293d829cc14 100644 --- a/Source/WebKit/UIProcess/API/mac/WKWebViewPrivateForTestingMac.h +++ b/Source/WebKit/UIProcess/API/mac/WKWebViewPrivateForTestingMac.h @@ -34,6 +34,7 @@ @property (nonatomic, readonly) BOOL _hasActiveVideoForControlsManager; @property (nonatomic, readonly) BOOL _shouldRequestCandidates; +@property (nonatomic, readonly) BOOL _allowsInlinePredictions; @property (nonatomic, readonly) NSMenu *_activeMenu; - (void)_requestControlledElementID; diff --git a/Source/WebKit/UIProcess/API/mac/WKWebViewTestingMac.mm b/Source/WebKit/UIProcess/API/mac/WKWebViewTestingMac.mm index 8d8507dded30..69d4226a1f01 100644 --- a/Source/WebKit/UIProcess/API/mac/WKWebViewTestingMac.mm +++ b/Source/WebKit/UIProcess/API/mac/WKWebViewTestingMac.mm @@ -75,6 +75,15 @@ - (BOOL)_shouldRequestCandidates return _impl->shouldRequestCandidates(); } +- (BOOL)_allowsInlinePredictions +{ +#if HAVE(INLINE_PREDICTIONS) + return _impl->allowsInlinePredictions(); +#else + return NO; +#endif +} + - (void)_insertText:(id)string replacementRange:(NSRange)replacementRange { [self insertText:string replacementRange:replacementRange]; diff --git a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm index dbd675951e26..4b4c757be9cc 100644 --- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm +++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm @@ -6914,7 +6914,14 @@ - (void)_updateTextInputTraits:(id)traits privateTraits.shortcutConversionType = _focusedElementInformation.elementType == WebKit::InputType::Password ? UITextShortcutConversionTypeNo : UITextShortcutConversionTypeDefault; #if HAVE(INLINE_PREDICTIONS) - traits.inlinePredictionType = (self.webView.configuration.allowsInlinePredictions || _page->preferences().inlinePredictionsInAllEditableElementsEnabled() || _focusedElementInformation.isWritingSuggestionsEnabled) ? UITextInlinePredictionTypeDefault : UITextInlinePredictionTypeNo; + bool allowsInlinePredictions = [&] { + if (self.webView.configuration.allowsInlinePredictions) + return true; + if (auto& state = _page->editorState(); state.hasPostLayoutData() && !_isFocusingElementWithKeyboard && !_page->waitingForPostLayoutEditorStateUpdateAfterFocusingElement()) + return state.postLayoutData->canEnableWritingSuggestions; + return _focusedElementInformation.isWritingSuggestionsEnabled; + }(); + traits.inlinePredictionType = allowsInlinePredictions ? UITextInlinePredictionTypeDefault : UITextInlinePredictionTypeNo; #endif [self _updateTextInputTraitsForInteractionTintColor]; diff --git a/Source/WebKit/UIProcess/mac/WebViewImpl.h b/Source/WebKit/UIProcess/mac/WebViewImpl.h index 437deb1690f0..c275305f4a8a 100644 --- a/Source/WebKit/UIProcess/mac/WebViewImpl.h +++ b/Source/WebKit/UIProcess/mac/WebViewImpl.h @@ -746,6 +746,10 @@ ALLOW_DEPRECATED_DECLARATIONS_END void removeTextIndicatorStyleForID(WTF::UUID); #endif +#if HAVE(INLINE_PREDICTIONS) + bool allowsInlinePredictions() const; +#endif + private: #if HAVE(TOUCH_BAR) void setUpTextTouchBar(NSTouchBar *); @@ -818,7 +822,6 @@ ALLOW_DEPRECATED_DECLARATIONS_END void handleRequestedCandidates(NSInteger sequenceNumber, NSArray *candidates); #if HAVE(INLINE_PREDICTIONS) - bool allowsInlinePredictions() const; void showInlinePredictionsForCandidates(NSArray *); void showInlinePredictionsForCandidate(NSTextCheckingResult *, NSRange, NSRange); #endif diff --git a/Source/WebKit/UIProcess/mac/WebViewImpl.mm b/Source/WebKit/UIProcess/mac/WebViewImpl.mm index 66a953863a2b..a1303ad69748 100644 --- a/Source/WebKit/UIProcess/mac/WebViewImpl.mm +++ b/Source/WebKit/UIProcess/mac/WebViewImpl.mm @@ -5291,18 +5291,12 @@ static BOOL shouldUseHighlightsForMarkedText(NSAttributedString *string) #if HAVE(INLINE_PREDICTIONS) bool WebViewImpl::allowsInlinePredictions() const { - const EditorState& editorState = m_page->editorState(); + auto& editorState = m_page->editorState(); if (editorState.hasPostLayoutData() && editorState.postLayoutData->canEnableWritingSuggestions) return NSSpellChecker.isAutomaticInlineCompletionEnabled; - if (!editorState.isContentEditable) - return false; - - if (!inlinePredictionsEnabled() && !m_page->preferences().inlinePredictionsInAllEditableElementsEnabled()) - return false; - - return NSSpellChecker.isAutomaticInlineCompletionEnabled; + return editorState.isContentEditable && inlinePredictionsEnabled() && NSSpellChecker.isAutomaticInlineCompletionEnabled; } void WebViewImpl::showInlinePredictionsForCandidate(NSTextCheckingResult *candidate, NSRange absoluteSelectedRange, NSRange oldRelativeSelectedRange) diff --git a/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm b/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm index 79063b4cbc4f..1dda99cde48f 100644 --- a/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm +++ b/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm @@ -783,8 +783,16 @@ } postLayoutData.baseWritingDirection = frame.editor().baseWritingDirectionForSelectionStart(); + postLayoutData.canEnableWritingSuggestions = [&] { + if (!selection.canEnableWritingSuggestions()) + return false; - postLayoutData.canEnableWritingSuggestions = selection.canEnableWritingSuggestions(); + if (!m_lastNodeBeforeWritingSuggestions) + return true; + + RefPtr currentNode = frame.editor().nodeBeforeWritingSuggestions(); + return !currentNode || m_lastNodeBeforeWritingSuggestions == currentNode.get(); + }(); } if (RefPtr editableRootOrFormControl = enclosingTextFormControl(selection.start()) ?: selection.rootEditableElement()) { diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp index ce7eb4dbcb55..0a9968cf7d7d 100644 --- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp +++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp @@ -7599,6 +7599,8 @@ void WebPage::didCommitLoad(WebFrame* frame) themeColorChanged(); + m_lastNodeBeforeWritingSuggestions = { }; + WebProcess::singleton().updateActivePages(m_processDisplayName); updateMainFrameScrollOffsetPinning(); @@ -9609,6 +9611,15 @@ void WebPage::frameNameWasChangedInAnotherProcess(FrameIdentifier frameID, const coreFrame->tree().setSpecifiedName(AtomString(frameName)); } +void WebPage::updateLastNodeBeforeWritingSuggestions(const KeyboardEvent& event) +{ + if (event.type() != eventNames().keydownEvent) + return; + + if (RefPtr frame = m_page->checkedFocusController()->focusedOrMainFrame()) + m_lastNodeBeforeWritingSuggestions = frame->editor().nodeBeforeWritingSuggestions(); +} + } // namespace WebKit #undef WEBPAGE_RELEASE_LOG diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h index 826f5a44fe66..280d24f901f3 100644 --- a/Source/WebKit/WebProcess/WebPage/WebPage.h +++ b/Source/WebKit/WebProcess/WebPage/WebPage.h @@ -2023,6 +2023,8 @@ class WebPage : public API::ObjectImpl, public IP void reapplyEditCommand(WebUndoStepID commandID); void didRemoveEditCommand(WebUndoStepID commandID); + void updateLastNodeBeforeWritingSuggestions(const WebCore::KeyboardEvent&); + void findString(const String&, OptionSet, uint32_t maxMatchCount, CompletionHandler, Vector&&, uint32_t, int32_t, bool)>&&); #if ENABLE(IMAGE_ANALYSIS) void findStringIncludingImages(const String&, OptionSet, uint32_t maxMatchCount, CompletionHandler, Vector&&, uint32_t, int32_t, bool)>&&); @@ -2746,6 +2748,8 @@ class WebPage : public API::ObjectImpl, public IP bool m_appUsesCustomAccentColor { false }; #endif + WeakPtr m_lastNodeBeforeWritingSuggestions; + bool m_textManipulationIncludesSubframes { false }; std::optional> m_textManipulationExclusionRules; diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm index 1fa9d6ef3c36..09692613d222 100644 --- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm +++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm @@ -605,6 +605,8 @@ static inline FloatRect adjustExposedRectForNewScale(const FloatRect& exposedRec if (handleKeyEventByRelinquishingFocusToChrome(event)) return true; + updateLastNodeBeforeWritingSuggestions(event); + // FIXME: Interpret the event immediately upon receiving it in UI process, without sending to WebProcess first. auto sendResult = WebProcess::singleton().parentProcessConnection()->sendSync(Messages::WebPageProxy::InterpretKeyEvent(editorState(ShouldPerformLayout::Yes), platformEvent->type() == PlatformKeyboardEvent::Type::Char), m_identifier); auto [eventWasHandled] = sendResult.takeReplyOr(false); diff --git a/Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm b/Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm index 764fc223effb..61110c736e13 100644 --- a/Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm +++ b/Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm @@ -324,6 +324,8 @@ static String commandNameForSelectorName(const String& selectorName) if (handleKeyEventByRelinquishingFocusToChrome(event)) return true; + updateLastNodeBeforeWritingSuggestions(event); + bool eventWasHandled = false; // Are there commands that could just cause text insertion if executed via Editor? diff --git a/Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm b/Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm index 42215c993281..83be9c4d20ad 100644 --- a/Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm +++ b/Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm @@ -458,6 +458,9 @@ static void playBackEvents(WKWebView *webView, UIScriptContext *context, NSStrin void UIScriptControllerMac::setInlinePrediction(JSStringRef jsText, unsigned startIndex) { #if HAVE(INLINE_PREDICTIONS) + if (!webView()._allowsInlinePredictions) + return; + auto fullText = jsText->string(); RetainPtr markedText = adoptNS([[NSMutableAttributedString alloc] initWithString:fullText.left(startIndex)]); [markedText appendAttributedString:adoptNS([[NSAttributedString alloc] initWithString:fullText.substring(startIndex) attributes:@{