[GTK] Implementation of atk_editable_text_insert_text ignores 'length' parameter

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

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.

[Mario Sanchez Prada]

Created attachment 157941 [details]
Patch proposal

Attaching a simple patch to fix this issue

[Carlos Garcia Campos]

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))

[Mario Sanchez Prada]

Created attachment 157944 [details]
Patch proposal

I think Carlos had a point here. Attaching a new patch (tested)

[Martin Robinson]

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.

[Carlos Garcia Campos]

(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.

[Mario Sanchez Prada]

(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

[Mario Sanchez Prada]

Committed r125403: <http://trac.webkit.org/changeset/125403>