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

Daniel Bates
Reported 2020-05-19 11:25:42 PDT
See bug 210246, comment 17.
Attachments
Patch (2.71 KB, patch)
2020-05-19 13:01 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (4.23 KB, patch)
2020-05-19 14:25 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Patch (4.94 KB, patch)
2020-05-19 17:32 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-05-19 11:40:16 PDT
<rdar://problem/63406502>
Daniel Bates
Comment 2 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.
Daniel Bates
Comment 3 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.
Darin Adler
Comment 4 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.
Darin Adler
Comment 5 2020-05-19 12:57:40 PDT
I guess I can just wait for you to land your fix and then rebase afterward.
Darin Adler
Comment 6 2020-05-19 13:01:14 PDT
Created attachment 399761 [details] Patch
Darin Adler
Comment 7 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.
Daniel Bates
Comment 8 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...
Daniel Bates
Comment 9 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!
Daniel Bates
Comment 10 2020-05-19 14:25:42 PDT
Created attachment 399771 [details] Patch Alley-oop
Daniel Bates
Comment 11 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.
Darin Adler
Comment 12 2020-05-19 17:32:25 PDT
Created attachment 399787 [details] Patch
Darin Adler
Comment 13 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.
EWS
Comment 14 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].
Note You need to log in before you can comment on or make changes to this bug.