353776 - need the surrounding text support for some language input

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Akira TAGOH]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; ja; rv:1.8.0.6) Gecko/20060808 Fedora/1.5.0.6-2.fc5 Firefox/1.5.0.6 pango-text
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; ja; rv:1.8.0.6) Gecko/20060808 Fedora/1.5.0.6-2.fc5 Firefox/1.5.0.6 pango-text

Right now firefox/thunderbird doesn't deal with Input Method properly that uses the surrounding text signals from GTK+. then the unexpected characters appears in the text areas.

Reproducible: Always

Steps to Reproduce:
1.try to input the characters through Input Method that uses the surrounding text signals, such as scim-sinhala and scim-m17n.
2.
3.

Actual Results:  
broken characters appears

Expected Results:  
should shows the proper characters.

Comment 1

[Akira TAGOH]

Attachment: proposed patch to support the surrounding text gtk+ signals

Comment 2

[Theppitak Karoonboonyanan]

This bug applies to Thai XIM, too. (Try with XMODIFIERS="@im=BasicCheck" under th_TH locale.)

I try to test your patch, but it seems to need some update to apply to current trunk.

Comment 3

[Arthit Suriyawongkul]

BugAThon Thailand

on Firefox 3 Beta 5pre, Mac OS X and Windows also has this problem.

On Mac OS X, this behavior is consistent with native apps.
On Linux, depends on IM system the user used.

On Linux, the user can notice this very explicitly, visually.
On Mac OS X, the user can notice this as well, but the difference is very marginal, have to carefully spot it.
On Windows, the user cannot notice this at all, visually.

Comment 4

[Theppitak Karoonboonyanan]

(In reply to comment #3)

> On Linux, the user can notice this very explicitly, visually.
> On Mac OS X, the user can notice this as well, but the difference is very
> marginal, have to carefully spot it.
> On Windows, the user cannot notice this at all, visually.

Please be more specific on how it's visually noticed. By means of special text rendering or what?

Comment 5

[Theppitak Karoonboonyanan]

Attachment: Updated Akira's patch for trunk

I'd like this bug to be fixed, as it's one of the most requested feature for Thai users, next to line wrapping. The fix would obviously benefit related languages like Lao, Sinhala as well.

So, I've updated Akira's patch to apply to trunk again.

Comment 6

[Theppitak Karoonboonyanan]

Attachment: Updated patch, fixing multi-line case

My previous patch has a flaw which broke multi-line text area. It returned the whole text buffer like what was done in Akira's original patch, but I chose the API for the cursor position that returned the offset in the current node (paragraph), instead of relative to the whole buffer:

+NS_IMETHODIMP 
+nsPlaintextEditor::RetrieveSurrounding(nsTextEventReply* aReply)
+{
+  nsresult result;
+
+  nsCOMPtr<nsISelection> selection;
+  result = GetSelection(getter_AddRefs(selection));
+  if (NS_FAILED(result))
+    return result;
+
+  OutputToString(NS_LITERAL_STRING("text/plain"),
+                 nsIDocumentEncoder::OutputLFLineBreak |
+                 nsIDocumentEncoder::OutputBodyOnly |
+                 nsIDocumentEncoder::OutputRaw,
+                 aReply->theText);
+
+  nsCOMPtr<nsIDOMNode> startNode;
+  result = GetStartNodeAndOffset(selection,
+                                 address_of(startNode),
+                                 &aReply->cursorPos);
+
+  return result;
+}

So, I've adjusted it to also retrive text of current node, instead of the whole buffer:

+NS_IMETHODIMP 
+nsPlaintextEditor::RetrieveSurrounding(nsTextEventReply* aReply)
+{
+  nsresult result;
+
+  nsCOMPtr<nsISelection> selection;
+  result = GetSelection(getter_AddRefs(selection));
+  if (NS_FAILED(result))
+    return result;
+
+  nsCOMPtr<nsIDOMNode> startNode;
+  result = GetStartNodeAndOffset(selection,
+                                 address_of(startNode),
+                                 &aReply->cursorPos);
+  if (NS_FAILED(result))
+    return result;
+
+  result = startNode->GetNodeValue(aReply->theText);
+
+  return result;
+}

Now it works in multi-line text area, as well as multi-line editable HTML.

Comment 7

[Theppitak Karoonboonyanan]

Comment on attachment 389088 [details] [diff] [review]
Updated patch, fixing multi-line case

As I've tested the patch for a while, can I request for a review?

Comment 8

[Gen Kanai [:gen]]

Adding Masayuki Nakano, Mozilla's i18n expert.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The approach isn't good, we should use query content event to get the surrounding text. And also you can use set selection event and content command event to delete the text. So, you should be able to fix this bug by changing only nsWindow.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 389088 [details] [diff] [review]
Updated patch, fixing multi-line case

-'ing per comment 9.

Comment 11

[Theppitak Karoonboonyanan]

Attachment: First (broken) patch with pure nsWindow.cpp changes

Thanks for the advice. Now I'm getting back to it. I've got this first patch so far. It's not complete yet. Retrieving surrounding works, but deleting does not.

From my investigation, it fails in nsWindow::IMEDeleteText(). NS_SELECTION_SET event returns an error, with these debug messages:

---8<---
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/thep/vcs/mozilla_hg/mozilla-central-imsurr2/content/events/src/nsContentEventHandler.cpp, line 883
WARNING: NS_ENSURE_TRUE(selectionEvent.mSucceeded) failed: file /home/thep/vcs/mozilla_hg/mozilla-central-imsurr2/widget/src/gtk2/nsWindow.cpp, line 7081
---8<---

So, it fails in nsContentEventHandler::OnSelectionEvent(), on return from nsIMEStateManager::GetFocusSelectionAndRoot().

Checking nsIMEStateManager::GetFocusSelectionAndRoot(), I find it fails because nsIMEStateManager::sTextStateObserver == NULL, that is, there is no text currently gets the focus. How does this happen? Do I miss some step?

Note that I have also tried inserting this code before dispatching the NS_SELECTION_SET event, but it doesn't help:

---8<---
    nsWindow *containerWindow = GetContainerWindow();
    if (!gFocusWindow && containerWindow) {
        containerWindow->DispatchActivateEvent();
    }
---8<---

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note I'll separate the IME code from nsWindow, see bug 520732. Please wait the fix, I'm testing the patch now.

(In reply to comment #11)
> From my investigation, it fails in nsWindow::IMEDeleteText(). NS_SELECTION_SET
> event returns an error, with these debug messages:

Maybe, it's a bug of I found in bug 528396. Cannot fix the bug by the patch of bug 528396?

Comment 13

[Theppitak Karoonboonyanan]

Yes, the bug is gone with your patch in bug 528396. Now surrounding characters are deleted when requested. However, the amount it deletes is not what expected.

With select-and-delete scheme, text is deleted cluster-wise, rather than character-wise as expected by input methods. For normal text editing, cluster-wise deletion is expected (see bug 157546). But for IME, the correction can be combining mark substitution or alike, which needs finer access than cluster.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, it needs to extend the nsContentHandler and the related events.

Comment 15

[Theppitak Karoonboonyanan]

Attachment: Patch with nsSelectionEvent modification

OK. In this patch, I have added 'mExpandToClusterBoundary' member to nsSelectionEvent, with default value set to true to prevent brekage on other code.

Now it works with Thai context-sensitive IME. Note that the patch is to be applied on top of your patch in bug 528396 (attachment 415380 [details] [diff] [review]).

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

bug 520732 and bug 528396 are fixed. you can create the patch right now.

Comment 17

[Theppitak Karoonboonyanan]

Attachment: Updated patch after GtkIMModule split

Patch updated according to the GtkIMModule split. Please review.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Please make OnRetrieveSurroundingNative() and OnDeleteSurroundingNative() methods and you should process in there. That frees you and other developers from |aModule->|.

And in them, check aContext is same as the result of GetContext(). If they are not matched, you should do nothing and just return false.

Please define the stack variable near the first used line. The C style definition could cause unused variable.

Please add PR_LOG()s to the new methods like other methods. It helps us to debugging without the problematic IME.

> +nsresult
> +nsGtkIMModule::GetText(PRUnichar*& aText, PRInt32& aLength, PRInt32& aCursorPos)

Use nsAString& instead of aText and aLength. And the caller should send nsAutoString instance to this method.

> +    aCursorPos = querySelectedTextEvent.mReply.mOffset;

mOffset is unsigned, but aCursorPos is signed. You need to check whether (mOffset <= PR_INT32MAX) or not.

> +    queryTextContentEvent.InitForQueryTextContent(
> +        0, aCursorPos + querySelectedTextEvent.mReply.mString.Length() + 32
> +    );

Length() is PRUint32, so, you need to check here too.

And I'm not sure whether this is correct or not. If you should return an entire paragraph which has cursor, you can look the example in the code for Windows.
See nsIMM32Handler::HandleDocumentFeed():
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsIMM32Handler.cpp#1070

Note that if the focused editor is HTML editor, the text contents are too big.

So, if you change the behavior to such, GetText() should be GetCurrentParagraph() or something.

> +gboolean
> +nsGtkIMModule::OnRetrieveSurroundingCallback(GtkIMContext  *aContext,
> +                                             nsGtkIMModule *aModule)

> +    utf8_str[wbytes] = 0;
> +    p = utf8_str;
> +    while (cursorPos > 0 && *p != 0) {
> +        p = g_utf8_next_char(p);
> +        cursorPos--;
> +    }

How about this?

> utf8_str_until_cursor =
>     g_utf16_to_utf8((const gunichar2 *)uniStr, cursor, NULL, &cursorInUTF8, NULL);

Otherwise, look ok. I have a worry. Do Undo/Redo commands work expectantly for you? NS_CONTENT_COMMAND_DELETE might separate a transaction as two ones. If it's so, it should be another bug. And I guess it's not important.

Comment 19

[Theppitak Karoonboonyanan]

(In reply to comment #18)
> Please make OnRetrieveSurroundingNative() and OnDeleteSurroundingNative()
> methods and you should process in there. That frees you and other developers
> from |aModule->|.
> 
> And in them, check aContext is same as the result of GetContext(). If they are
> not matched, you should do nothing and just return false.
> 
> Please define the stack variable near the first used line. The C style
> definition could cause unused variable.
> 
> Please add PR_LOG()s to the new methods like other methods. It helps us to
> debugging without the problematic IME.

OK. All are done.

> > +nsresult
> > +nsGtkIMModule::GetText(PRUnichar*& aText, PRInt32& aLength, PRInt32& aCursorPos)
> 
> Use nsAString& instead of aText and aLength. And the caller should send
> nsAutoString instance to this method.

Also done.

> > +    aCursorPos = querySelectedTextEvent.mReply.mOffset;
> 
> mOffset is unsigned, but aCursorPos is signed. You need to check whether
> (mOffset <= PR_INT32MAX) or not.

I make aCursorPos unsigned instead. It can be.

> > +    queryTextContentEvent.InitForQueryTextContent(
> > +        0, aCursorPos + querySelectedTextEvent.mReply.mString.Length() + 32
> > +    );
> 
> Length() is PRUint32, so, you need to check here too.

All are made unsigned anyway.

> And I'm not sure whether this is correct or not. If you should return an entire
> paragraph which has cursor, you can look the example in the code for Windows.
> See nsIMM32Handler::HandleDocumentFeed():
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsIMM32Handler.cpp#1070
> 
> Note that if the focused editor is HTML editor, the text contents are too big.
> 
> So, if you change the behavior to such, GetText() should be
> GetCurrentParagraph() or something.

OK. I've imitated the code in GtkIMModule, and tested it.

> > +gboolean
> > +nsGtkIMModule::OnRetrieveSurroundingCallback(GtkIMContext  *aContext,
> > +                                             nsGtkIMModule *aModule)
> 
> > +    utf8_str[wbytes] = 0;
> > +    p = utf8_str;
> > +    while (cursorPos > 0 && *p != 0) {
> > +        p = g_utf8_next_char(p);
> > +        cursorPos--;
> > +    }
> 
> How about this?
> 
> > utf8_str_until_cursor =
> >     g_utf16_to_utf8((const gunichar2 *)uniStr, cursor, NULL, &cursorInUTF8, NULL);

I use this instead:

    gtk_im_context_set_surrounding(
        aContext, utf8_str, wbytes,
        g_utf8_offset_to_pointer(utf8_str, cursorPos) - utf8_str
    );

> Otherwise, look ok. I have a worry. Do Undo/Redo commands work expectantly for
> you? NS_CONTENT_COMMAND_DELETE might separate a transaction as two ones. If
> it's so, it should be another bug. And I guess it's not important.

I'm not sure about this. Never thought about it before. For me, it's OK. But users may think differently.

Updated patch is coming soon.

Comment 20

[Theppitak Karoonboonyanan]

Attachment: Updated patch after review

Updated patch. Please review again. Thanks.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +    if (NS_FAILED(GetCurrentParagraph(uniStr, cursorPos)))
> +        return FALSE;

Use {}.

> +    gtk_im_context_set_surrounding(
> +        aContext, utf8_str, wbytes,
> +        g_utf8_offset_to_pointer(utf8_str, cursorPos) - utf8_str
> +    );

gtk_im_context_set_surrounding(aContext, utf8_str, wbytes,
    g_utf8_offset_to_pointer(utf8_str, cursorPos) - utf8_str);

> +    return DeleteText(aOffset, (PRUint32)aNChars) == NS_OK;

If the result is NS_ERROR_* by NS_ENSURE_*(), any logs are not recorded, please calls PR_LOG() here if DeleteText() is failed.

So,

if (NS_SUCCEEDED(DeleteText(...))) {
    return TRUE;
}
PR_LOG(...);
return FALSE;

Comment 22

[Theppitak Karoonboonyanan]

Attachment: Updated patch as commented

(In reply to comment #21)
> > +    if (NS_FAILED(GetCurrentParagraph(uniStr, cursorPos)))
> > +        return FALSE;
> 
> Use {}.

I found {} omissions for the case of single 'return' elsewhere in the source tree. But I'm following GtkIMModule style as suggested BTW.

Other two are also corrected as suggested.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 434474 [details] [diff] [review]
Updated patch as commented

Great! Thank you for your work. Can you land the patch to tree? Or should I do it? If so, I'll do it tonight (JST).

(In reply to comment #22)
> Created an attachment (id=434474) [details]
> Updated patch as commented
> 
> (In reply to comment #21)
> > > +    if (NS_FAILED(GetCurrentParagraph(uniStr, cursorPos)))
> > > +        return FALSE;
> > 
> > Use {}.
> 
> I found {} omissions for the case of single 'return' elsewhere in the source
> tree. But I'm following GtkIMModule style as suggested BTW.

The rule was changed, so, old code doesn't have the {} in many points, see https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures

Comment 24

[Theppitak Karoonboonyanan]

(In reply to comment #23)
> Great! Thank you for your work. Can you land the patch to tree? Or should I do
> it? If so, I'll do it tonight (JST).

I don't have the commit right. So, please do it. Thanks.

> (In reply to comment #22)
> > I found {} omissions for the case of single 'return' elsewhere in the source
> > tree. But I'm following GtkIMModule style as suggested BTW.
> 
> The rule was changed, so, old code doesn't have the {} in many points, see
> https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures

Yes, I had also checked this page before deciding to follow your suggestion at last.

Thanks a lot for your reviews!

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/3d350995be5d

landed, thank you.

Comment 26

[Gen Kanai [:gen]]

Theppitak, Masayuki, thank you for your good collaboration and contributions here.

Once we have nightly builds for testing, could we have the Thai people on this bug please promote testing of this patch via blogs in Thailand, etc. so we can have a wide group of Thai users to test this before it goes into Firefox?

Comment 27

[Theppitak Karoonboonyanan]

I've just blogged about this (in Thai):
  http://thep.blogspot.com/2010/03/mozilla-ime-surrounding-text-support.html

Another related bug is Bug 425900. It should already be gone. Needs verification as well. (In fact, it should have been marked as a duplicate of this bug in the first place, but I didn't have the permission to do so.)

Comment 28

[Arthit Suriyawongkul]

tested with 20100329 nightly build on Mac OS,
looks like the patch doesn't integrated yet.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #28)
> tested with 20100329 nightly build on Mac OS,
> looks like the patch doesn't integrated yet.

The patch is only for Linux. If you have found a bug on Mac, please file a new bug.

Comment 30

[Dietrich Ayala (:dietrich)]

Arthit, does the problem still occur on Mac for you? If so, can you file a bug for it?