Closed Bug 417315 Opened 12 years ago Closed 12 years ago

Cannot use IME menus during IME transaction

Categories

(Core :: Internationalization, defect, major)

x86
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, jp-critical, regression)

Attachments

(2 files, 7 obsolete files)

3.64 KB, patch
masayuki
: review+
masayuki
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
8.12 KB, patch
masayuki
: review+
masayuki
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
Attached patch Patch v0.1 (obsolete) — Splinter Review
We cannot use IME menus (keyboard layout switching menu, I don't know the official name in English) during IME transaction.

Because current trunk build commit the IME transaction at losing focus. This is safe and simple fix for some bugs. But the menu gets the focus at opened. Therefore, we cannot use the menu.

The menu has some features of IMEs and some users might use them. However, I'm not sure that I can fix Gecko1.9 final.

I attach a simple patch. That removes the force commit code at loosing the focus. But when switch between 2 Gecko windows, there is a serious bug (in system (in other words, in IME), the transaction was committed, but Gecko still continues the transaction). We cannot fix this bug until all regression bugs being gone.
Attached patch Patch v0.2 (obsolete) — Splinter Review
This fixes the found issues. I need more testing for this.
Attachment #303082 - Attachment is obsolete: true
Attached patch Patch v1.0 (obsolete) — Splinter Review
o.k. I cannot found any regression on Mac.

However, this patch cannot fix  this bug on search bar of browser window. We need to change the toolkit code and editor code for it. I'll create the patch, but it should be in other review process, because they are not related.
Attachment #303135 - Attachment is obsolete: true
Comment on attachment 303270 [details] [diff] [review]
Patch v1.0

We should not define NS_KBSC_USE_SHARED_CONTEXT on Mac. And we can remove nsTSMManager::CommitIME() at childview gets/loses the focus.

When window is changed between a Gecko application, nsIKBStateControl::ResetInputState is called, and it calls nsTSMManaager::CommitIME, finally, it fires the IME composition end event. However, it is sent to rood element, not last focused content, therefore, we need to change nsPresShell.
Attachment #303270 - Flags: superreview?(roc)
Attachment #303270 - Flags: review?(roc)
Attachment #303270 - Flags: review?(joshmoz)
Maybe, I can fix this bug soon. This should block the release.
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9beta4
The textbox of toolkit with clickSelectsAll always calls selectAll when that gets focus. However, if the editor has IME transaction, it should not call selectAll. Because it commits the IME transaction. Therefore, we need to get whether the editor has IME transaction, and then, textbox should not call selectAll.

First, I need editor part's review.
Attachment #303310 - Flags: superreview?(peterv)
Attachment #303310 - Flags: review?(peterv)
NS_SHOULD_EVENT_TARGET_IS_LAST_FOCUSED_CONTENT isn't a very good name. Also, it should be a static function with #ifdefs in its body, and it should be always defined. How about calling it TargetUnfocusedEventToLastFocusedContent?

Instead of #if0'ing  NS_KBSC_USE_SHARED_CONTEXT, just remove it completely.
Comment on attachment 303310 [details] [diff] [review]
Patch for toolkit and editor v1.0

Please get an additional review from a toolkit peer on the toolkit part.
Attachment #303310 - Flags: superreview?(peterv)
Attachment #303310 - Flags: superreview+
Attachment #303310 - Flags: review?(peterv)
Attachment #303310 - Flags: review+
Comment on attachment 303310 [details] [diff] [review]
Patch for toolkit and editor v1.0

Neil:

See comment 5 for this patch.
Attachment #303310 - Flags: review?(enndeakin)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #303270 - Attachment is obsolete: true
Attachment #303479 - Flags: superreview?(roc)
Attachment #303479 - Flags: review?(roc)
Attachment #303479 - Flags: review?(joshmoz)
Attachment #303270 - Flags: superreview?(roc)
Attachment #303270 - Flags: review?(roc)
Attachment #303270 - Flags: review?(joshmoz)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Oops, sorry for the spam.
Attachment #303479 - Attachment is obsolete: true
Attachment #303480 - Flags: superreview?(roc)
Attachment #303480 - Flags: review?(roc)
Attachment #303480 - Flags: review?(joshmoz)
Attachment #303479 - Flags: superreview?(roc)
Attachment #303479 - Flags: review?(roc)
Attachment #303479 - Flags: review?(joshmoz)
Comment on attachment 303310 [details] [diff] [review]
Patch for toolkit and editor v1.0

>+            else if (this.clickSelectsAll) {
>+              const nsIEditorIMESupport =
>+                      Components.interfaces.nsIEditorIMESupport;
>+              imeEditor = this.editor.QueryInterface(nsIEditorIMESupport);

You need to declare imeEditor. (var or let)

>+              if (!imeEditor || !imeEditor.composing)
>+                this.editor.selectAll();
>+            }

Also, even though this was a problem before, the code block for if (this.clickSelectsAll) should really be in try/catch
Attachment #303310 - Attachment is obsolete: true
Attachment #303552 - Flags: review?(enndeakin)
Attachment #303310 - Flags: review?(enndeakin)
Comment on attachment 303552 [details] [diff] [review]
Patch for toolkit and editor v1.1

Actually, it's the 'this.editor' and 'selectAll' calls that will cause an exception (a textbox in a non-chrome document for instance), so I don't think the additional selectAll call within the catch block is necessary.
Attachment #303552 - Flags: review?(enndeakin) → review+
Neil:

I misunderstand why you need the try/catch. It seems that this is your wanted patch. If you don't like this, please deny this. (I don't set the review flag, because the previous is granted.)
Attachment #303552 - Attachment is obsolete: true
Whe  you(In reply to comment #14)
> Created an attachment (id=303568) [details]
> Patch for toolkit and editor v1.2
> 
> Neil:
> 
> I misunderstand why you need the try/catch. 

Mainly because it causes an exception when used from non-chrome. But the code had that problem already so it isn't a big deal if you checked in either the original patch (with the var change) or the latest one.
Comment on attachment 303480 [details] [diff] [review]
Patch v1.1

+        if (NS_TargetUnfocusedEvenToLastFocusedContent(aEvent)) {

Missing a "t" in "Event"

+#endif // defined(MOZ_X11) || defined(XP_MACOSX)

The // comment is incorrect. Just remove it.
Attachment #303480 - Flags: superreview?(roc)
Attachment #303480 - Flags: superreview-
Attachment #303480 - Flags: review?(roc)
Attachment #303480 - Flags: review+
Comment on attachment 303480 [details] [diff] [review]
Patch v1.1

oops
Attachment #303480 - Flags: superreview- → superreview+
Attachment #303480 - Flags: review?(joshmoz) → review+
Attached patch Patch v1.2Splinter Review
This should be landed now. This bug is major feature non-working bug.
The risk is middle. Because some behavior is depended on IMEs. However, it's same on other applications. So, if there is some problem, it might be IME bugs.
Attachment #303480 - Attachment is obsolete: true
Attachment #304268 - Flags: superreview+
Attachment #304268 - Flags: review+
Attachment #304268 - Flags: approval1.9?
Comment on attachment 303568 [details] [diff] [review]
Patch for toolkit and editor v1.2

This is major feature bug. And this patch's risk is low.
Attachment #303568 - Flags: superreview+
Attachment #303568 - Flags: review+
Attachment #303568 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Comment on attachment 303568 [details] [diff] [review]
Patch for toolkit and editor v1.2

a=beltzner for 1.9
Attachment #303568 - Flags: approval1.9? → approval1.9+
Comment on attachment 304268 [details] [diff] [review]
Patch v1.2

a=beltzner for 1.9
Attachment #304268 - Flags: approval1.9? → approval1.9+
checked-in, thanks!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This patch added a whole slew of warnings to my build: ../../../../dist/include/widget/nsGUIEvent.h:1250: warning: ‘PRBool NS_TargetUnfocusedEventToLastFocusedContent(nsEvent*)’ defined but not used

because NS_TargetUnfocusedEventToLastFocusedContent is declared static in a wildly-included header file. Perhaps you meant it to be 'inline' instead of 'static'?
You need to log in before you can comment on or make changes to this bug.