need the surrounding text support for some language input

RESOLVED FIXED in mozilla1.9.3a4

Status

()

Core
Widget: Gtk
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Akira TAGOH, Assigned: Theppitak Karoonboonyanan)

Tracking

(Blocks: 1 bug, {intl})

Trunk
mozilla1.9.3a4
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 239629 [details] [diff] [review]
proposed patch to support the surrounding text gtk+ signals
(Reporter)

Updated

12 years ago
Version: unspecified → 1.5.0.x Branch
(Assignee)

Comment 2

11 years ago
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.

Updated

11 years ago
Blocks: 65896

Comment 3

10 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: intl
OS: Linux → All
Version: 1.5.0.x Branch → Trunk
(Assignee)

Comment 4

10 years ago
(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?
(Assignee)

Comment 5

9 years ago
Created attachment 387614 [details] [diff] [review]
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.
(Assignee)

Comment 6

9 years ago
Created attachment 389088 [details] [diff] [review]
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.
Attachment #387614 - Attachment is obsolete: true
(Assignee)

Comment 7

9 years ago
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?
Attachment #389088 - Flags: review?(peterv)

Comment 8

9 years ago
Adding Masayuki Nakano, Mozilla's i18n expert.
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 on attachment 389088 [details] [diff] [review]
Updated patch, fixing multi-line case

-'ing per comment 9.
Attachment #389088 - Flags: review?(peterv) → review-
(Assignee)

Comment 11

8 years ago
Created attachment 417894 [details] [diff] [review]
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<---
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?
(Assignee)

Comment 13

8 years ago
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.
Ah, it needs to extend the nsContentHandler and the related events.
(Assignee)

Comment 15

8 years ago
Created attachment 418540 [details] [diff] [review]
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]).
Attachment #417894 - Attachment is obsolete: true
bug 520732 and bug 528396 are fixed. you can create the patch right now.
(Assignee)

Comment 17

8 years ago
Created attachment 434167 [details] [diff] [review]
Updated patch after GtkIMModule split

Patch updated according to the GtkIMModule split. Please review.
Attachment #389088 - Attachment is obsolete: true
Attachment #418540 - Attachment is obsolete: true
Attachment #434167 - Flags: review?(masayuki)
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.
(Assignee)

Comment 19

8 years ago
(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.
(Assignee)

Comment 20

8 years ago
Created attachment 434238 [details] [diff] [review]
Updated patch after review

Updated patch. Please review again. Thanks.
Attachment #434167 - Attachment is obsolete: true
Attachment #434238 - Flags: review?(masayuki)
Attachment #434167 - Flags: review?(masayuki)
> +    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;
(Assignee)

Comment 22

8 years ago
Created attachment 434474 [details] [diff] [review]
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.
Attachment #434238 - Attachment is obsolete: true
Attachment #434474 - Flags: review?(masayuki)
Attachment #434238 - Flags: review?(masayuki)
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
Attachment #434474 - Flags: review?(masayuki) → review+
Assignee: nobody → thep
Component: General → Widget: Gtk
Product: Firefox → Core
QA Contact: general → gtk
Target Milestone: --- → mozilla1.9.3a4
(Assignee)

Comment 24

8 years ago
(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!
http://hg.mozilla.org/mozilla-central/rev/3d350995be5d

landed, thank you.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 26

8 years ago
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?
(Assignee)

Comment 27

8 years ago
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.)
tested with 20100329 nightly build on Mac OS,
looks like the patch doesn't integrated yet.
(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.
OS: All → Linux
Arthit, does the problem still occur on Mac for you? If so, can you file a bug for it?
Depends on: 757049
You need to log in before you can comment on or make changes to this bug.