Bug 162009 - Inserting a space after inserting an accepted candidate scrolls the document and causes a flicker
Summary: Inserting a space after inserting an accepted candidate scrolls the document ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-15 00:08 PDT by Wenson Hsieh
Modified: 2016-09-16 11:16 PDT (History)
10 users (show)

See Also:


Attachments
Patch (35.92 KB, patch)
2016-09-15 00:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (78.75 KB, patch)
2016-09-15 09:45 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Address iOS build failure. (79.51 KB, patch)
2016-09-15 09:58 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Another attempt to address build failures. (79.63 KB, patch)
2016-09-15 11:36 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Address build failures on 10.11 and earlier. (80.04 KB, patch)
2016-09-15 12:00 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
New unit test, and addresses Tim's review comments. (83.32 KB, patch)
2016-09-15 20:42 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Suppresses EditorStateChanged while selecting text when replacing a soft space. (82.67 KB, patch)
2016-09-15 23:15 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for landing (83.26 KB, patch)
2016-09-16 10:43 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2016-09-15 00:08:01 PDT
Inserting a space after inserting an accepted candidate scrolls the document and causes a flicker
Comment 1 Wenson Hsieh 2016-09-15 00:11:17 PDT
<rdar://problem/28086237>
Comment 2 Wenson Hsieh 2016-09-15 00:22:54 PDT
Created attachment 288936 [details]
Patch
Comment 3 WebKit Commit Bot 2016-09-15 00:24:51 PDT
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.
Comment 4 Wenson Hsieh 2016-09-15 09:45:33 PDT
Created attachment 288959 [details]
Patch
Comment 5 Wenson Hsieh 2016-09-15 09:58:03 PDT
Created attachment 288963 [details]
Address iOS build failure.
Comment 6 Wenson Hsieh 2016-09-15 11:36:18 PDT
Created attachment 288977 [details]
Another attempt to address build failures.
Comment 7 Wenson Hsieh 2016-09-15 12:00:10 PDT
Created attachment 288982 [details]
Address build failures on 10.11 and earlier.
Comment 8 Tim Horton 2016-09-15 18:30:45 PDT
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 9 Wenson Hsieh 2016-09-15 20:31:47 PDT
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.
Comment 10 Wenson Hsieh 2016-09-15 20:42:04 PDT
Created attachment 289031 [details]
New unit test, and addresses Tim's review comments.
Comment 11 Wenson Hsieh 2016-09-15 20:47:54 PDT
> > 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?
Comment 12 Wenson Hsieh 2016-09-15 23:15:01 PDT
Created attachment 289035 [details]
Suppresses EditorStateChanged while selecting text when replacing a soft space.
Comment 13 Tim Horton 2016-09-16 10:23:51 PDT
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.
Comment 14 Wenson Hsieh 2016-09-16 10:43:38 PDT
Created attachment 289073 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2016-09-16 11:16:14 PDT
Comment on attachment 289073 [details]
Patch for landing

Clearing flags on attachment: 289073

Committed r206033: <http://trac.webkit.org/changeset/206033>
Comment 16 WebKit Commit Bot 2016-09-16 11:16:19 PDT
All reviewed patches have been landed.  Closing bug.