Closed Bug 55751 Opened 20 years ago Closed 15 years ago

Mnemonic doesn't work when IME is on

Categories

(Core :: Internationalization, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: kazhik, Assigned: masayuki)

References

Details

(Keywords: inputmethod, intl)

Attachments

(1 file, 23 obsolete files)

29.84 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
When IME is on, [Alt-F] - [O] doesn't work. [Alt-F] opens
[File] menu, but [O] is sent to a text field. It should open
Open File dialog.

Win32 build 2000100520 on Win98
I can reprodce this using today's trunk build on WinNT4-J.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reassign to yokoyama, cc to ftang, shanjian.
Assignee: nhotta → yokoyama
Status: NEW → ASSIGNED
Put rtm to keyword.
Keywords: rtm
Keywords: rtm
same problem happens on Linux
Keywords: intl, nsbeta1
Changed QA contact to ylong@netscape.com.
QA Contact: teruko → ylong
Updating the target milestone.
Target Milestone: --- → Future
Target Milestone: Future → mozilla0.9.1
Target Milestone: mozilla0.9.1 → mozilla1.0
Target Milestone: mozilla1.0 → Future
Blocks: 104166
The same is true of shortcuts in the context menu. 
OS: other → Windows XP
Assignee: yokoyama → masayuki
Status: ASSIGNED → NEW
Can this bug be fixed?
WinIE has same problem in the Editor(IE is disabling IME on other of Editor).
I think the way of to fix this bug is very difficult way.
We should disable IME on other of Editor.
This is same approach of WinIE.

But we have FAYT. If we disable IME on other of editor, we cannot use IME at FAYT.
Therefore, we should implement Findbar on Suite too.
And FAYT's editor of Firefox and Suite should tread to input for FAYT.

This solution is related Bug 113187.
Blocks: 113187
No longer blocks: 104166
Depends on: 259454
Severity: normal → major
OS: Windows XP → All
Priority: P3 → --
Hardware: PC → All
(In reply to comment #8)
> Can this bug be fixed?
> WinIE has same problem in the Editor(IE is disabling IME on other of Editor).

Does MS IE have the same problem? I can't reproduce it with MS IE (and Korean
IME in textarea like this bugzilla comment input area). BTW, can you explain
what  you meant by 'other of Editor'? 



IE has same problem.
On WinXP-ja SP2 + ATOK17, if the focus is set in editor and IME is active, Alt +
F does not work.(I can hear error sound.)

The 'other of editor' is the non-editor widget.
e.g., button, checkbox, menu-window and the canvas of the page.
(In reply to comment #10)
> IE has same problem.
> On WinXP-ja SP2 + ATOK17, if the focus is set in editor and IME is active, Alt +
> F does not work.(I can hear error sound.)

See comment #0. What's reported was that 'Alt-F' works (brings up the file
submenu) but 'O' (for file open) doesn't work. Anyway, MS IE doesn't have the
same problem with the standard Korean IME on Win 2k although it seems to have
the same problem with ATOK17. 

> What's reported was that 'Alt-F' works (brings up the file
> submenu) but 'O' (for file open) doesn't work.

Oh, you are right, however I think that these problem are same.
Because the cause of both problem is 'eating key events' by IME.
Therefore if non-editor widget disable IME, this problem cannot be reproduced on
non-editor widget.
Oops...

I found same problem on IE.
Execute the next steps.

1. Set focus editor(but exclude password field).
2. Turn on IME.
3. _Click_ "Favorites" by mouse.
4. Type 'A' or 'O'.

The favorites menu is not windows native menu.
This situation is same as Mozilla.

And I found new pattern. Following pattern is reproduced on Mozilla and IE.

1. Turn on IME.
2. Type Alt key.
3. Type F key.

The file menu is not opened by 'F' key.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: Future → mozilla1.9alpha
Attached patch Patch rv1.0 (obsolete) — Splinter Review
Attached patch Patch rv1.0 content/ (obsolete) — Splinter Review
Attachment #193818 - Flags: review?(peterv)
Attached patch Patch rv1.0 widget/ (obsolete) — Splinter Review
Attachment #193820 - Flags: review?(roc)
My patch makes IE like IME control on Windows(Other OSs are not implemented XP
IME API). On non-editor widgets, we don't use IME, so, these widgets should
disable IME if it has focus.

My patch hooks on focus content event. The focus content event has IME status
that's default value is 'Disabled'. When editor widget gets the focus, the
editor set 'Enable' status to the focus content event reply. ESM is setting IME
status on current widget according to event reply's IME status.
Priority: P2 → P1
This patch fixes bug 113187 too.
Attachment #193818 - Flags: review?(peterv) → review-
Attached patch Patch rv1.1 content/ (obsolete) — Splinter Review
Attachment #193818 - Attachment is obsolete: true
Attachment #193822 - Flags: review?(peterv)
Attached patch Patch rv1.1 (obsolete) — Splinter Review
Attachment #193817 - Attachment is obsolete: true
Attachment #193822 - Attachment description: Patch rv1.1 conetent/ → Patch rv1.1 content/
Comment on attachment 193822 [details] [diff] [review]
Patch rv1.1 content/

>Index: content/events/public/nsIPrivateFocusEvent.h
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998

this is wrong
Attachment #193819 - Flags: review?(timeless) → review+
Attachment #193822 - Flags: review?(peterv) → review-
Attached patch Patch rv1.2 content/ (obsolete) — Splinter Review
Attachment #193822 - Attachment is obsolete: true
Attachment #193895 - Flags: review?(peterv)
Attached patch Patch rv1.2 (obsolete) — Splinter Review
Attachment #193823 - Attachment is obsolete: true
Attachment #193895 - Flags: review?(peterv) → review-
Attached patch Patch rv1.3 content/ (obsolete) — Splinter Review
Attachment #193895 - Attachment is obsolete: true
Attachment #194107 - Flags: review?(peterv)
Attachment #193820 - Flags: review?(roc)
Attached patch Patch rv1.3 widget/ (obsolete) — Splinter Review
Attachment #193820 - Attachment is obsolete: true
Attachment #194108 - Flags: review?(roc)
Attached patch Patch rv1.3 (obsolete) — Splinter Review
Attachment #193896 - Attachment is obsolete: true
Attachment #194109 - Flags: review?
If this bug is fixed by current patch, there are two regressions.
But these regressions will be fixed easy. Don't worry.

regressions:
1. Cannot use IME in FAYT of Firefox.
2. Cannot use IME in FAYT of Suite.
Attachment #194107 - Flags: review?(peterv) → review-
Attached patch Patch rv1.4 content/ (obsolete) — Splinter Review
Attachment #194107 - Attachment is obsolete: true
Attachment #194191 - Flags: review?(peterv)
Attachment #194108 - Flags: review?(roc) → review-
Attached patch Patch rv1.4 widget/ (obsolete) — Splinter Review
Attachment #194108 - Attachment is obsolete: true
Attachment #194192 - Flags: review?(roc)
Attached patch Patch rv1.4 (obsolete) — Splinter Review
Attachment #194109 - Attachment is obsolete: true
Attached file testcase (obsolete) —
These cases are cleared by Patch rv1.4. But Firefox's FAYT problem still
happen.
Attachment #194191 - Flags: review?(peterv)
Is there a reason why you need to fix the regression in a separate patch?
(In reply to comment #33)
> Is there a reason why you need to fix the regression in a separate patch?

I think that if I separate to the bugs, we can get clearer comments and logs.
If I fix all regressions in this bug, if someone find similar regressions, it
may not be easy to find his/her needed comments in this many comments.

And simply, regressions are not exists in current patch. These will be fixed in
other components that are toolkit/components and xpfe(not core. it is fixed in
FAYT components). I think that reviews for these components need many days(these
reviewers current main works are for 1.8 branch, not trunk). I hope that other
regressions should be found by many nightly testers in this term.
> And simply, regressions are not exists in current patch.

I'm not sure what you mean, can you rephrase that?
(In reply to comment #35)
> > And simply, regressions are not exists in current patch.
> 
> I'm not sure what you mean, can you rephrase that?

I need change XPFE's FAYT code and Toolkit's widget code for fixing regressions.
Those files is not included in current patch.

This patch isn't really so big. I want to fix any known regressions in this
patch before landing. It's ultimately simpler and less work this way.
O.K. I will be adding new patches.
Comment on attachment 194192 [details] [diff] [review]
Patch rv1.4 widget/

clearing based on comment.

Just put it all in one big patch. I'll review the whole thing.
Attachment #194192 - Flags: review?(roc)
Attached patch Patch rv1.9(work in progress) (obsolete) — Splinter Review
Firstshot of new patch.
This can be built on Windows only. And this patch's IME controling for FAYT of
seamonkey does not work fine.
Attachment #194191 - Attachment is obsolete: true
Attachment #194192 - Attachment is obsolete: true
Attachment #194193 - Attachment is obsolete: true
*** Bug 310760 has been marked as a duplicate of this bug. ***
Depends on: 310626
Attached patch Patch rv1.9.1(work in progress) (obsolete) — Splinter Review
Attachment #197371 - Attachment is obsolete: true
Attached patch Patch rv2.0 (obsolete) — Splinter Review
This fix this bug. And I cannot find regression in my test.

Spec of this patch:
1. If focus is set to OBJECT or Editor, it enables IME.
   Otherwise, it disables IME.
2. If FAYT is stated on Seamonkey, it enables IME.
3. If FAYT is finished on Seamonkey, it disables IME.
Attachment #193819 - Attachment is obsolete: true
Attachment #194195 - Attachment is obsolete: true
Attachment #198826 - Attachment is obsolete: true
Attachment #199445 - Flags: superreview?(roc)
Attachment #199445 - Flags: review?(roc)
This is a lot of code and the FAYT special-case doesn't make me happy.

Maybe a better approach is to implement some sort of general feature to control whether IME should be actived when an element is focused ... e.g., a CSS property like IE's ime-mode:
http://www.blooberry.com/indexdot/css/properties/intl/imemode.htm
or maybe related to XForms' inputmode attribute, which has additional features:
http://www.w3.org/TR/2005/PER-xforms-20051006/sliceE.html
(I thought we might already have a bug on this, but I can't find it)

Then we could just check the content property on a focus change to determine whether to enable or disable IME. The default would be to disable IME; we'd set the property for editors and FAYT, but users/web authors would be able to control it in their own content too. (I heard requests for this while I was at IBM.) What do you think?
I have a plan that is to implement '-moz-ime-mode' CSS property in bug 279246.
I think we can implement this property by current patch's mechanism.
So, we will change the result of |nsEditor::GetDesirableIMEStatus|.

But non-Editor widgets should not enable IME.
Because there is no merit for usability.
(In reply to comment #45)
> I have a plan that is to implement '-moz-ime-mode' CSS property in bug 279246.
> I think we can implement this property by current patch's mechanism.
> So, we will change the result of |nsEditor::GetDesirableIMEStatus|.
> 
> But non-Editor widgets should not enable IME.
> Because there is no merit for usability.
 
It's possible to create your own DHTML widget where you trap key events and you want to receive IME input. That's what FAYT is, but web authors could build their own as well. For example, a list that you can search by typing characters that's implemented using DHTML rather than using a standard <select>. (BTW does this patch disable IME in select drop-downs?)

Also, the current patch makes a number of changes to FAYT. We really need to avoid that by at least making it easy to turn IME on/off for content in our own code. FAYT code shouldn't be aware of IME except for one method or property that says "I need IME when focused".
(In reply to comment #46)
> It's possible to create your own DHTML widget where you trap key events and you
> want to receive IME input. That's what FAYT is, but web authors could build
> their own as well. For example, a list that you can search by typing characters
> that's implemented using DHTML rather than using a standard <select>. (BTW does
> this patch disable IME in select drop-downs?)

I don't think so, IME composition string rendering and IME candidate list controling is very difficult. Maybe, it cannot implement by DHTML.
# especially, we cannot control IME candidate list from JS code.
If it is possible, the DHTML content cannot use on WinIE.
(WinIE disable IME on its non-Editor widgets having focus.)

> Also, the current patch makes a number of changes to FAYT. We really need to
> avoid that by at least making it easy to turn IME on/off for content in our own
> code. FAYT code shouldn't be aware of IME except for one method or property
> that says "I need IME when focused".
This is difficult issue. I don't have more simple idea for this...
Currently patch enable IME at starting FAYT input, and disable IME at finished FAYT input. Is this complicated? 
http://lxr.mozilla.org/mozilla/source/editor/libeditor/text/nsEditorEventListeners.cpp#438
438 nsTextEditorTextListener::HandleText(nsIDOMEvent* aTextEvent)
439 {

443    nsCOMPtr<nsIPrivateTextEvent> textEvent = do_QueryInterface(aTextEvent);
449    nsAutoString            composedText;
450    nsresult                result;
451    nsIPrivateTextRangeList *textRangeList;
452    nsTextEventReply        *textEventReply;

454    textEvent->GetText(composedText);
455    textEvent->GetInputRange(&textRangeList);
456    textEvent->GetEventReply(&textEventReply);

Roc:
See this code. We cannot process IME composition string in JS code.
We should use native code for IME processing.
So, we *cannot* create IME usable editor by DHTML.

For IME users, composition string and candidate list is most important UI. Without these, we cannot use IME.

FYI:
Current SeaMonkey's IME implementation is not good. Because the composition string doesn't render correctly, and candidate list is not positioned correct position. I think that SeaMonkey should port Firefox's Find Toolbar. But this is not scope of this bug. It is bug 97023.
> We really need to
> avoid that by at least making it easy to turn IME on/off for content in our own
> code. FAYT code shouldn't be aware of IME except for one method or property
> that says "I need IME when focused".

I have one more comment.

The SeaMonkey's FAYT codes *know* the IME.
Because it is handling the IME event for the Brwoser Window and it treats the Composition String itself.
So, FAYT code is controling the IME transactions itself already. 
I think that:

Currently, any scripts cannot process and control the IME on Gecko.
Your IME usable editor by DHTML is very interesting idea for me. However, we cannot implement it on current Gecko. Because we need to expand the API for IME transaction. But it's not scope of this bug. We don't have any reason for enabling IME on the non-Editor widgets(and non-Object element).
So these FAYT changes are actually just porting code over from Firefox to Seamonkey?

Should FAYT really be using an editor?

This patch is still a lot of code and new interfaces for something that doesn't seem on the surface to be very complicated. For example, can you explain why we need FocusContent events and the reply?
(In reply to comment #51)
> Should FAYT really be using an editor?

Right. We cannot process and control all IME features on current Gecko.

> For example, can you explain why we
> need FocusContent events and the reply?

If we don't use the reply of FocusContent events, we need to write IME enabling/disabling code in the FocusContent event on the *all* elements.
This is not useful for developers. Maybe, all developers will not be disabling IME on FocusContent events on new elements that is created by them. If so, I will need to fix to IME bugs on all new elements :-(

The reply system of thie patch, other developers don't need to care IME status. Because they doesn't change the reply value, the default value is disabling IME.
> (In reply to comment #51)
>> Should FAYT really be using an editor?
> 
> Right. We cannot process and control all IME features on current Gecko.

Right. We cannot process and control all IME features *on non-Editor widgets* on current Gecko.
After each focus change, can't we just check where the focus is and determine whether the IME should be disabled or not, rather than mess around with replies to the event?
I have no idea. Because HTML/PlainText editor is handling the focus event itself.
By HTML editor, almost elements can become Editor widgets also. Of course, they can become non-Editor widget too, in many cases.
And I think that if we can check the current focus is in Editor or not, we need many processes.

for Editor:
1. need to check that the current focus is set to Editor or not.
2. need to get the editor interface.
3. need to check that the Editor is password field or not.
4. (after bug 279246) need to get the -moz-ime-mode style.

for Object:
1. need to check that the current focus is set to Object or not.
2. need to check that the Object is using plug-in or not.

We need to check these on every focus changing times.
This is not reasonable. My approch is only changing the reply of FocusContent events if it should enable IME. This is very reasonable. There are no futility costs. And I think that my approach is simple for IME status controling.
Attachment #199445 - Flags: superreview?(roc)
Attachment #199445 - Flags: review?(roc)
Attached patch Patch rv2.1 (obsolete) — Splinter Review
Updating to current trunk.
Attachment #199445 - Attachment is obsolete: true
Attachment #201522 - Flags: superreview?(roc)
Attachment #201522 - Flags: review?(roc)
Why can't we add a method to nsIContent alongside IsFocusable(), something like GetDesiredIMEState(), returning "disabled" as the default implementation in nsGenericElement? nsHTMLObjectElement, nsHTMLInputElement could override it and choose the result the same way they choose their focusability. We could call it and update the IME state from nsEventManager::ShiftFocusInternal. ShiftFocusInternal would also need to check if we're in an editable document and take that into account. Is this approach really hard? It doesn't seem so to me.
Fmm... I'll try to create new test patch.
Attached patch Patch rv3.0 (obsolete) — Splinter Review
This patch is more simple than previous one.
But this patch also has many minnor issues on FAYT of SeaMonkey(on Firefox, all green!).

I think that for FAYT module of SeaMonkey, we need to refact it. Because it has many bugs of the transaction managiment.(inputting transaction, finding transaction). If I fix all minor issues without refactoring, the code will be spaghetti.

Roc, I hope that we should fix this bug by this patch. And I should file a new bug for FAYT refactoring with Neil and Aaron.
Attachment #201522 - Attachment is obsolete: true
Attachment #202955 - Flags: superreview?(roc)
Attachment #202955 - Flags: review?(roc)
Attachment #201522 - Flags: superreview?(roc)
Attachment #201522 - Flags: review?(roc)
Attached patch Patch rv3.1 (obsolete) — Splinter Review
Sorry, I fogot to change IID of nsIEventStateManager.
Attachment #202955 - Attachment is obsolete: true
Attachment #202957 - Flags: superreview?(roc)
Attachment #202957 - Flags: review?(roc)
Attachment #202955 - Flags: superreview?(roc)
Attachment #202955 - Flags: review?(roc)
This looks really good!

+  // Suppress/Unsuppress IME state controlling for FAYT of SeaMonkey.
+  NS_IMETHOD NotifyStartFAYTInput(PRUint32* aIMEState) = 0;
+  NS_IMETHOD NotifyFinishFAYTInput() = 0;

except this looks really bad, putting in special FAYT code in the eventmanager. Will the refactoring of Seamonkey FAYT make this go away?

If so, I wonder if it would be better to just disable IME in Seamonkey FAYT until it can be fixed, instead of checking in special hack code. Without the Seamonkey FAYT support this patch would be even simpler. If you don't agree,  you should at least mark with comments the code that you expect to be removed by the Seamonkey FAYT work.

I'll have more comments tomorrow so you needn't revise the patch just yet.
> Will the refactoring of Seamonkey FAYT make this go away?

No, we need these methods after refactoring.
# If we will port the find toolbar to SeaMonkey, we can remove these methods.
# But I think that it is not time for porting. Because we have many bugs in
# find toolbar. I'm working on these bugs in Trunk. It is no good existing same
# modules in different modules.

In this patch, the IME controller is ESM. On FAYT of SeaMonkey, we must override the controlloring(always enable IME) by ESM. Don't you like the names of the methods? If so, we use following names instead:
nsIEventStateManager::FixIMEState(PRUint32 aUseState, PRUint32 *aCurrentState);
nsIEventStateManager::EnableIMEStateControl();

If not so, do you have any idea?

I have an idea but I don't like this. In the ESM, we make a pref for disable/enable IME controlling. And the default value is enable in all product.
But if the users likes FAYT more than this bug, they can disable IME controlling by pref themselves.
> I have an idea but I don't like this. In the ESM, we make a pref for
> disable/enable IME controlling. And the default value is enable in all product.
> But if the users likes FAYT more than this bug, they can disable IME
> controlling by pref themselves.

This means we can remove these methods and all changes in nsTypeAheadFind.*
I don't really like the idea of a pref either. And I don't want to check in some nasty code just to keep Seamonkey FAYT working if we have no clear plan to remove it.

I'm leaning towards disabling IME for Seamonkey FAYT for now. It is then Seamonkey people's problem to fix it to work like the Firefox find toolbar, or port the Firefox find toolbar. Hopefully they can do that within the 1.9 timeframe (the next year).
More comments:

+  /*
+   * Get desired IME state for the content.
+   *
+   * @return The desired IME status for the content.
+   *         That is combination of IME_STATUS_*.
+   */

You need to say what combinations of flags are allowed. E.g., I assume IME_STATUS_ENABLE | IME_STATUS_DISABLE is not allowed. You also need to say it means when a flag is not set (e.g., IME_STATUS_NONE) ... I assume it means "leave IME in its current state". And does DISABLE also force IME to close or should people set both?

"ControlIMEState" is probably better named "UpdateIMEState".

+  // On Print Preview, we don't need IME.
+  if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {

Printing too. We really need a boolean method to check whether the context is interactive, but you don't need to do that here.

+  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
+                               nsIKBStateControl** aResult);

I think we don't need this if we disable IME in Seamonkey FAYT, but if we do keep it, it doesn't need to be virtual, right?

+  [noscript] void getDesirableIMEStatus(out unsigned long aState);

Better name "getPreferredIMEState"?

I like how this simplifies editor code!
(In reply to comment #66)
> More comments:
> 
> +  /*
> +   * Get desired IME state for the content.
> +   *
> +   * @return The desired IME status for the content.
> +   *         That is combination of IME_STATUS_*.
> +   */
> 
> You need to say what combinations of flags are allowed. E.g., I assume
> IME_STATUS_ENABLE | IME_STATUS_DISABLE is not allowed.
Yes.

> You also need to say it
> means when a flag is not set (e.g., IME_STATUS_NONE) ... I assume it means
> "leave IME in its current state".
right.

> And does DISABLE also force IME to close or
> should people set both?
Sorry, I cannot understand this. What you mean?

> +  // On Print Preview, we don't need IME.
> +  if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {
> 
> Printing too. We really need a boolean method to check whether the context is
> interactive, but you don't need to do that here.
Did you mean that we need a new function, but I don't need create it in this time?

> +  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
> +                               nsIKBStateControl** aResult);
> 
> I think we don't need this if we disable IME in Seamonkey FAYT, but if we do
> keep it, it doesn't need to be virtual, right?
Right. Should I remove this too? we don't need this if we disable IME in FAYT.

thanks,
>> +  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
>> +                               nsIKBStateControl** aResult);
>> 
>> I think we don't need this if we disable IME in Seamonkey FAYT, but if we do
>> keep it, it doesn't need to be virtual, right?
> Right. Should I remove this too? we don't need this if we disable IME in FAYT.

Oops. We need this. for SetIMEState/GetIMEState. not related FAYT.
> > And does DISABLE also force IME to close or
> > should people set both?
> Sorry, I cannot understand this. What you mean?

If I set DISABLE, will that close an open IME, or should I set DISABLE | CLOSE?

> > Printing too. We really need a boolean method to check whether the context is
> > interactive, but you don't need to do that here.
> Did you mean that we need a new function, but I don't need create it in this
> time?

Yes

(In reply to comment #68)
> >> +  NS_IMETHOD GetKBStateControl(nsPresContext* aPresContext,
> >> +                               nsIKBStateControl** aResult);
> >> 
> >> I think we don't need this if we disable IME in Seamonkey FAYT, but if we do
> >> keep it, it doesn't need to be virtual, right?
> > Right. Should I remove this too? we don't need this if we disable IME in FAYT.
> 
> Oops. We need this. for SetIMEState/GetIMEState. not related FAYT.

Ah yes, you need GetKBStateControl(nsPresContext* aPresContext, nsIKBStateControl** aResult) for SetIMEState (and it should be nonvirtual). However, you don't need GetIMEState.
(In reply to comment #69)
> > > And does DISABLE also force IME to close or
> > > should people set both?
> > Sorry, I cannot understand this. What you mean?
> 
> If I set DISABLE, will that close an open IME, or should I set DISABLE | CLOSE?
We don't need CLOSE for disable. Because when enabled, the open state is recovered. If CLOSE is set too, the open state is CLOSED when enabled.

> > Oops. We need this. for SetIMEState/GetIMEState. not related FAYT.
> 
> However, you don't need GetIMEState.
Oh, you are right. 

Attached patch Patch rv4.0 (obsolete) — Splinter Review
O.K. Let's go.
Attachment #202957 - Attachment is obsolete: true
Attachment #203028 - Flags: superreview?(roc)
Attachment #203028 - Flags: review?(roc)
Attachment #202957 - Flags: superreview?(roc)
Attachment #202957 - Flags: review?(roc)
Comment on attachment 203028 [details] [diff] [review]
Patch rv4.0

It's good.

+  if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {
+    SetIMEState(aPresContext, nsIContent::IME_STATUS_DISABLE);
+    return;
+  }

Please also check for printing here.

I'll give you some better text for the comment.
Attachment #203028 - Flags: superreview?(roc)
Attachment #203028 - Flags: superreview+
Attachment #203028 - Flags: review?(roc)
Attachment #203028 - Flags: review+
  * @return The desired IME status for the content.
  *         This is a combination of IME_STATUS_* flags,
  *         controlling what happens to IME when the content takes focus.
  *         If this is IME_STATUS_NONE, IME remains in its current state.
  *         IME_STATUS_ENABLE and IME_STATUS_DISABLE must not be set
  *         together; likewise IME_STATUS_OPEN and IME_STATUS_CLOSE must
  *         not be set together.
  *         If you return IME_STATUS_DISABLE, you should not set the
  *         OPEN or CLOSE flag; that way, when IME is next enabled,
  *         the previous OPEN/CLOSE state will be restored (unless the newly
  *         focused content specifies the OPEN/CLOSE state by setting the OPEN
  *         or CLOSE flag with the ENABLE flag).

Thanks for being patient. This is really good work.
Attached patch Patch rv4.1(for check-in) (obsolete) — Splinter Review
Attachment #203028 - Attachment is obsolete: true
Attachment #203032 - Flags: superreview+
Attachment #203032 - Flags: review+
Checked-in!

(In reply to comment #73)
> Thanks for being patient. This is really good work.

You too! Thank you very much!
# This and bug 113187 was disliked by IME users.
# But this fixed on XP code and Win code only. We need to work for Mac and Linux.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
backed out the patch. Because with VC6, failed to build.

> ../../dist/include/content\nsIContent.h(582) : error C2252: 'IME_STATUS_NONE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(583) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(583) : error C2252: 'IME_STATUS_ENABLE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(584) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(584) : error C2252: 'IME_STATUS_DISABLE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(585) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(585) : error C2252: 'IME_STATUS_OPEN' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(586) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(586) : error C2252: 'IME_STATUS_CLOSE' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(588) : error C2065: 'IME_STATUS_ENABLE' : undeclared identifier
> ../../dist/include/content\nsIContent.h(588) : error C2065: 'IME_STATUS_DISABLE' : undeclared identifier
> ../../dist/include/content\nsIContent.h(588) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(588) : error C2252: 'IME_STATUS_MASK_ENABLED' : pure specifier can only be specified for functions
> ../../dist/include/content\nsIContent.h(590) : error C2065: 'IME_STATUS_OPEN' : undeclared identifier
> ../../dist/include/content\nsIContent.h(590) : error C2065: 'IME_STATUS_CLOSE' : undeclared identifier
> ../../dist/include/content\nsIContent.h(590) : error C2258: illegal pure syntax, must be '= 0'
> ../../dist/include/content\nsIContent.h(590) : error C2252: 'IME_STATUS_MASK_OPENED' : pure specifier can only be specified for functions

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
To make VC6 happy you'll have to initialize those PRUint32s outside the class.

Try using an enum instead:

  static enum IME_Status
  {
    IME_STATUS_NONE    = 0,
    IME_STATUS_ENABLE  = 1 << 0,
    IME_STATUS_DISABLE = 1 << 1,
    IME_STATUS_OPEN    = 1 << 2,
    IME_STATUS_CLOSE   = 1 << 3,
    IME_STATUS_MASK_ENABLED = IME_STATUS_ENABLE | IME_STATUS_DISABLE,
    IME_STATUS_MASK_OPENED  = IME_STATUS_OPEN | IME_STATUS_CLOSE
  }

and adjust your methods to take and return the enum instead of a bare PRUint32.
Thank you, I'm testing with following code:

  enum {
    IME_STATUS_NONE    = 0x0000,
    IME_STATUS_ENABLE  = 0x0001,
    IME_STATUS_DISABLE = 0x0002,
    IME_STATUS_OPEN    = 0x0004,
    IME_STATUS_CLOSE   = 0x0008
  };
  enum {
    IME_STATUS_MASK_ENABLED = IME_STATUS_ENABLE | IME_STATUS_DISABLE,
    IME_STATUS_MASK_OPENED  = IME_STATUS_OPEN | IME_STATUS_CLOSE
  };

If I will confirm this is O.K., I'll check-in without review.
Thanks.(Now, testing with VC6 on VMWare, So, I need an hour.)
Attachment #203032 - Attachment is obsolete: true
Attachment #203071 - Flags: superreview+
Attachment #203071 - Flags: review+
checked-in.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
I'm now seeing this all the time:
WARNING: NS_ENSURE_TRUE(kb) failed, file nsEventStateManager.cpp, line 5557
Ah, do you using GTK2 or OS/2 or BeOS?
I filed a new bug for the comment 81. See bug 316805.
Depends on: 323214
Mnemonic still does not work even in the trunk. With the IME on and the focus in text area, I can invoke File menu by pressing Alt-F, but pressing 'o' with the file menu  does not open up a file picker. Instead, 'o' is processed by the IME in text area.  The very same symptom described in comment #0. 

right. but it should be separated to new bug. because we need to implement another mechanism for the editors. I have tried to it on Windows, but I failed it and I cannot find what way can fix this bug...
Hmm... Why was this bug resolved as fixed when the symptom reported in comment #0 hadn't been fixed? Either this bug should be reopened or the bug summary should be changed to match what WAS fixed here?  If we do the latter, we should file a new bug about the original problem (have you filed one already?). 

When the editors doesn't have a focus, this bug was fixed. But there are still some issues, it is also right. Therefore, I marked to fixed.

I think that the bug on editor is a *new* bug of my approach in this bug. The xul menu windows cannot have focus, but they have the actual keyboard focus. Therefore, the patch doesn't work fine. I think that we should _suspend_ the composing temporarily at popping up the XUL menu and resume the composing at closing the *all* XUL menus.

# I didn't file a new bug.
You need to log in before you can comment on or make changes to this bug.