Closed Bug 1112212 Opened 10 years ago Closed 9 years ago

Switch to key event mode when input has key listeners

Categories

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Product:
Firefox for Android Graveyard
Component:
Keyboards and IME
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox37 fixed, firefox38 affected, firefox41 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 37
Tracking Flags:
Tracking Status
firefox37 --- fixed
firefox38 --- affected
firefox41 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(4 files, 9 obsolete files)

Send key events in onKeyMultiple (v2)
8.13 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
Add may-have flags for key event and input event listeners
4.20 KB, patch
jchen
: review+
Details | Diff | Splinter Review
Set flag in InputContext for IME-unaware webapps (v6)
6.47 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Implement dummy-key-events mode for Android (v1)
4.49 KB, patch
esawin
: review+
Details | Diff | Splinter Review 
A lot of pages rely on key listeners for input fields. However, virtual keyboards on Android don't always generate key events during normal editing. As a result, a lot of sites with widgets like dropdown filters don't work quite right.

Fennec has an alternative input mode where we try to always generate key events. Currently it's only used for plugins, but we can switch to this mode if we see that an input field has key listeners.
Add may-have flags for key events and input events, so that we can check them when deciding which keyboard mode to use on Android.
Attachment #8538019 - Flags: review?(bugs)
In plugin mode, we only send key events instead of composition events. For input fields that have key event listeners but not input event listeners, we switch to plugin mode so that the page can receive all key events.
Attachment #8538025 - Flags: review?(masayuki)
Comment on attachment 8538019 [details] [diff] [review]
Add may-have flags for key event and input event listeners (v1)

mMayHaveUserInputEventListener is odd name, especially because there is also
event named 'input'.

So call it mMayHaveCompositionEventListener, and the method
MayHaveCompositionEventListener().

And for key event case, 
mMayHaveKeyEventListener/MayHaveKeyEventListener().
Attachment #8538019 - Flags: review?(bugs) → review+
Comment on attachment 8538025 [details] [diff] [review]
Use key event mode for inputs with key event listeners (v1)

This checks just one EvenListenerManager. What if some node in the ancestor chain has relevant listeners?

For MutationEvent listeners we do
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp?rev=296c052ea50b#3807
Attachment #8538025 - Flags: review?(masayuki) → review-
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8538025 [details] [diff] [review]
> Use key event mode for inputs with key event listeners (v1)
> 
> This checks just one EvenListenerManager. What if some node in the ancestor
> chain has relevant listeners?

Is it important to check the ancestor nodes? So far all the cases I've seen involve listeners on the element itself. Because this is only a workaround for web compatibility, I'm leaning towards limiting our scope to the elements themselves and not their ancestors.
It would be totally inconsistent behavior if ancestors aren't checked.
In DOM the listener really can be anywhere in the event target chain.
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8538025 [details] [diff] [review]
> Use key event mode for inputs with key event listeners (v1)
> 
> This checks just one EvenListenerManager. What if some node in the ancestor
> chain has relevant listeners?
> 
> For MutationEvent listeners we do
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.
> cpp?rev=296c052ea50b#3807

And also I don't like this approach. Why don't you add a new member into InputContext and nsWindow for Android checks it? Additionally, I guess you need this hack when the state is PASSWORD too. I don't like the name PASSWORD, though, it could be used for <input type="text"> too when its style has |ime-mode: disabled;|.

Anyway, this may not work as you expected if a focus event listener tries to add composition event listener, I guess such behavior must be minority, though.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1897
Changed the names.
Attachment #8538019 - Attachment is obsolete: true
Attachment #8539502 - Flags: review+
Changed to checking the ancestor chain similar to nsContentUtils::HasMutationListeners (we only care about web content listeners so I left out the XBL bit). Also added a separate field in InputContext.
Attachment #8538025 - Attachment is obsolete: true
Attachment #8539506 - Flags: review?(masayuki)
Attachment #8539506 - Flags: review?(bugs)
Because the plugin mode and key-events-only mode are the same on Android, this patch swiches us to plugin mode when we're in key-events-only mode.
Attachment #8539508 - Flags: review?(cpeterson)
The old onKeyMultiple implementation didn't really work. We should be sending real key events instead.
Attachment #8539509 - Flags: review?(cpeterson)
Comment on attachment 8539506 [details] [diff] [review]
Use key event mode for inputs with key event listeners (v2)

>+#ifdef MOZ_WIDGET_ANDROID

Hmm, I don't like to use #ifdef in IMEStateManager... How about to use pref? E.g., "intl.key_events.dispatch_during_composition.if_no_composition_event_handler"?

>+namespace {

Why do you need this? Do we have a good reason or new coding style rule?

>+
>+bool
>+UseKeyEventsMode(nsINode* aNode) {

nit: put |{| next line.

>+  bool haveKeyEvents = false;
>+  bool haveCompositionEvents = false;
>+
>+  while (aNode) {
>+    EventListenerManager* const mgr = aNode->GetExistingListenerManager();
>+    if (mgr) {
>+      haveKeyEvents |= mgr->MayHaveKeyEventListener();
>+      haveCompositionEvents |= mgr->MayHaveCompositionEventListener();
>+    }
>+    aNode = aNode->GetParentNode();
>+  }
>+  return haveKeyEvents && !haveCompositionEvents;

Looks like that if the loop finds composition event listeners, can quit from this with false.

> struct InputContext {
>-  InputContext() : mNativeIMEContext(nullptr) {}
>+  InputContext() : mNativeIMEContext(nullptr), mUseKeyEvents(false) {}

Please be careful, this will be conflict with bug 1051842.

>+  /* True if the IME should generate key events instead of composition events

"instead of"? I think that you need to dispatch composition events for nsEditor. So, probably, you will dispatch key events even during composition. Do I misunderstand?

>+     when editing, so that key event listeners are notified. */
>+  bool mUseKeyEvents;
Comment on attachment 8539506 [details] [diff] [review]
Use key event mode for inputs with key event listeners (v2)

(This doesn't deal with Shadow DOM, but that can be fixed in a follow, since Shadow DOM isn't enabled by default anyway. I could fix that.)


>+#ifdef MOZ_WIDGET_ANDROID
>+namespace {
Per coding style
"We prefer using 'static' instead of anonymous C++ namespaces."


>+
>+bool
>+UseKeyEventsMode(nsINode* aNode) {
static bool
UseKeyEventsMode(nsINode* aNode)
{



>+  bool haveKeyEvents = false;
>+  bool haveCompositionEvents = false;
>+
>+  while (aNode) {
>+    EventListenerManager* const mgr = aNode->GetExistingListenerManager();
>+    if (mgr) {
>+      haveKeyEvents |= mgr->MayHaveKeyEventListener();
>+      haveCompositionEvents |= mgr->MayHaveCompositionEventListener();
if (mgr->MayHaveCompositionEventListener()) {
  return false;
}

>+#ifdef MOZ_WIDGET_ANDROID
>+  // Normal text editing using Android virtual keyboards doesn't always
>+  // generate key events. However, when an input field has key event listeners
>+  // but not input event listeners,
You're not dealing with input events here, but composition events.

>+  if (context.mIMEState.IsEditable() && UseKeyEventsMode(aContent)) {
>+    // Use key events mode in order to always generate key events.
>+    context.mUseKeyEvents = true;
>+  }

Hmm, do we want to write this
context.mUseKeyEvents = context.mIMEState.IsEditable() && UseKeyEventsMode(aContent);
so that we value is updated always.
Attachment #8539506 - Flags: review?(bugs) → review-
Comment on attachment 8539508 [details] [diff] [review]
Implement use-key-events mode for Android (v1)

Review of attachment 8539508 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: widget/android/nsWindow.cpp
@@ +2161,5 @@
>          enabled = IMEState::DISABLED;
> +
> +    } else if (aContext.mUseKeyEvents) {
> +        // On Android, the "use-key-events" mode is the same as the "plugin"
> +        // mode, so swich to plugin mode if we are forcing key events.

s/swich/switch/
Attachment #8539508 - Flags: review?(cpeterson) → review+
Comment on attachment 8539509 [details] [diff] [review]
Send key events in onKeyMultiple (v1)

Review of attachment 8539509 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoEvent.java
@@ +265,5 @@
>      public static GeckoEvent createNoOpEvent() {
>          return GeckoEvent.get(NativeGeckoEvent.NOOP);
>      }
>  
> +    public static GeckoEvent createKeyEvent(KeyEvent k, int action, int metaState) {

I don't see any changes to createKeyEvent's callers in this bug's patches. Did you forget to hg qrefresh GeckoInputConnection.java? It's the only caller I see in DXR.

@@ +288,5 @@
>      }
>  
> +    private void initKeyEvent(KeyEvent k, int action, int metaState) {
> +        // Use a separate action argument so we can override the key's
> +        // original action, e.g. change ACTION_MULTIPLE to ACTION_DOWN.

Maybe expand this comment to explain why we would want to override the original action from ACTION_MULTIPLE to ACTION_DOWN.
Attachment #8539509 - Flags: review?(cpeterson) → feedback+
(In reply to Chris Peterson (:cpeterson) from comment #16)
> Comment on attachment 8539509 [details] [diff] [review]
> Send key events in onKeyMultiple (v1)
> 
> ::: mobile/android/base/GeckoEvent.java
> @@ +265,5 @@
> >      public static GeckoEvent createNoOpEvent() {
> >          return GeckoEvent.get(NativeGeckoEvent.NOOP);
> >      }
> >  
> > +    public static GeckoEvent createKeyEvent(KeyEvent k, int action, int metaState) {
> 
> I don't see any changes to createKeyEvent's callers in this bug's patches.
> Did you forget to hg qrefresh GeckoInputConnection.java? It's the only
> caller I see in DXR.

This patch contains changes to createKeyEvent's callers :)
Attachment #8539509 - Attachment is obsolete: true
Attachment #8540427 - Flags: review?(cpeterson)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> Comment on attachment 8539506 [details] [diff] [review]
> Use key event mode for inputs with key event listeners (v2)
> 
> >+#ifdef MOZ_WIDGET_ANDROID
> 
> Hmm, I don't like to use #ifdef in IMEStateManager... How about to use pref?
> E.g.,
> "intl.key_events.dispatch_during_composition.
> if_no_composition_event_handler"?

I added a "intl.ime.key_event_listener_compatibility" pref.

> >+  /* True if the IME should generate key events instead of composition events
> 
> "instead of"? I think that you need to dispatch composition events for
> nsEditor. So, probably, you will dispatch key events even during
> composition. Do I misunderstand?

On Android, when the user presses a key, normally the IME starts a composition and sets the composition to the entered character. In key events mode, the IME sends a key press event instead, so there is no composition event. Is this what you meant?


(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8539506 [details] [diff] [review]
> Use key event mode for inputs with key event listeners (v2)
> 
> (This doesn't deal with Shadow DOM, but that can be fixed in a follow, since
> Shadow DOM isn't enabled by default anyway. I could fix that.)

Okay.

> >+#ifdef MOZ_WIDGET_ANDROID
> >+  // Normal text editing using Android virtual keyboards doesn't always
> >+  // generate key events. However, when an input field has key event listeners
> >+  // but not input event listeners,
> You're not dealing with input events here, but composition events.

We actually need to detect both "input" and "composition*" events. Attachment 8539502 [details] [diff] does that. Maybe we should rename the flag to MayHaveInputOrCompositionEventListener or split it into two flags?

> >+  if (context.mIMEState.IsEditable() && UseKeyEventsMode(aContent)) {
> >+    // Use key events mode in order to always generate key events.
> >+    context.mUseKeyEvents = true;
> >+  }
> 
> Hmm, do we want to write this
> context.mUseKeyEvents = context.mIMEState.IsEditable() &&
> UseKeyEventsMode(aContent);
> so that we value is updated always.

InputContext constructor initializes mUseKeyEvents to false.
Attachment #8539506 - Attachment is obsolete: true
Attachment #8539506 - Flags: review?(masayuki)
Attachment #8540465 - Flags: review?(masayuki)
Attachment #8540465 - Flags: review?(bugs)
Comment on attachment 8540427 [details] [diff] [review]
Send key events in onKeyMultiple (v2)

Review of attachment 8540427 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM now. :)
Attachment #8540427 - Flags: review?(cpeterson) → review+
Comment on attachment 8540465 [details] [diff] [review]
Use key event mode for inputs with key event listeners (v3)

>+static bool
>+UseKeyEventsMode(nsINode* aNode)
>+{
>+  // Normal text editing using Android virtual keyboards doesn't always
>+  // generate key events. However, when an input field has key event listeners
>+  // but not input/composition event listeners, the page expects to receive
>+  // key events for every input. Therefore, for better web compatibility, we
>+  // use a special mode that always generates key events in this situation.
>+
>+  if (!Preferences::GetBool("intl.ime.key_event_listener_compatibility",
>+                            false)) {
>+    return false;
>+  }

I think that you should use Preferences::AddBoolVarChache() since it's in very hot path.

>+
>+  bool haveKeyEventsListener = false;
>+
>+  while (aNode) {
>+    EventListenerManager* const mgr = aNode->GetExistingListenerManager();
>+    if (mgr) {
>+      if (mgr->MayHaveCompositionEventListener()) {
>+        // Don't use key events mode if we have input/composition listeners.
>+        return false;
>+      }
>+      haveKeyEventsListener |= mgr->MayHaveKeyEventListener();
>+    }
>+    aNode = aNode->GetParentNode();
>+  }
>+
>+  // If we don't have input/composition listeners but have key listeners, we
>+  // should use key events mode if enabled.
>+  return haveKeyEventsListener;

Ah, does this mean if one of |mgr->MayHaveKeyEventListener()| returns true, then, always returns true? Then, in such case, you can quit the loop earlier.

>@@ -764,16 +797,21 @@ IMEStateManager::SetIMEState(const IMEState& aState,
> 
>   NS_ENSURE_TRUE_VOID(aWidget);
> 
>   InputContext oldContext = aWidget->GetInputContext();
> 
>   InputContext context;
>   context.mIMEState = aState;
> 
>+  if (context.mIMEState.IsEditable() && UseKeyEventsMode(aContent)) {
>+    // Use key events mode in order to always generate key events.
>+    context.mUseKeyEvents = true;

You can write this as:

context.mUseKeyEvents =
  context.mIMEState.IsEditable() && UseKeyEventsMode(aContext);

>+#ifdef MOZ_WIDGET_ANDROID
>+pref("intl.ime.key_event_listener_compatibility", true);
>+#else
>+pref("intl.ime.key_event_listener_compatibility", false);
>+#endif

I don't like this pref name because this is unclear what the "key_event_listener_compatibility" actually changes.

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>index 14e44ee..3bbb172 100644
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -401,17 +401,20 @@ struct IMEState {
>   // has focus.
>   bool MaybeEditable() const
>   {
>     return IsEditable() || mEnabled == PLUGIN;
>   }
> };
> 
> struct InputContext {
>-  InputContext() : mNativeIMEContext(nullptr) {}
>+  InputContext()
>+      : mNativeIMEContext(nullptr)
>+      , mUseKeyEvents(false)

Please reduce indent two spaces.

>@@ -423,16 +426,20 @@ struct InputContext {
> 
>   /* A hint for the action that is performed when the input is submitted */
>   nsString mActionHint;
> 
>   /* Native IME context for the widget.  This doesn't come from the argument of
>      SetInputContext().  If there is only one context in the process, this may
>      be nullptr. */
>   void* mNativeIMEContext;
>+
>+  /* True if the IME should generate key events instead of composition events
>+     when editing, so that key event listeners are notified. */
>+  bool mUseKeyEvents;

I realized that mUseKeyEvents isn't clear when other developers look this name. Perhaps, mNeedsKeyEventsDuringComposition?
Attachment #8540465 - Flags: review?(masayuki) → review-
Comment on attachment 8539508 [details] [diff] [review]
Implement use-key-events mode for Android (v1)

>+    } else if (aContext.mUseKeyEvents) {
>+        // On Android, the "use-key-events" mode is the same as the "plugin"
>+        // mode, so swich to plugin mode if we are forcing key events.
>+        enabled = IMEState::PLUGIN;
>     }

Hmm, in PLUGIN mode, does Android widget dispatch keypress events during composition? If so, you should change it for the new mode because D3E defines keydown and keyup events may be dispatched during composition but keypress events are not so.
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#events-composition-event-key-events
FYI: KeyboardEvent.isComposing is automatically set in EventStateManager ;-)
https://bugzilla.mozilla.org/attachment.cgi?id=8403772&action=diff
Changed to "MayHaveInputOrCompositionEventListener".
Attachment #8539502 - Attachment is obsolete: true
Attachment #8542725 - Flags: review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> Comment on attachment 8540465 [details] [diff] [review]
> Use key event mode for inputs with key event listeners (v3)
> 
> >@@ -423,16 +426,20 @@ struct InputContext {
> > 
> >   /* A hint for the action that is performed when the input is submitted */
> >   nsString mActionHint;
> > 
> >   /* Native IME context for the widget.  This doesn't come from the argument of
> >      SetInputContext().  If there is only one context in the process, this may
> >      be nullptr. */
> >   void* mNativeIMEContext;
> >+
> >+  /* True if the IME should generate key events instead of composition events
> >+     when editing, so that key event listeners are notified. */
> >+  bool mUseKeyEvents;
> 
> I realized that mUseKeyEvents isn't clear when other developers look this
> name. Perhaps, mNeedsKeyEventsDuringComposition?

What do you think of 'mEditorHasKeyButNotInputOrCompositionListener'? It will be true if the editor has an attached key listener, but not input/composition listener. Then, it's the widget code's responsibility to decide what action to take based on this flag. On Android, it means we should not use compositions and should send key events.

> >+#ifdef MOZ_WIDGET_ANDROID
> >+pref("intl.ime.key_event_listener_compatibility", true);
> >+#else
> >+pref("intl.ime.key_event_listener_compatibility", false);
> >+#endif
> 
> I don't like this pref name because this is unclear what the
> "key_event_listener_compatibility" actually changes.

I renamed the pref 'intl.ime.set_input_context_has_listener_flag', which corresponds to the mEditorHasKeyButNotInputOrCompositionListener flag. We also now use Preferences::AddBoolVarChache().

> >+      }
> >+      haveKeyEventsListener |= mgr->MayHaveKeyEventListener();
> >+    }
> >+    aNode = aNode->GetParentNode();
> >+  }
> >+
> >+  // If we don't have input/composition listeners but have key listeners, we
> >+  // should use key events mode if enabled.
> >+  return haveKeyEventsListener;
> 
> Ah, does this mean if one of |mgr->MayHaveKeyEventListener()| returns true,
> then, always returns true? Then, in such case, you can quit the loop earlier.

We don't always return true even if |mgr->MayHaveKeyEventListener()| is true, because an ancestor could return true for |mgr->MayHaveInputOrCompositionEventListener()|, in which case this function will return false.
Attachment #8540465 - Attachment is obsolete: true
Attachment #8540465 - Flags: review?(bugs)
Attachment #8542733 - Flags: review?(masayuki)
Attachment #8542733 - Flags: review?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> Comment on attachment 8539508 [details] [diff] [review]
> Implement use-key-events mode for Android (v1)
> 
> >+    } else if (aContext.mUseKeyEvents) {
> >+        // On Android, the "use-key-events" mode is the same as the "plugin"
> >+        // mode, so swich to plugin mode if we are forcing key events.
> >+        enabled = IMEState::PLUGIN;
> >     }
> 
> Hmm, in PLUGIN mode, does Android widget dispatch keypress events during
> composition? If so, you should change it for the new mode because D3E
> defines keydown and keyup events may be dispatched during composition but
> keypress events are not so.
> https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#events-
> composition-event-key-events

I don't think we send keypress events during composition. For example, when we enter 'abc', in normal mode, the IME will send: compositionstart -> compositionupdate('abc') -> compositionend. However, in plugin mode, the IME will send: keydown/press/up('a') -> keydown/press/up('b') -> keydown/press/up('c').
Comment on attachment 8542733 [details] [diff] [review]
Set flag in InputContext for inputs with key event but not input or composition event listeners (v4)

Sorry for the delay due to the new year's vacation.

>+bool IMEStateManager::sSetInputContextHasListenerFlag = false;

Hmm, this and similar names are still not clear to me. How about sCheckIfIMEAwareWebApps? If web apps check neither input event nor composition event, that means that the web app isn't "IME aware".

>+  Preferences::AddBoolVarCache(
>+      &sSetInputContextHasListenerFlag,
>+      "intl.ime.set_input_context_has_listener_flag", false);

nit: indent should be two white spaces.

How about "intl.ime.hack.on_ime_unaware_apps.fire_key_events_during_composition"?

There are some "intl.tsf.hack.foo_bar". (TSF is an IME framework of Windows) So, starting with "intl.ime.hack." makes sense for consistency.

>@@ -764,16 +787,20 @@ IMEStateManager::SetIMEState(const IMEState& aState,
> 
>   NS_ENSURE_TRUE_VOID(aWidget);
> 
>   InputContext oldContext = aWidget->GetInputContext();
> 
>   InputContext context;
>   context.mIMEState = aState;
> 
>+  context.mEditorHasKeyButNotInputOrCompositionListener =
>+      context.mIMEState.IsEditable() && sSetInputContextHasListenerFlag &&
>+      HasKeyButNotInputOrCompositionListener(aContent);

nit: indent level.

>+#ifdef MOZ_WIDGET_ANDROID
>+pref("intl.ime.set_input_context_has_listener_flag", true);
>+#else
>+pref("intl.ime.set_input_context_has_listener_flag", false);
>+#endif

I wonder, even if this sets true, it doesn't work on the other platforms. So, you should document that here.

>+  /* True if the editor has content key event listeners attached but not
>+   * input/composiion event listeners attached. This enables a key-events-only
>+   * mode on Android for compatibility with pages relying on key listeners. */
>+  bool mEditorHasKeyButNotInputOrCompositionListener;

This is clear but too long. How about mMayBeIMEUnaware?
(In reply to Jim Chen [:jchen] from comment #25)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> > Comment on attachment 8539508 [details] [diff] [review]
> > Implement use-key-events mode for Android (v1)
> > 
> > >+    } else if (aContext.mUseKeyEvents) {
> > >+        // On Android, the "use-key-events" mode is the same as the "plugin"
> > >+        // mode, so swich to plugin mode if we are forcing key events.
> > >+        enabled = IMEState::PLUGIN;
> > >     }
> > 
> > Hmm, in PLUGIN mode, does Android widget dispatch keypress events during
> > composition? If so, you should change it for the new mode because D3E
> > defines keydown and keyup events may be dispatched during composition but
> > keypress events are not so.
> > https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#events-
> > composition-event-key-events
> 
> I don't think we send keypress events during composition. For example, when
> we enter 'abc', in normal mode, the IME will send: compositionstart ->
> compositionupdate('abc') -> compositionend. However, in plugin mode, the IME
> will send: keydown/press/up('a') -> keydown/press/up('b') ->
> keydown/press/up('c').

Wait, so, do you mean that in the hacky mode, nsWindow for Android doesn't dispatch composition events? If so, it's too bad for nsEditor. nsEditor needs composition events for showing composing string in the editor.
(In reply to Jim Chen [:jchen] from comment #24)
> > >+      }
> > >+      haveKeyEventsListener |= mgr->MayHaveKeyEventListener();
> > >+    }
> > >+    aNode = aNode->GetParentNode();
> > >+  }
> > >+
> > >+  // If we don't have input/composition listeners but have key listeners, we
> > >+  // should use key events mode if enabled.
> > >+  return haveKeyEventsListener;
> > 
> > Ah, does this mean if one of |mgr->MayHaveKeyEventListener()| returns true,
> > then, always returns true? Then, in such case, you can quit the loop earlier.
> 
> We don't always return true even if |mgr->MayHaveKeyEventListener()| is
> true, because an ancestor could return true for
> |mgr->MayHaveInputOrCompositionEventListener()|, in which case this function
> will return false.

Ah, absolutely! Thanks!
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> Comment on attachment 8542733 [details] [diff] [review]
> Set flag in InputContext for inputs with key event but not input or
> composition event listeners (v4)
> 
> Hmm, this and similar names are still not clear to me. How about
> sCheckIfIMEAwareWebApps? If web apps check neither input event nor
> composition event, that means that the web app isn't "IME aware".

Ok, I like "IME aware". But I think the static bool variable, the pref, and the InputContext member should all have the same names to avoid confusion? What do you think about:

"sCheckForIMEUnawareWebApps" for the static bool variable
"intl.ime.hack.check_for_ime_unaware_webapps" for the pref
"InputContext::mMayBeIMEUnaware" for the InputContext member

> >+#ifdef MOZ_WIDGET_ANDROID
> >+pref("intl.ime.set_input_context_has_listener_flag", true);
> >+#else
> >+pref("intl.ime.set_input_context_has_listener_flag", false);
> >+#endif
> 
> I wonder, even if this sets true, it doesn't work on the other platforms.
> So, you should document that here.

I added a comment to say it's Android-specific.
Attachment #8542733 - Attachment is obsolete: true
Attachment #8542733 - Flags: review?(masayuki)
Attachment #8542733 - Flags: review?(bugs)
Attachment #8545022 - Flags: review?(masayuki)
Attachment #8545022 - Flags: review?(bugs)
(In reply to Jim Chen [:jchen] from comment #29)
> Created attachment 8545022 [details] [diff] [review]
> Set flag in InputContext for IME-unaware webapps (v5)
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> > Comment on attachment 8542733 [details] [diff] [review]
> > Set flag in InputContext for inputs with key event but not input or
> > composition event listeners (v4)
> > 
> > Hmm, this and similar names are still not clear to me. How about
> > sCheckIfIMEAwareWebApps? If web apps check neither input event nor
> > composition event, that means that the web app isn't "IME aware".
> 
> Ok, I like "IME aware". But I think the static bool variable, the pref, and
> the InputContext member should all have the same names to avoid confusion?

I don't think so.

When a developer reads the implementation of IMEStateManager, the static variable should represent what IMEStateManager behave.

When a heavy user or a developer sees the pref name, the pref name should represent what occurs after changing the pref.

When a developer reads the widget code, the InputContext member should represent the state of focused content. Therefore, I suggested different names. How do you think the names from this angle?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> (In reply to Jim Chen [:jchen] from comment #29)
> > Created attachment 8545022 [details] [diff] [review]
> > Set flag in InputContext for IME-unaware webapps (v5)
> > 
> > Ok, I like "IME aware". But I think the static bool variable, the pref, and
> > the InputContext member should all have the same names to avoid confusion?
> 
> I don't think so.
> 
> When a developer reads the implementation of IMEStateManager, the static
> variable should represent what IMEStateManager behave.
> 
> When a heavy user or a developer sees the pref name, the pref name should
> represent what occurs after changing the pref.
> 
> When a developer reads the widget code, the InputContext member should
> represent the state of focused content. Therefore, I suggested different
> names. How do you think the names from this angle?

Yeah that sounds reasonable. I updated to the following names:

"sCheckForIMEUnawareWebApps" for the IMEStateManager static member.
"intl.ime.hack.on_ime_unaware_apps.fire_key_events_for_composition" for the pref.
"mMayBeIMEUnaware" for the InputContext member.
Attachment #8545022 - Attachment is obsolete: true
Attachment #8545022 - Flags: review?(masayuki)
Attachment #8545022 - Flags: review?(bugs)
Attachment #8545343 - Flags: review?(masayuki)
Comment on attachment 8545343 [details] [diff] [review]
Set flag in InputContext for IME-unaware webapps (v6)

Thanks, looks great to me.
Attachment #8545343 - Flags: review?(masayuki) → review+
So this bug introduced a regression, bug 1121430. Since Jim is away, is anybody else willing to step up and take a look here? Or should I just back it out?
Comment on attachment 8539508 [details] [diff] [review]
Implement use-key-events mode for Android (v1)

>+    } else if (aContext.mUseKeyEvents) {
>+        // On Android, the "use-key-events" mode is the same as the "plugin"
>+        // mode, so swich to plugin mode if we are forcing key events.
>+        enabled = IMEState::PLUGIN;
>     }
> 
>+    mInputContext = aContext;
>     mInputContext.mIMEState.mEnabled = enabled;

This is bad approach. This means that you changed the result of nsIWidget::GetInputContext(). You should check |mUseKeyEvents| at handling native key events.
Masayuki, would you be able to write a patch to fix it? I'm happy to test the patch to see if it fixes the problem I'm seeing.
Flags: needinfo?(masayuki)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)
> Masayuki, would you be able to write a patch to fix it? I'm happy to test
> the patch to see if it fixes the problem I'm seeing.

Unfortunately, I'm busy because of bug 917322 and bug 1119609. If it's okay after them, (perhaps, late 38 or 39?), I'd be able to work on this.
Flags: needinfo?(masayuki)
In that case I'll back out these patches for now as the regression is quite severe. Thanks!
I backed out the four patches on this bug in https://hg.mozilla.org/integration/mozilla-inbound/rev/a220d54c8c14
Status: RESOLVED → REOPENED
status-firefox38: fixed → affected
Resolution: FIXED → ---
Eugen - Snorp asked me to throw this at you. Welcome to IME bugs
Assignee: nchen → esawin
Depends on: 1122345
I think that this idea is very good quirk for keeping compatibility with existing bad web pages and conforming D3E spec. Therefore, I think that this should be fixed in TextEventDispatcher. Then, all platforms will support this feature when bug 1137560 is fixed.
Depends on: 1137567, 1137560
Assignee: esawin → nchen
I am sorry, how is this Fx Android bug related to B2G mozInputMethod API impl (bug 1137557)?
Flags: needinfo?(masayuki)
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #43)
> I am sorry, how is this Fx Android bug related to B2G mozInputMethod API
> impl (bug 1137557)?

Nothing. nsITextInputProcessor is a client of TextEventDispatcher. So, B2G already uses TextEventDispatcher (for composition).
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #44)
> Nothing. nsITextInputProcessor is a client of TextEventDispatcher. So, B2G
> already uses TextEventDispatcher (for composition).

I assume the dependency is added by mistake then?
No longer depends on: 1137557
Let's fix this bug first. We can follow up in bug 1137567, when we switch to TextEventDispatcher later.

When the input has key event listeners, instead of using a key-events-only mode, this patch makes us send dummy key events with keycode 0, so that scripts can be triggered on text input.
Attachment #8539508 - Attachment is obsolete: true
Attachment #8614174 - Flags: review?(esawin)
Comment on attachment 8614174 [details] [diff] [review]
Implement dummy-key-events mode for Android (v1)

Review of attachment 8614174 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming that we don't trigger any unexpected behaviour with the 0 keyCodes, this looks OK.
Attachment #8614174 - Flags: review?(esawin) → review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: