Inserting a space after inserting an accepted candidate scrolls the document and causes a flicker

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

Inserting a space after inserting an accepted candidate scrolls the document and causes a flicker

[Wenson Hsieh]

<rdar://problem/28086237>

[Wenson Hsieh]

Created attachment 288936 [details]
Patch

[WebKit Commit Bot]

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.

[Wenson Hsieh]

Created attachment 288959 [details]
Patch

[Wenson Hsieh]

Created attachment 288963 [details]
Address iOS build failure.

[Wenson Hsieh]

Created attachment 288977 [details]
Another attempt to address build failures.

[Wenson Hsieh]

Created attachment 288982 [details]
Address build failures on 10.11 and earlier.

[Tim Horton]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 289031 [details]
New unit test, and addresses Tim's review comments.

[Wenson Hsieh]

> > 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?

[Wenson Hsieh]

Created attachment 289035 [details]
Suppresses EditorStateChanged while selecting text when replacing a soft space.

[Tim Horton]

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.

[Wenson Hsieh]

Created attachment 289073 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289073 [details]
Patch for landing

Clearing flags on attachment: 289073

Committed r206033: <http://trac.webkit.org/changeset/206033>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.