Add a notifier observer about the IME State

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 482749 [details] [diff] [review]
Patch - Add a NotifyObservers to nsIMEStateManager.cpp

Most of the development of the front-end side of Fennec happens on the desktop, it would _really_ help if we could emulate the virtual keyboard behavior by sending the current IME state thought an observer.

I have already an addon based on this code to emulate a Virtual Keyboard and it seems to perform very well, but maybe there is some better place to put this notifier?
Attachment #482749 - Flags: review?(masayuki)
Just echoing what Vivien said. This notification would make it easier for us to test the IME state without needing to run on a device with a virtual keyboard.
Comment on attachment 482749 [details] [diff] [review]
Patch - Add a NotifyObservers to nsIMEStateManager.cpp

I don't understand this bug well.

What means the IME state? There are some states about IME:

1. enabled vs. disabled
2. turned on (opened) vs. turned off (closed)
3. just inputting mode change, like Hiragana mode, Katakana mode, Hiragana-kotei mode, etc...

The patch always notifies at #1. But it only sometimes notifies #2 and #3, not always (e.g., when the state was changed by users, this patch doesn't notifies it). This inconsistency can make some CJK addon developers be confused.
Attachment #482749 - Flags: review?(masayuki) → review-
For mobile developers, IME is closely tied to virtual keyboard. There are at least two reasons we want a patch like this:

1. We want to be able to catch when a virtual keyboard becomes visible and is hidden. We want this to make sure we are updating the UI correctly, moving focused textboxes into view for example.
2. We want to be able to test and simulate the aspects of a virtual keyboard on desktop OSes (like Linux) where a virtual keyboard is not present. The notifications allow us to add browser-chrome tests to make sure the UI is working correctly. One way we can do this is by making our own simulated virtual keyboard on desktop Linux.

We would like to have as complete coverage as possible. I'm not sure how we can catch state changes made by the user. Do you have any ideas?

Vivien can add details or correct any mistakes I made.
(In reply to comment #2)
> Comment on attachment 482749 [details] [diff] [review]
> Patch - Add a NotifyObservers to nsIMEStateManager.cpp
> 
> I don't understand this bug well.
> 
> What means the IME state? There are some states about IME:
> 
> 1. enabled vs. disabled
> 2. turned on (opened) vs. turned off (closed)
> 3. just inputting mode change, like Hiragana mode, Katakana mode,
> Hiragana-kotei mode, etc...
> 

As far as I know we are interested only in #2 so I could move the code into the "if (aState & nsIContent::IME_STATUS_MASK_OPENED) " check if needed

When you said that there is some paths the patch is not covering do you mean:
 * http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#1943
 * http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#7473
 * http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.cpp#361

If this is not what you mean, could you give me a hint?
(I have looked at http://mxr.mozilla.org/mozilla-central/search?string=IMEIsOpen&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central but this is disapointing since it looks like test_imestate.html tries to define the nsIDOMWindowUtils.IMEIsOpen which is readOnly)
Created attachment 482987 [details] [diff] [review]
Patch v0.2

I think we finally just want to know when the IME is enabled/disabled
Assignee: nobody → 21
Attachment #482749 - Attachment is obsolete: true
Attachment #482987 - Flags: review?(masayuki)
Comment on attachment 482987 [details] [diff] [review]
Patch v0.2

okay, then,

> +      observerService->NotifyObservers(nsnull, "imestate-change", imeState.get());

Isn't "ime-enabled-state-changed" or something better? The important points are "enabled" and change_d_. I hope you look for a better name.

> +      imeState.AppendInt(state);

The state values are defined in nsIDOMWindowUtils. Please use these consts when you use this notification on our product.

Finally, you must notify it after the focus moving is finished. I.e., you should create a runnable event and you should notify it from the event. The reason is that some native APIs' behavior isn't stable during focus moving especially on Mac.
(In reply to comment #4)
> If this is not what you mean, could you give me a hint?

Users can change the open state by their operation. E.g., clicks on IME's floating toolbar, presses some system's shortcut keys or uses IME menu. Then, we don't know the timing of the change. We can catch the change on Windows. I'm not sure on Mac. And we cannot know on GTK2. Fortunately, now, you needs only enable/disable state change here.
Created attachment 483623 [details] [diff] [review]
Patch v0.3

> > +      imeState.AppendInt(state);
> 
> The state values are defined in nsIDOMWindowUtils. Please use these consts when
> you use this notification on our product.

I'm a bit unsure of what you mean here? Do you mean I should pay attention to use the constants inside the listener of the notifications?

For all your others comments I've updated the patch, I don't think we need a nsRunnable event for the TabParent notifier.

In all cases thanks for all your directions.
Attachment #482987 - Attachment is obsolete: true
Attachment #483623 - Flags: review?(masayuki)
Attachment #482987 - Flags: review?(masayuki)
Comment on attachment 483623 [details] [diff] [review]
Patch v0.3

>diff -r eb674a183386 content/events/src/nsIMEStateManager.cpp
>+  if (widget) {
>     widget->SetIMEEnabled(aValue);
>+
>+   nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();

fix the strange indentation level.

>+    if (observerService) {
>+      nsAutoString state;
>+      state.AppendInt(aValue);
>+      observerService->NotifyObservers(nsnull, "ime-enabled-state-changed", state.get());
>+    }
>+  }

Otherwise, looks okay.
Attachment #483623 - Flags: review?(masayuki) → review+
(In reply to comment #10)
> Created attachment 483623 [details] [diff] [review]
> Patch v0.3
> 
> > > +      imeState.AppendInt(state);
> > 
> > The state values are defined in nsIDOMWindowUtils. Please use these consts when
> > you use this notification on our product.
> 
> I'm a bit unsure of what you mean here? Do you mean I should pay attention to
> use the constants inside the listener of the notifications?

I meant that when you write a handler of the notification on our product, you should use the defined values in nsIDOMWindowUtils at comparing the values rather than numeric.

> For all your others comments I've updated the patch, I don't think we need a
> nsRunnable event for the TabParent notifier.

I'm not sure on e10s, but the code also works on non-e10s, so, it's needed.

Thank you for your work.
tracking-fennec: --- → 2.0b2+
Thanks

http://hg.mozilla.org/mozilla-central/rev/e0631e860286
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.