Skip to content

Commit

Permalink
Cherry-pick 6cd1f4e. rdar://127392270
Browse files Browse the repository at this point in the history
    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
  • Loading branch information
whsieh authored and Mohsin Qureshi committed May 7, 2024
1 parent b99e3c5 commit 54f915a
Show file tree
Hide file tree
Showing 17 changed files with 145 additions and 25 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<!DOCTYPE html>
<html>
<head>
<script src="../../../resources/ui-helper.js"></script>
<style>
[contenteditable] {
font-size: 16px;
width: 300px;
height: 100px;
caret-color: transparent;
}
</style>
</head>
<body>
<div contenteditable></div>
<script>
let editor = document.querySelector("[contenteditable]");
if (window.testRunner)
testRunner.waitUntilDone();
addEventListener("load", async () => {
editor.focus();
if (!window.testRunner)
return;

for (let character of [..."I want to celeb"]) {
await UIHelper.typeCharacter(character);
await UIHelper.ensurePresentationUpdate();
}
await UIHelper.setInlinePrediction("celebrate", 5);
await UIHelper.ensurePresentationUpdate();
testRunner.notifyDone();
});
</script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<!DOCTYPE html>
<html>
<head>
<script src="../../../resources/ui-helper.js"></script>
<style>
[contenteditable] {
font-size: 16px;
width: 300px;
height: 100px;
caret-color: transparent;
}
</style>
</head>
<body>
<div contenteditable></div>
<script>
let editor = document.querySelector("[contenteditable]");
if (window.testRunner)
testRunner.waitUntilDone();
addEventListener("load", async () => {
editor.focus();
if (!window.testRunner)
return;

for (let character of [..."I want to celeb"]) {
await UIHelper.typeCharacter(character);
await UIHelper.ensurePresentationUpdate();
}
await UIHelper.setInlinePrediction("celebrate", 5);
await UIHelper.ensurePresentationUpdate();
testRunner.notifyDone();
});

let typedText = "";
editor.addEventListener("keydown", event => {
if (event.key.length > 1)
return;

typedText += event.key === " " ? "\xa0" : event.key;
setTimeout(() => {
for (let childNode of [...editor.childNodes])
childNode.remove();
const newText = document.createTextNode(typedText);
editor.appendChild(newText);
getSelection().setPosition(newText, newText.length);
});
});
</script>
</body>
</html>
1 change: 1 addition & 0 deletions LayoutTests/platform/mac-wk2/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -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 ]

Expand Down
13 changes: 0 additions & 13 deletions Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/editing/Editor.h
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ class Editor final : public CanMakeCheckedPtr<Editor> {
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; }
Expand Down
3 changes: 3 additions & 0 deletions Source/WebCore/editing/VisibleSelection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
9 changes: 9 additions & 0 deletions Source/WebKit/UIProcess/API/mac/WKWebViewTestingMac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
9 changes: 8 additions & 1 deletion Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
Original file line number Diff line number Diff line change
Expand Up @@ -6914,7 +6914,14 @@ - (void)_updateTextInputTraits:(id<UITextInputTraits>)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];
Expand Down
5 changes: 4 additions & 1 deletion Source/WebKit/UIProcess/mac/WebViewImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 *);
Expand Down Expand Up @@ -818,7 +822,6 @@ ALLOW_DEPRECATED_DECLARATIONS_END
void handleRequestedCandidates(NSInteger sequenceNumber, NSArray<NSTextCheckingResult *> *candidates);

#if HAVE(INLINE_PREDICTIONS)
bool allowsInlinePredictions() const;
void showInlinePredictionsForCandidates(NSArray<NSTextCheckingResult *> *);
void showInlinePredictionsForCandidate(NSTextCheckingResult *, NSRange, NSRange);
#endif
Expand Down
10 changes: 2 additions & 8 deletions Source/WebKit/UIProcess/mac/WebViewImpl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 9 additions & 1 deletion Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
11 changes: 11 additions & 0 deletions Source/WebKit/WebProcess/WebPage/WebPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7599,6 +7599,8 @@ void WebPage::didCommitLoad(WebFrame* frame)

themeColorChanged();

m_lastNodeBeforeWritingSuggestions = { };

WebProcess::singleton().updateActivePages(m_processDisplayName);

updateMainFrameScrollOffsetPinning();
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions Source/WebKit/WebProcess/WebPage/WebPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -2023,6 +2023,8 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
void reapplyEditCommand(WebUndoStepID commandID);
void didRemoveEditCommand(WebUndoStepID commandID);

void updateLastNodeBeforeWritingSuggestions(const WebCore::KeyboardEvent&);

void findString(const String&, OptionSet<FindOptions>, uint32_t maxMatchCount, CompletionHandler<void(std::optional<WebCore::FrameIdentifier>, Vector<WebCore::IntRect>&&, uint32_t, int32_t, bool)>&&);
#if ENABLE(IMAGE_ANALYSIS)
void findStringIncludingImages(const String&, OptionSet<FindOptions>, uint32_t maxMatchCount, CompletionHandler<void(std::optional<WebCore::FrameIdentifier>, Vector<WebCore::IntRect>&&, uint32_t, int32_t, bool)>&&);
Expand Down Expand Up @@ -2746,6 +2748,8 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
bool m_appUsesCustomAccentColor { false };
#endif

WeakPtr<WebCore::Node, WebCore::WeakPtrImplWithEventTargetData> m_lastNodeBeforeWritingSuggestions;

bool m_textManipulationIncludesSubframes { false };
std::optional<Vector<WebCore::TextManipulationController::ExclusionRule>> m_textManipulationExclusionRules;

Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
3 changes: 3 additions & 0 deletions Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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:@{
Expand Down

0 comments on commit 54f915a

Please sign in to comment.