Bug 93804 - [GTK] Implementation of atk_editable_text_insert_text ignores 'length' parameter
Summary: [GTK] Implementation of atk_editable_text_insert_text ignores 'length' parameter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-08-13 01:40 PDT by Mario Sanchez Prada
Modified: 2012-08-13 02:43 PDT (History)
1 user (show)

See Also:


Attachments
Patch proposal (3.88 KB, patch)
2012-08-13 01:46 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (3.87 KB, patch)
2012-08-13 02:16 PDT, Mario Sanchez Prada
cgarcia: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2012-08-13 01:40:20 PDT
I just realized that the implementation of atk_editable_text_insert_text() in WebKitAccessibleInterfaceEditableText.cpp is wrong, as it's ignoring the 'length' parameter, causing the full string passed is always inserted in the text field.
Comment 1 Mario Sanchez Prada 2012-08-13 01:46:31 PDT
Created attachment 157941 [details]
Patch proposal

Attaching a simple patch to fix this issue
Comment 2 Carlos Garcia Campos 2012-08-13 02:08:15 PDT
Comment on attachment 157941 [details]
Patch proposal

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceEditableText.cpp:78
> +    if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(string).substring(0, length), false, 0))

I'm not sure this is always correct, according to the ATK api docs, length is the size in bytes, so maybe we should get the substring before converting the string from utf8 to utf16. You could do something like:

if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(CString(string, length)).data(), false, 0))
Comment 3 Mario Sanchez Prada 2012-08-13 02:16:19 PDT
Created attachment 157944 [details]
Patch proposal

I think Carlos had a point here. Attaching a new patch (tested)
Comment 4 Martin Robinson 2012-08-13 02:20:07 PDT
Comment on attachment 157944 [details]
Patch proposal

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

> Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceEditableText.cpp:78
> -    if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(string), false, 0))
> +    if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(CString(string, length).data()), false, 0))

Now might also be a good time to return early if the string is null, as the comment above suggests. It's always a good idea to be distrustful of data from outside the library.
Comment 5 Carlos Garcia Campos 2012-08-13 02:25:36 PDT
(In reply to comment #4)
> (From update of attachment 157944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157944&action=review
> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceEditableText.cpp:78
> > -    if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(string), false, 0))
> > +    if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(CString(string, length).data()), false, 0))
> 
> Now might also be a good time to return early if the string is null, as the comment above suggests. It's always a good idea to be distrustful of data from outside the library.

I agree it would be good to fix the FIXME, but probably as a separate bug, since it's a different issue.
Comment 6 Mario Sanchez Prada 2012-08-13 02:37:32 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 157944 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=157944&action=review
> > 
> > > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceEditableText.cpp:78
> > > -    if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(string), false, 0))
> > > +    if (document->frame()->editor()->insertTextWithoutSendingTextEvent(String::fromUTF8(CString(string, length).data()), false, 0))
> > 
> > Now might also be a good time to return early if the string is null, as the comment above suggests. It's always a good idea to be distrustful of data from outside the library.
> 
> I agree it would be good to fix the FIXME, but probably as a separate bug, since it's a different issue.

As it's just an early return, I'd go for it now, not in a separate bug.

I'll do it when landing
Comment 7 Mario Sanchez Prada 2012-08-13 02:43:20 PDT
Committed r125403: <http://trac.webkit.org/changeset/125403>