Holding down a key to choose an accented character should fire "insertReplacementText" input events
<rdar://problem/29019305>
Created attachment 293373 [details] Patch
Comment on attachment 293373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293373&action=review > LayoutTests/fast/events/input-event-insert-replacement-expected.txt:1 > +To manually test this, do things. Whoops, forgot to remove this! > LayoutTests/fast/events/input-event-insert-replacement.html:4 > + <p>To manually test this, do things.</p> Whoops, forgot to remove this!
Created attachment 293374 [details] Patch
Created attachment 293384 [details] Patch
Created attachment 293385 [details] Patch
Comment on attachment 293385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293385&action=review I’m going to say r=me but I see some things that seem a bit wrong to me. > Source/WebCore/editing/TypingCommand.cpp:150 > + , m_currentHTMLTextToInsert(createMarkup(Text::create(document, textToInsert))) Seems quite roundabout to create a text element just to turn it into markup. Can we slightly refactor the markup code so we can do this in a way that doesn’t involve creating and destroying a DOM node? > Source/WebCore/editing/TypingCommand.cpp:494 > + m_currentHTMLTextToInsert = createMarkup(Text::create(document(), text)); Ditto. > Source/WebCore/editing/TypingCommand.h:149 > + String m_currentPlainTextToInsert; > + String m_currentHTMLTextToInsert; This really seems strange. I don’t understand why we need m_currentHTMLTextToInsert. Is this about non-breaking spaces or something? > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:294 > +- (void)_insertText:(id)string replacementRange:(NSRange)replacementRange; Other methods in this file have availability macros. This should have one too. But also, is this really the right technique? What is the philosophy behind adding this? > Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:120 > + void insertText(JSStringRef, long location, long length); These should be int. IDL long maps to C++ int, not C++ long. > Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm:40 > + return [NSString stringWithCString:toWTFString(toWK(string)).utf8().data() encoding:NSUTF8StringEncoding]; The trip through WK, UTF-8, and C strings is unnecessarily complicated and inefficient. A simpler and more efficient way to write this function is: return (NSString *)adoptCF(JSStringCopyCFString(kCFAllocatorDefault, string)).autorelease(); Also, no reason to mention the argument type in the name of the function, since C++ has overloading, so you can just call it nsString I suppose.
Comment on attachment 293385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293385&action=review Thank you for looking at this! I'll hold off on landing until I find a better way to compute the html text without having to create and parse a text node. >> Source/WebCore/editing/TypingCommand.cpp:150 >> + , m_currentHTMLTextToInsert(createMarkup(Text::create(document, textToInsert))) > > Seems quite roundabout to create a text element just to turn it into markup. Can we slightly refactor the markup code so we can do this in a way that doesn’t involve creating and destroying a DOM node? Yes -- all we should really need to do here is escape special characters like '<' or '>'. I'll find a better way to do this. >> Source/WebCore/editing/TypingCommand.cpp:494 >> + m_currentHTMLTextToInsert = createMarkup(Text::create(document(), text)); > > Ditto. I'll find a way to do this that does not involve creating/destroying and parsing a Text node. >> Source/WebCore/editing/TypingCommand.h:149 >> + String m_currentHTMLTextToInsert; > > This really seems strange. I don’t understand why we need m_currentHTMLTextToInsert. Is this about non-breaking spaces or something? This string corresponds to the contents of DataTransfer's "text/html". m_currentHTMLTextToInsert will not be necessary once I come up with a cleaner way to get the "text/html" representation that doesn't involve creating and destroying a text node. >> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:294 >> +- (void)_insertText:(id)string replacementRange:(NSRange)replacementRange; > > Other methods in this file have availability macros. This should have one too. But also, is this really the right technique? What is the philosophy behind adding this? I'll add an availability macro. I originally tried to write a test that holds down the 'a' key and selects one of the accented characters (by pressing 1 afterwards or clicking the popover) but instead decided to mock the first call into WK code (i.e. insertText:replacementRange:) that is fired when selecting an accented character. My reasoning was that it would be less susceptible to flakiness due to UI changes or configuration changes on the bots, and might allow us to test other codepaths that call try to insert replacement text from the UI process via this method call. >> Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:120 >> + void insertText(JSStringRef, long location, long length); > > These should be int. IDL long maps to C++ int, not C++ long. I see -- fixed. >> Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm:40 >> + return [NSString stringWithCString:toWTFString(toWK(string)).utf8().data() encoding:NSUTF8StringEncoding]; > > The trip through WK, UTF-8, and C strings is unnecessarily complicated and inefficient. A simpler and more efficient way to write this function is: > > return (NSString *)adoptCF(JSStringCopyCFString(kCFAllocatorDefault, string)).autorelease(); > > Also, no reason to mention the argument type in the name of the function, since C++ has overloading, so you can just call it nsString I suppose. That's much cleaner! Done.
Created attachment 293418 [details] Patch for landing
Comment on attachment 293418 [details] Patch for landing Clearing flags on attachment: 293418 Committed r208143: <http://trac.webkit.org/changeset/208143>