Inserting a space after inserting an accepted candidate scrolls the document and causes a flicker
<rdar://problem/28086237>
Created attachment 288936 [details] Patch
Attachment 288936 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/mac/CandidateTests.mm:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 288959 [details] Patch
Created attachment 288963 [details] Address iOS build failure.
Created attachment 288977 [details] Another attempt to address build failures.
Created attachment 288982 [details] Address build failures on 10.11 and earlier.
Comment on attachment 288982 [details] Address build failures on 10.11 and earlier. View in context: https://bugs.webkit.org/attachment.cgi?id=288982&action=review > Source/WebKit2/ChangeLog:24 > + new text. This causes a momentary range selection which the web process notifies the UI process about, prompting > + us to hide the candidates list. To address this, we put guards around this process of asynchronous text Beth fixed a similar bug using only WebProcess side changes today: https://trac.webkit.org/changeset/205983 Maybe you could do something similar (in WebPage::insertTextAsync, temporarychange a bit, don't send updates)? Would be a good bit less invasive, I think? > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:276 > +- (void)_handleAcceptedCandidate:(NSTextCheckingResult *)candidate; Availability macros are missing. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2444 > + if ([m_view respondsToSelector:@selector(_didHandleAcceptedCandidate)]) What? This is WebViewImpl, the view is WKView/WKWebView. How can this ever fail? > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3898 > + m_isTextInsertionReplacingSoftSpace = false; > interpretKeyEvent(event, ^(BOOL handledByInputMethod, const Vector<WebCore::KeypressCommand>& commands) { > ASSERT(!handledByInputMethod || commands.isEmpty()); > - m_page->handleKeyboardEvent(NativeWebKeyboardEvent(event, handledByInputMethod, commands)); > + m_page->handleKeyboardEvent(NativeWebKeyboardEvent(event, handledByInputMethod, m_isTextInsertionReplacingSoftSpace, commands)); This all seems surprisingly invasive.
Comment on attachment 288982 [details] Address build failures on 10.11 and earlier. View in context: https://bugs.webkit.org/attachment.cgi?id=288982&action=review >> Source/WebKit2/ChangeLog:24 >> + us to hide the candidates list. To address this, we put guards around this process of asynchronous text > > Beth fixed a similar bug using only WebProcess side changes today: https://trac.webkit.org/changeset/205983 > > Maybe you could do something similar (in WebPage::insertTextAsync, temporarychange a bit, don't send updates)? Would be a good bit less invasive, I think? We chatted about this briefly. I'm concerned about affecting code paths outside of what's needed to fix this issue. After poking around, I found that other than dictation, the setMarkedText:selectedRange:replacementRange: codepath also calls into insertTextAsync when typing in password fields. I think we should be wary of changing any behavior here (at least, given the time frame we have) since it's public API, and we don't know exactly how not letting the UI process know about the transient range selection during this async call might affect behavior in a lot of cases. >> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:276 >> +- (void)_handleAcceptedCandidate:(NSTextCheckingResult *)candidate; > > Availability macros are missing. Added. Thanks! >> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2444 >> + if ([m_view respondsToSelector:@selector(_didHandleAcceptedCandidate)]) > > What? This is WebViewImpl, the view is WKView/WKWebView. How can this ever fail? WKView doesn't implement _didHandleAcceptedCandidate (and I don't think it has a need to) >> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3898 >> + m_page->handleKeyboardEvent(NativeWebKeyboardEvent(event, handledByInputMethod, m_isTextInsertionReplacingSoftSpace, commands)); > > This all seems surprisingly invasive. I'll file a follow-up to this to address how we're firing editorState updates for transient range selections when editing. Again, I think making a change here might be dangerous -- Beth's change was localized to the side effects of calling WebPage::dictionaryPopupInfoForRange, but as I wrote above, I think a similar approach here would have a bigger effect since insertTextAsync is called by public API (setMarkedText:). We could make it so that we add an argument to WebPage::InsertTextAsync indicating whether to suppress editor state change updates until the end, but I don't think there's any real benefit to doing that over the approach taken in this patch.
Created attachment 289031 [details] New unit test, and addresses Tim's review comments.
> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3898 > > + m_isTextInsertionReplacingSoftSpace = false; > > interpretKeyEvent(event, ^(BOOL handledByInputMethod, const Vector<WebCore::KeypressCommand>& commands) { > > ASSERT(!handledByInputMethod || commands.isEmpty()); > > - m_page->handleKeyboardEvent(NativeWebKeyboardEvent(event, handledByInputMethod, commands)); > > + m_page->handleKeyboardEvent(NativeWebKeyboardEvent(event, handledByInputMethod, m_isTextInsertionReplacingSoftSpace, commands)); > > This all seems surprisingly invasive. Sorry, I realized after responding that this wasn't directed at the will/didInsertTextAsync change. Do you think I should add another constructor here without the replacesSoftSpace bool that just passes false, so the other places that call NativeWebKeyboardEvent() won't have to worry about replacesSoftSpace?
Created attachment 289035 [details] Suppresses EditorStateChanged while selecting text when replacing a soft space.
Comment on attachment 289035 [details] Suppresses EditorStateChanged while selecting text when replacing a soft space. View in context: https://bugs.webkit.org/attachment.cgi?id=289035&action=review > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:656 > + unsigned m_pendingAsynchronousTextInsertionCount { 0 }; I don't think you want this anymore. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:2444 > + if ([m_view respondsToSelector:@selector(_didHandleAcceptedCandidate)]) Add empty implementations to WKView too instead.
Created attachment 289073 [details] Patch for landing
Comment on attachment 289073 [details] Patch for landing Clearing flags on attachment: 289073 Committed r206033: <http://trac.webkit.org/changeset/206033>
All reviewed patches have been landed. Closing bug.