478030 - [TSF] The composition string is not displayed on WinXP + MS-IME

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

On WinXP + MS-IME, the composition string is not displayed. But on WinVista, I cannot reproduce this bug.

The cause may be:
> |OnUpdateComposition| is called with pRangeNew == null.
> Therefore, we don't update IME selection. Looks like we need to create the log
> of |InsertTextAtSelection|. And we should use it if pRangeNew is null.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I first reported this issue in the original TSF bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=88831#c107

Comment 2

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

taking.

Comment 3

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

Attachment: Patch v1.0

If pRangeNew is null, we can get the current range from pComposition.

This patch fixes the composition string invisible problem. But the composition string doesn't looks good for me. Because we fail to get the attrs. But it should be another bug.

Comment 4

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

Jim, if you don't/cannot want to review TSF support code, please tell me it. I'll find another reviewer for it (Maybe, Kimura-san). Otherwise please review the patch ASAP.

Comment 5

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

This bug should be a blocker. Because the composing string is most important information for Japanese IME. So, without this fix, Japanese testers cannot test enough.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #5)
> This bug should be a blocker. Because the composing string is most important
> information for Japanese IME. So, without this fix, Japanese testers cannot
> test enough.

Chinese users too.

Comment 7

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

Attachment: Patch v2.0

ok, the previous patch may be wrong approach.

OnUpdateComposition, the composing range should not be updated yet if TIP implements correctly.

SetSelection will be called after OnUpdateComposition for setting the IME selection to composition string. This is the cause of the XP selection broken bug of my previous comment.

Therefore, we should not dispatch text event in OnUpdateCompsition, instead of that, we should dispatch them in SetSelection. This patch works perfectly on Vista+MS-IME and XP+MS-IME.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Thank you for your work, Masayuki-san. It looks good.

1) On Vista I can see flickering with the patch using Microsoft Pinyin.
 Steps: Use Microsoft Pinyin on Vista, type in "nihaoma", press space, and then use left and right keys to move the cursor. There is flickering when the cursor moves. Can you reproduce this? It might be unimportant.

2) TestWinTSF is broken on a debug build because of NS_NOTREACHED in GetProperty. Can you include the fix in your patch?

Thanks.

Comment 9

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

(In reply to comment #8)
> 1) On Vista I can see flickering with the patch using Microsoft Pinyin.
>  Steps: Use Microsoft Pinyin on Vista, type in "nihaoma", press space, and then
> use left and right keys to move the cursor. There is flickering when the cursor
> moves. Can you reproduce this? It might be unimportant.

mmmm, I cannot reproduce it, but maybe it can be happen by my patch. Because SetSelection is called many times. I think I should create a new flag which is set to true in OnUpdateComposition, and SendTextEventForCompositionString should be called only when the flag is true. Then, the flag should be cleared.

> 2) TestWinTSF is broken on a debug build because of NS_NOTREACHED in
> GetProperty. Can you include the fix in your patch?

OK, I'll check it.

Comment 10

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

Attachment: Patch v3.0 (work in progress)

This is better. Text event is dispatched by both OnQueryComposition and SetSelection. nsTextStore cache the latest text event, and if current event is same as previous one, it doesn't dispatch the event again. This can suppress the flickering in some cases and stronger implementation for TIP implementation differences. However, this is not perfect solution for flickering. Because even when only caret position is changed, we need to dispatch new text event. I think that we should implement similar process to nsEditor. However, it should not be the scope of this bug. (and it's not important now)

Comment 11

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

Attachment: Patch v3.1

I expanded the test. Now, it's implemented the attribute property related interfaces.

Comment 12

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

Attachment: Patch v3.1.1

removed an unused variable. sorry for the spam.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 363309 [details] [diff] [review]
Patch v3.1.1

Thank you!

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (mLastDispatchedTextEvent->rangeArray)
+      delete [] mLastDispatchedTextEvent->rangeArray;
+    delete mLastDispatchedTextEvent;

Is there a reason we can't make the nsEvent destructor virtual, give the subclasses proper destructors and avoid having to repeat code like this (and similar code elsewhere)?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 363309 [details] [diff] [review]
Patch v3.1.1

+  mLastDispatchedTextEvent-> rangeArray = nsnull;

Unnecessary space.

+  // Clear the text event log

Not sure what you mean by "log" here. Just say "clear the saved text event" I think.

Comment 16

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

(In reply to comment #14)
> +    if (mLastDispatchedTextEvent->rangeArray)
> +      delete [] mLastDispatchedTextEvent->rangeArray;
> +    delete mLastDispatchedTextEvent;
> 
> Is there a reason we can't make the nsEvent destructor virtual, give the
> subclasses proper destructors and avoid having to repeat code like this (and
> similar code elsewhere)?

here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#6737

but maybe, the nsTextEvent can not be an owner of the buffer.

Comment 17

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

Attachment: Patch v3.1.2 (for check-in)

Comment 18

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

checked-in.
http://hg.mozilla.org/mozilla-central/rev/9174e1bbfa9f