Bug 212093

Summary: REGRESSION (r259930): Dictation marker at start of text is removed when added trailing whitespace is collapsed
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer
Priority: P2 Keywords: InRadar, Regression
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=212097
Bug Depends on: 210246    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Daniel Bates 2020-05-19 11:25:42 PDT 
See bug 210246, comment 17.
Comment 1 Radar WebKit Bug Importer 2020-05-19 11:40:16 PDT 
<rdar://problem/63406502>
Comment 2 Daniel Bates 2020-05-19 11:59:32 PDT 
Steps to reproduce using iOS Simulator build:

1 Add the following test case:

[[

#include "config.h"

#if PLATFORM(IOS_FAMILY)

#import "PlatformUtilities.h"
#import "TestInputDelegate.h"
#import "TestWKWebView.h"
#import "UIKitSPI.h"
#import "WKWebViewConfigurationExtras.h"
#import <WebKit/WKWebViewPrivate.h>
#import <wtf/unicode/CharacterNames.h>

namespace TestWebKitAPI {

TEST(InsertTextAlternatives, InsertTrailingSpaceWhitespaceRebalance)
{
    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 300, 300) configuration:configuration]);
    auto inputDelegate = adoptNS([[TestInputDelegate alloc] init]);
    [inputDelegate setFocusStartsInputSessionPolicyHandler:[] (WKWebView *, id <_WKFocusedElementInfo>) { return _WKFocusStartsInputSessionPolicyAllow; }];
    [webView _setInputDelegate:inputDelegate.get()];

    [webView synchronouslyLoadHTMLString:@"<body contenteditable='true'></body>"];
    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"document.body.focus()"];
    [[webView textInputContentView] insertText:@"hello" alternatives:@[@"yellow"] style:UITextAlternativeStyleNone];
    [[webView textInputContentView] insertText:@" "];
    [webView waitForNextPresentationUpdate];
    EXPECT_TRUE([[webView objectByEvaluatingJavaScript:@"internals.hasDictationAlternativesMarker(0, 5)"] boolValue]); // hello
}

} // namespace TestWebKitAPI

#endif // PLATFORM(IOS_FAMILY)

]]

2. Run it.

Then it fails.
Comment 3 Daniel Bates 2020-05-19 12:05:03 PDT 
Just FYI, I plan to add the test case ^^^ in the patch for bug 212097, flipping the EXPECT_TRUE() to FALSE to make the test pass for now.
Comment 4 Darin Adler 2020-05-19 12:06:45 PDT 
(In reply to Daniel Bates from comment #3)
> Just FYI, I plan to add the test case ^^^ in the patch for bug 212097,
> flipping the EXPECT_TRUE() to FALSE to make the test pass for now.

OK. But I don’t understand why. That will make it harder for me to land this fix.
Comment 5 Darin Adler 2020-05-19 12:57:40 PDT 
I guess I can just wait for you to land your fix and then rebase afterward.
Comment 6 Darin Adler 2020-05-19 13:01:14 PDT 
Created attachment 399761 [details]
Patch
Comment 7 Darin Adler 2020-05-19 13:02:05 PDT 
I put a fix here. Dan, feel free to grab the bug if you’d like to take it further.
Comment 8 Daniel Bates 2020-05-19 13:14:02 PDT 
(In reply to Darin Adler from comment #4)
> (In reply to Daniel Bates from comment #3)
> > Just FYI, I plan to add the test case ^^^ in the patch for bug 212097,
> > flipping the EXPECT_TRUE() to FALSE to make the test pass for now.
> 
> OK. But I don’t understand why. That will make it harder for me to land this
> fix.

Yes, but it makes my workflow easier 🙂. More importantly I am saving you from any agony should that test depend on my work. I don't think it does as I'm typing this...
Comment 9 Daniel Bates 2020-05-19 13:14:11 PDT 
(In reply to Darin Adler from comment #7)
> I put a fix here. Dan, feel free to grab the bug if you’d like to take it
> further.

Thank you!
Comment 10 Daniel Bates 2020-05-19 14:25:42 PDT 
Created attachment 399771 [details]
Patch

Alley-oop
Comment 11 Daniel Bates 2020-05-19 14:29:39 PDT 
Comment on attachment 399771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399771&action=review

Patch looks good. Needs change log for Tools change I did for you.

> Source/WebCore/dom/DocumentMarkerController.cpp:598
> +        unsigned targetStartOffset = clampTo<unsigned>(static_cast<int>(marker.startOffset()) + delta);

OK as-is. No change needed. Optimal solution would use auto because types in template args <-- is that the right word?

> Source/WebCore/dom/DocumentMarkerController.cpp:599
> +        unsigned targetEndOffset = clampTo<unsigned>(static_cast<int>(marker.endOffset()) + delta);

Ditto.
Comment 12 Darin Adler 2020-05-19 17:32:25 PDT 
Created attachment 399787 [details]
Patch
Comment 13 Darin Adler 2020-05-19 17:32:47 PDT 
Comment on attachment 399787 [details]
Patch

I plan to set commit-queue+ once the EWS tests all pass.
Comment 14 EWS 2020-05-19 20:13:53 PDT 
Committed r261903: <https://trac.webkit.org/changeset/261903>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399787 [details].