Bug 164209 – Holding down a key to choose an accented character should fire "insertReplacementText" input events

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Wenson Hsieh]

Holding down a key to choose an accented character should fire "insertReplacementText" input events

[Wenson Hsieh]

<rdar://problem/29019305>

[Wenson Hsieh]

Created attachment 293373 [details]
Patch

[Wenson Hsieh]

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!

[Wenson Hsieh]

Created attachment 293374 [details]
Patch

[Wenson Hsieh]

Created attachment 293384 [details]
Patch

[Wenson Hsieh]

Created attachment 293385 [details]
Patch

[Darin Adler]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 293418 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 293418 [details]
Patch for landing

Clearing flags on attachment: 293418

Committed r208143: <http://trac.webkit.org/changeset/208143>