Bug 52997

Summary: [GTK] DRT's TextInputController is unimplemented on GTK
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Initial implementation of TextInputController
none
Updated patch none

Description Alejandro G. Castro 2011-01-24 00:55:25 PST 
GTK needs support for the TextInputController in order to pass some tests.
Comment 1 Alejandro G. Castro 2011-01-24 01:13:16 PST 
Tests skipped due to this reason:

fast/forms/input-maxlength-ime-completed.html
fast/forms/input-maxlength-ime-preedit.html
fast/text/international/thai-cursor-position.html
fast/events/ime-composition-events-001.html
fast/dom/tab-in-right-alignment.html
editing/input/ime-composition-clearpreedit.html
svg/text/caret-in-svg-text.xhtml
editing/inserting/insert-composition-whitespace.html

New test skipped today:

http://trac.webkit.org/changeset/76489
Comment 2 Carlos Garcia Campos 2011-01-28 06:33:22 PST 
Created attachment 80443 [details]
Initial implementation of TextInputController

With this initial implementation all tests mentioned here pass now, except svg/text/caret-in-svg-text.xhtml that still fails, I don't know why though.
Comment 3 Martin Robinson 2011-01-28 09:36:20 PST 
Comment on attachment 80443 [details]
Initial implementation of TextInputController

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

This is awesome! I'd prefer you to try to go through the im-context where possible so that we have adequate testing on the WebKit layer. Does this also need "doCommand"? Other than that, it looks great!

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:471
> +void DumpRenderTreeSupportGtk::setComposition(WebKitWebView* webView, const gchar* text, gint start, gint end)

Please use char and int here, since this isn't public API.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:489
> +    String compositionString = String::fromUTF8(text);
> +    Vector<CompositionUnderline> underlines;
> +    underlines.append(CompositionUnderline(0, compositionString.length(), Color(0, 0, 0), false));
> +    editor->setComposition(compositionString, underlines, start, end);

If possible I think I'd either like to see this touch the GtkIMContext of the WebView either here or below. That way this will also test the WebKit layer.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:492
> +void DumpRenderTreeSupportGtk::confirmComposition(WebKitWebView* webView, const gchar* text)

ghcar -> char

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:510
> +    if (editor->hasComposition()) {
> +        if (text)
> +            editor->confirmComposition(String::fromUTF8(text));
> +        else
> +            editor->confirmComposition();
> +    } else
> +        editor->insertText(String::fromUTF8(text), 0);

Same comment as above.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:513
> +bool DumpRenderTreeSupportGtk::firstRectForCharacterRange(WebKitWebView* webView, gint location, gint length, GdkRectangle* rect)

gint -> int. It's fine to keep GdkRectangle since it's the only rect type that DumpRenderTree and WebKit share.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:540
> +bool DumpRenderTreeSupportGtk::selectedRange(WebKitWebView* webView, gint* start, gint* end)

Ditto.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:562
> +    RefPtr<Range> testRange = Range::create(scope->document(), scope, 0, range->startContainer(), range->startOffset());
> +    ASSERT(testRange->startContainer() == scope);
> +    *start = TextIterator::rangeLength(testRange.get());
> +
> +    ExceptionCode ec;
> +    testRange->setEnd(range->endContainer(), range->endOffset(), ec);
> +    ASSERT(testRange->startContainer() == scope);
> +    *end = TextIterator::rangeLength(testRange.get());

GTK+ doesn't seem to have the equivalent of seletion events the same way Qt and Mac do, so we must do this here. :)

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:76
> +    static void setComposition(WebKitWebView*, const gchar* text, gint start, gint end);
> +    static void confirmComposition(WebKitWebView*, const gchar* text);
> +    static bool firstRectForCharacterRange(WebKitWebView*, gint location, gint length, GdkRectangle*);
> +    static bool selectedRange(WebKitWebView*, gint* start, gint* end);

These should probably just be static functions in TextInputController.h

> Tools/DumpRenderTree/gtk/TextInputController.cpp:65
> +    g_return_val_if_fail((!exception || !*exception), JSValueMakeUndefined(context));
> +
> +    DumpRenderTreeSupportGtk::setComposition(view, stringBuffer.get(), start, end);
> +
> +    return JSValueMakeUndefined(context);

Can you just use the im-context property from the WebView and emit the preedit-changed signal?

> Tools/DumpRenderTree/gtk/TextInputController.cpp:87
> +static JSValueRef insertTextCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> +{
> +    WebKitWebView* view = webkit_web_frame_get_web_view(mainFrame);
> +    if (!view)
> +        return JSValueMakeUndefined(context);
> +
> +    if (argumentCount < 1)
> +        return JSValueMakeUndefined(context);
> +
> +    JSStringRef string = JSValueToStringCopy(context, arguments[0], exception);
> +    g_return_val_if_fail((!exception || !*exception), JSValueMakeUndefined(context));
> +
> +    size_t bufferSize = JSStringGetMaximumUTF8CStringSize(string);
> +    GOwnPtr<gchar> stringBuffer(static_cast<gchar*>(g_malloc(bufferSize)));
> +    JSStringGetUTF8CString(string, stringBuffer.get(), bufferSize);
> +    JSStringRelease(string);
> +
> +    DumpRenderTreeSupportGtk::confirmComposition(view, stringBuffer.get());
> +
> +    return JSValueMakeUndefined(context);

Is it possible just to get the im-context via the im-context property on the WebView and then emit the "commit" signal?
Comment 4 Carlos Garcia Campos 2011-01-28 10:00:23 PST 
(In reply to comment #3)
> (From update of attachment 80443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80443&action=review
> 
> This is awesome! I'd prefer you to try to go through the im-context where possible so that we have adequate testing on the WebKit layer. Does this also need "doCommand"? Other than that, it looks great!

Yes, well, that's why the changelog says "initial" implementation, I added the minimum stuff to fix the tests mentioned in this bug report, but there are several TextInputController methods not implemented, like doCommand that we can add in future patches. 

> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:471
> > +void DumpRenderTreeSupportGtk::setComposition(WebKitWebView* webView, const gchar* text, gint start, gint end)
> 
> Please use char and int here, since this isn't public API.

Ok

> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:489
> > +    String compositionString = String::fromUTF8(text);
> > +    Vector<CompositionUnderline> underlines;
> > +    underlines.append(CompositionUnderline(0, compositionString.length(), Color(0, 0, 0), false));
> > +    editor->setComposition(compositionString, underlines, start, end);
> 
> If possible I think I'd either like to see this touch the GtkIMContext of the WebView either here or below. That way this will also test the WebKit layer.
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:492
> > +void DumpRenderTreeSupportGtk::confirmComposition(WebKitWebView* webView, const gchar* text)
> 
> ghcar -> char
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:510
> > +    if (editor->hasComposition()) {
> > +        if (text)
> > +            editor->confirmComposition(String::fromUTF8(text));
> > +        else
> > +            editor->confirmComposition();
> > +    } else
> > +        editor->insertText(String::fromUTF8(text), 0);
> 
> Same comment as above.
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:513
> > +bool DumpRenderTreeSupportGtk::firstRectForCharacterRange(WebKitWebView* webView, gint location, gint length, GdkRectangle* rect)
> 
> gint -> int. It's fine to keep GdkRectangle since it's the only rect type that DumpRenderTree and WebKit share.
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:540
> > +bool DumpRenderTreeSupportGtk::selectedRange(WebKitWebView* webView, gint* start, gint* end)
> 
> Ditto.
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:562
> > +    RefPtr<Range> testRange = Range::create(scope->document(), scope, 0, range->startContainer(), range->startOffset());
> > +    ASSERT(testRange->startContainer() == scope);
> > +    *start = TextIterator::rangeLength(testRange.get());
> > +
> > +    ExceptionCode ec;
> > +    testRange->setEnd(range->endContainer(), range->endOffset(), ec);
> > +    ASSERT(testRange->startContainer() == scope);
> > +    *end = TextIterator::rangeLength(testRange.get());
> 
> GTK+ doesn't seem to have the equivalent of seletion events the same way Qt and Mac do, so we must do this here. :)
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:76
> > +    static void setComposition(WebKitWebView*, const gchar* text, gint start, gint end);
> > +    static void confirmComposition(WebKitWebView*, const gchar* text);
> > +    static bool firstRectForCharacterRange(WebKitWebView*, gint location, gint length, GdkRectangle*);
> > +    static bool selectedRange(WebKitWebView*, gint* start, gint* end);
> 
> These should probably just be static functions in TextInputController.h

They are here because they use WebCore API.

> > Tools/DumpRenderTree/gtk/TextInputController.cpp:65
> > +    g_return_val_if_fail((!exception || !*exception), JSValueMakeUndefined(context));
> > +
> > +    DumpRenderTreeSupportGtk::setComposition(view, stringBuffer.get(), start, end);
> > +
> > +    return JSValueMakeUndefined(context);
> 
> Can you just use the im-context property from the WebView and emit the preedit-changed signal?
> 
> > Tools/DumpRenderTree/gtk/TextInputController.cpp:87
> > +static JSValueRef insertTextCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> > +{
> > +    WebKitWebView* view = webkit_web_frame_get_web_view(mainFrame);
> > +    if (!view)
> > +        return JSValueMakeUndefined(context);
> > +
> > +    if (argumentCount < 1)
> > +        return JSValueMakeUndefined(context);
> > +
> > +    JSStringRef string = JSValueToStringCopy(context, arguments[0], exception);
> > +    g_return_val_if_fail((!exception || !*exception), JSValueMakeUndefined(context));
> > +
> > +    size_t bufferSize = JSStringGetMaximumUTF8CStringSize(string);
> > +    GOwnPtr<gchar> stringBuffer(static_cast<gchar*>(g_malloc(bufferSize)));
> > +    JSStringGetUTF8CString(string, stringBuffer.get(), bufferSize);
> > +    JSStringRelease(string);
> > +
> > +    DumpRenderTreeSupportGtk::confirmComposition(view, stringBuffer.get());
> > +
> > +    return JSValueMakeUndefined(context);
> 
> Is it possible just to get the im-context via the im-context property on the WebView and then emit the "commit" signal?

I don't know how im-context works, I'll try to use it instead. Thanks for the comments!
Comment 5 Martin Robinson 2011-01-28 10:23:14 PST 
Comment on attachment 80443 [details]
Initial implementation of TextInputController

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

>> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:76
>> +    static bool selectedRange(WebKitWebView*, gint* start, gint* end);
> 
> These should probably just be static functions in TextInputController.h

Sorry! This comment was made in error.
Comment 6 Carlos Garcia Campos 2011-02-04 05:24:24 PST 
(In reply to comment #3)
> (From update of attachment 80443 [details])
> If possible I think I'd either like to see this touch the GtkIMContext of the WebView either here or below. That way this will also test the WebKit layer.

The problem is that it's not always possible to use the im-context, see below

> > Tools/DumpRenderTree/gtk/TextInputController.cpp:65
> > +    g_return_val_if_fail((!exception || !*exception), JSValueMakeUndefined(context));
> > +
> > +    DumpRenderTreeSupportGtk::setComposition(view, stringBuffer.get(), start, end);
> > +
> > +    return JSValueMakeUndefined(context);
> 
> Can you just use the im-context property from the WebView and emit the preedit-changed signal?

preedit-changed callback doesn't receive the start and end, so setComposition() is always called with 0, 0

frame->editor()->setComposition(preeditString, underlines, 0, 0);
 
> Is it possible just to get the im-context via the im-context property on the WebView and then emit the "commit" signal?

in this case it would probably be possible, but I'm not sure it makes sense to use it only in this case.
Comment 7 Carlos Garcia Campos 2011-02-04 06:34:41 PST 
Created attachment 81210 [details]
Updated patch

Updated patch using int/char instead of gint/gchar
Comment 8 Martin Robinson 2011-02-07 09:18:01 PST 
Comment on attachment 81210 [details]
Updated patch

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

Okay! It's too bad we cannot use the real IME API to do this, but I understand if it just isn't possible.

> LayoutTests/platform/gtk/Skipped:3776
>  # No TextInputController support
>  # https://bugs.webkit.org/show_bug.cgi?id=52997

I think it makes sense to remove this comment now.

> LayoutTests/platform/gtk/Skipped:3777
>  svg/text/caret-in-svg-text.xhtml

Please open a specific bug for this test explaining what feature is missing and add it above.
Comment 9 Carlos Garcia Campos 2011-02-08 02:21:14 PST 
Committed r77917: <http://trac.webkit.org/changeset/77917>