685395 - Redesign IME APIs of nsIWidget for mobile devices

Status: RESOLVED FIXED

(Core :: Widget, defect)

Comment 1

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

Attachment: Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())

Hmm, there are still being there the obsolete APIs which were obsolete after API frozen for Fx4.

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 559358 [details] [diff] [review]
Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())

There's a comment reference to GetIMEEnabled() here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/mozqwidget.cpp#65
You should probably just remove the second sentence.

Would it make sense to rename the following GetIMEEnabled() to GetInputMode() ?
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#174
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.h#106
http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.cpp#440

Comment 3

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

(In reply to Mats Palmgren [:mats] from comment #2)
> Would it make sense to rename the following GetIMEEnabled() to
> GetInputMode() ?
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#174
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.h#106
> http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/
> PuppetWidget.cpp#440

Sure. I fixed it by part.3.

Comment 4

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

Attachment: Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())

Comment 5

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

Attachment: Patch part.2 Sort out IME enabled state

Sorts out the IME enabled state stuff.

I think that IMEContext should be renamed to InputContext. It can hold other information (e.g., <input>'s information) which isn't related to IME. And now, the related APIs are not include "IME".

And I think that the IME enabled states should be defined in InputContext rather than nsIWidget.

Finally, InputContext should be in mozilla::widget namespace. If it should be in global namespace, it should have prefix "ns" but it's obsolete.

# SetInputMode() and GetInputMode() should be renamed to SetInputContext() and GetInputContext()? If so, I'll add a new patch for it.

Comment 6

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

Attachment: Patch part.3 IME open state should be able to set/get by InputContext

This patch combines IME open state to other bits of IME enabled state.

If you have better idea for the method names of InputContext, let me know.

And this patch removes GetIMEOpenState() and SetIMEOpenState() from nsIWidget. So, this patch needs sr.

Comment 7

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

Attachment: Patch part.4 Reomve IME state in nsIContent

This patch removes nsIContent's IME state. It doesn't make sense for now because part.3 combines IME open state to IME enabled state like this. So, these state users can use InputContext's IME state directly.

Comment 8

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

Attachment: Patch part.5 Software keyboard open state should be able to set/get by InputContext

This adds software keyboard open state to IME enabled/open state too. And mobile developers can use nsIMEStateManager::UpdateVKBOpenState() instead of nsIMEStateManager::OnChangeFocus(). The new method is safer.

Comment 9

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

Attachment: Patch part.6 qt widget should check software keyboard open state in SetInputMode()

I'll request review for this after roc's reviews are finished.

Comment 10

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

Attachment: part.7 android widget should check software keyboard open state in SetInputMode()

Comment 11

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

Attachment: Patch part.8 gtk widget should check software keyboard open state in SetInputMode()

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 560370 [details] [diff] [review]
Patch part.3 IME open state should be able to set/get by InputContext

> widget/public/nsIWidget.h
>    * WARNING: If you change these values, you also need to edit:
>    *   nsIDOMWindowUtils.idl
>    *   nsDOMWindowUtils::SetIMEEnabled
>    *   nsContentUtils::GetWidgetStatusFromIMEStatus
>    */

I can't find any nsDOMWindowUtils::SetIMEEnabled so this comment
needs updating.


In dom/interfaces/base/nsIDOMWindowUtils.idl

  /**
   * Get IME open state. TRUE means 'Open', otherwise, 'Close'.
   * This property works only when IMEEnabled is IME_STATUS_ENABLED.
   */

"IMEEnabled" should be "IMEStatus" in that comment?

The rest looks fine to me.  sr=mats

Comment 13

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

Attachment: Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())

oops, I forgot to remove the comment in mozqtwidget.

Comment 14

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

Comment on attachment 560369 [details] [diff] [review]
Patch part.2 Sort out IME enabled state

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

Looks like this patch doesn't change anything except move IMEContext to mozilla::widget::InputContext? If so, "Sort out IME enabled state" is not a good description. Just say "Move IMEContext to mozilla::widget::InputContext".

Renaming Set/GetInputMode to Set/GetInputContext sounds good. While you're at it, it's probably worth having SetInputContext return void and GetInputContext return the struct directly.

::: widget/src/xpwidgets/nsBaseWidget.h
@@ +146,5 @@
>    virtual nsresult        ForceUpdateNativeMenuAt(const nsAString& indexString) { return NS_ERROR_NOT_IMPLEMENTED; }
>    NS_IMETHOD              ResetInputState() { return NS_OK; }
>    NS_IMETHOD              SetIMEOpenState(PRBool aState) { return NS_ERROR_NOT_IMPLEMENTED; }
>    NS_IMETHOD              GetIMEOpenState(PRBool* aState) { return NS_ERROR_NOT_IMPLEMENTED; }
> +  NS_IMETHOD              SetInputMode(const mozilla::widget::InputContext& aContext) { return NS_ERROR_NOT_IMPLEMENTED; }

Add "typedef mozilla::widget::InputContext InputContext;" to nsBaseWidget avoid having to write mozilla::widget:: all over the place.

Comment 15

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

It's ugly to have some flags that are only valid for SetInputContext and other flags that are only valid for GetInputContext.

And I'm still not convinced that having specific API for mobile keyboards is the right way to go.

I'd really like to have a model where layout/content simply tell nsIWidget as much as possible about the user's actions and what content the user has focused, and widget is responsible for choosing the IME/keyboard state. In that model, InputContext would convey all the information about what the user has focused and how they got into that state.

Comment 16

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

Attachment: Patch part.2 Move IMEContext to mozilla::widget::InputContext

Comment 17

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

Attachment: Patch part.3 Bug 685395 part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

Comment 18

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

Attachment: Patch part.4 IME open state should be able to set/get by InputContext

Comment 19

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

Attachment: Patch part4 (-w)

Comment 20

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

Attachment: Patch part.5 Reomve IME state in nsIContent

I'll create the other patches for software keyboard (without changing nsIWidget) after these patches are +ed.

Comment 21

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

Comment on attachment 563287 [details] [diff] [review]
Patch part.2 Move IMEContext to mozilla::widget::InputContext

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

::: widget/src/cocoa/nsChildView.h
@@ +552,1 @@
>  

Indent mInputContext to align with mView/mParentView

Comment 22

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

Comment on attachment 563290 [details] [diff] [review]
Patch part.3 Bug 685395 part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

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

::: widget/public/nsIWidget.h
@@ +298,5 @@
> +  friend class mozilla::dom::TabParent;
> +  friend class mozilla::widget::PuppetWidget;
> +
> +private:
> +  PRUint32 mAction;

Since you're using a struct here anyway, simpler to just have separate fields for the source, the focus info, and the process info.

@@ +304,5 @@
> +public:
> +  enum {
> +    SOURCE_VALUE_MASK =                     0x00FF,
> +    // The source is unknown. Might be changed by content's script.
> +    BY_UNKNOWN =                            0x0000,

Let's call this the "cause" instead of the source.

And instead of BY_, use CAUSE_UNKNOWN etc.

@@ +311,5 @@
> +    BY_MOVE_FOCUS =                         0x0001,
> +    // The source is user's keyboard operation.
> +    BY_KEY =                                0x0002,
> +    // The source is user's mouse operation.
> +    BY_MOUSE =                              0x0003,

Are these any keyboard and mouse operation, including e.g. entering test? or only for focus changes caused by keyboard/mouse operation?

@@ +379,5 @@
> +  }
> +
> +  PRBool IsCallerContentProcess() const {
> +    return (GetCallerProcessType() == CALLER_IS_CONTENT_PROCESS);
> +  }

Is it really the *process* that matters here, or is it actually whether the caller was chrome that matters?

Comment 23

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

Attachment: Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())

Comment 24

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

Attachment: Patch part.2 Move IMEContext to mozilla::widget::InputContext

Comment 25

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> @@ +311,5 @@
> > +    BY_MOVE_FOCUS =                         0x0001,
> > +    // The source is user's keyboard operation.
> > +    BY_KEY =                                0x0002,
> > +    // The source is user's mouse operation.
> > +    BY_MOUSE =                              0x0003,
> 
> Are these any keyboard and mouse operation, including e.g. entering test? or
> only for focus changes caused by keyboard/mouse operation?

I'm not sure what did you mean "entering test"...

When nsIMEStateManager needs to call nsIWidget::SetInputContext(), it should set the latest reason. E.g., if keyboard/mouse event handler of content's script causes it, the keyboard/mouse value shouldn't be set.

MOVE_FOCUS may cause mislead, yes. It might not be necessary (probably, it can be CAUSE_UNKNOWN) because the new struct has focus behavior. Let me check it.

> @@ +379,5 @@
> > +  }
> > +
> > +  PRBool IsCallerContentProcess() const {
> > +    return (GetCallerProcessType() == CALLER_IS_CONTENT_PROCESS);
> > +  }
> 
> Is it really the *process* that matters here, or is it actually whether the
> caller was chrome that matters?

http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/nsWindow.cpp#3283
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkIMModule.cpp#595
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsWindow.cpp#1864

If the content process's non-user-input causes SetInputContext(), they don't open software keyboard now.

Comment 26

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

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#338

set by this logic.

Comment 27

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #25)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> > @@ +311,5 @@
> > > +    BY_MOVE_FOCUS =                         0x0001,
> > > +    // The source is user's keyboard operation.
> > > +    BY_KEY =                                0x0002,
> > > +    // The source is user's mouse operation.
> > > +    BY_MOUSE =                              0x0003,
> > 
> > Are these any keyboard and mouse operation, including e.g. entering test? or
> > only for focus changes caused by keyboard/mouse operation?
> 
> I'm not sure what did you mean "entering test"...

Sorry, I meant "entering text".

If the user moves focus with the keyboard, e.g. by pressing "tab", is the cause going to be BY_KEY or BY_MOVE_FOCUS?

Are there keypresses that don't move the focus but cause a call to SetInputContext? If so, what cause value should be passed?

> When nsIMEStateManager needs to call nsIWidget::SetInputContext(), it should
> set the latest reason. E.g., if keyboard/mouse event handler of content's
> script causes it, the keyboard/mouse value shouldn't be set.

Makes sense.

> > @@ +379,5 @@
> > > +  }
> > > +
> > > +  PRBool IsCallerContentProcess() const {
> > > +    return (GetCallerProcessType() == CALLER_IS_CONTENT_PROCESS);
> > > +  }
> > 
> > Is it really the *process* that matters here, or is it actually whether the
> > caller was chrome that matters?
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/nsWindow.cpp#3283
> http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkIMModule.
> cpp#595
> http://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsWindow.
> cpp#1864
> 
> If the content process's non-user-input causes SetInputContext(), they don't
> open software keyboard now.

Right but that doesn't answer my question. What if chrome JS in the content process moves focus, the virtual keyboard should still be activated right? I think it's really "chrome vs content" here, the process itself isn't important.

Comment 28

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #25)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> > > @@ +311,5 @@
> > > > +    BY_MOVE_FOCUS =                         0x0001,
> > > > +    // The source is user's keyboard operation.
> > > > +    BY_KEY =                                0x0002,
> > > > +    // The source is user's mouse operation.
> > > > +    BY_MOUSE =                              0x0003,
> > > 
> > > Are these any keyboard and mouse operation, including e.g. entering test? or
> > > only for focus changes caused by keyboard/mouse operation?
> > 
> > I'm not sure what did you mean "entering test"...
> 
> Sorry, I meant "entering text".
> 
> If the user moves focus with the keyboard, e.g. by pressing "tab", is the
> cause going to be BY_KEY or BY_MOVE_FOCUS?

Then, BY_KEY. I removed BY_MOVE_FOCUS from latest patch.

> > > @@ +379,5 @@
> > > > +  }
> > > > +
> > > > +  PRBool IsCallerContentProcess() const {
> > > > +    return (GetCallerProcessType() == CALLER_IS_CONTENT_PROCESS);
> > > > +  }
> > > 
> > > Is it really the *process* that matters here, or is it actually whether the
> > > caller was chrome that matters?
> > 
> > http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/nsWindow.cpp#3283
> > http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkIMModule.
> > cpp#595
> > http://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsWindow.
> > cpp#1864
> > 
> > If the content process's non-user-input causes SetInputContext(), they don't
> > open software keyboard now.
> 
> Right but that doesn't answer my question. What if chrome JS in the content
> process moves focus, the virtual keyboard should still be activated right? I
> think it's really "chrome vs content" here, the process itself isn't
> important.

Hmm, right. I think that the cause coming from trusted code or not is important.

Comment 29

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

Attachment: Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

Here is the latest patch.

Comment 30

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

Anyway, if comment 26's part is wrong, we should fix that in another bug. It shouldn't be in scope of this bug except renaming the mIsCallerContentProcess.

Comment 31

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

Comment on attachment 564740 [details] [diff] [review]
Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

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

::: widget/public/nsIWidget.h
@@ +301,5 @@
> +    CAUSE_KEY =                             0x1,
> +    // The cause is user's mouse operation.
> +    CAUSE_MOUSE =                           0x2
> +  };
> +  PRUint8 mCause;

Define the enum type as "Cause" and declare "Cause mCause;"

@@ +319,5 @@
> +    // Menu lost pseudo focus that means focused content will handle keyboard
> +    // events.
> +    MENU_LOST_PSEUDO_FOCUS =                0x4
> +  };
> +  PRUint8 mFocusBehavior;

Likewise, make the enum "FocusChange" .. and call this mFocusChange.

@@ +325,5 @@
> +  /**
> +   * mIsCallerContentProcess is TRUE if nsIWidget::SetInputContext() is called
> +   * by content process.  Otherwise, FALSE.
> +   */
> +  bool mIsCallerContentProcess;

I really think we should change this here instead of creating a new API with the wrong names and then fixing it.

For which values of mCause and mFocusChange will mIsCallerContentProcess (or whatever we decide to call it) relevant? Only for CAUSE_UNKNOWN?

If it's only CAUSE_UNKNOWN, then it might make sense to simply split CAUSE_UNKNOWN into CAUSE_UNKNOWN_CONTENT and CAUSE_UNKNOWN_CHROME.

Comment 32

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> Comment on attachment 564740 [details] [diff] [review] [diff] [details] [review]
> Patch part.3 Rename SetInputMode()/GetInputMode() to
> SetInputContext()/GetInputContext() and make SetInputContext() take the
> reason by a separated argument
> @@ +325,5 @@
> > +  /**
> > +   * mIsCallerContentProcess is TRUE if nsIWidget::SetInputContext() is called
> > +   * by content process.  Otherwise, FALSE.
> > +   */
> > +  bool mIsCallerContentProcess;
> 
> I really think we should change this here instead of creating a new API with
> the wrong names and then fixing it.

I agree only for changing new API design in this bug. But if we need to fix the user (nsIMEStateManager), that should be in separated bug I think. Because it changes the actual behavior. (I guess that when chrome changes the content focus, current code sets "is content process" to the context.)

> For which values of mCause and mFocusChange will mIsCallerContentProcess (or
> whatever we decide to call it) relevant? Only for CAUSE_UNKNOWN?
> 
> If it's only CAUSE_UNKNOWN, then it might make sense to simply split
> CAUSE_UNKNOWN into CAUSE_UNKNOWN_CONTENT and CAUSE_UNKNOWN_CHROME.

It does make sense. Thank you.

Comment 33

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

Attachment: Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

Comment 34

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

Comment on attachment 564861 [details] [diff] [review]
Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

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

What happens if content synthesizes a keyboard event or mouse event that changes focus? Does it get CAUSE_UNKNOWN_CONTENT or CAUSE_KEY/MOUSE?

Basically looking good!

::: content/events/src/nsIMEStateManager.cpp
@@ +251,5 @@
>    // commit current composition
>    widget->ResetInputState();
>  
> +  // XXX Needs additional param for this method because we don't know the cause
> +  //     is whether by chrome script or content script here.

We'd better do something about this?

Or perhaps we could change CAUSE_UNKNOWN_CONTENT to be CAUSE_UNKNOWN which means we don't know whether it's from chrome or content, but we don't trust it in any case?

@@ +384,5 @@
> +      (XRE_GetProcessType() == GeckoProcessType_Content) ? 0 : 1;
> +  }
> +
> +  return sIsTrustedProcess ? InputContextAction::CAUSE_UNKNOWN_CHROME :
> +                             InputContextAction::CAUSE_UNKNOWN_CONTENT;

You probably don't need to cache this, I'd just not bother.

::: content/events/src/nsIMEStateManager.h
@@ +67,5 @@
> +   * IME enabled state is changed.  If focus isn't actually changed and IME
> +   * enabled state isn't changed, this will do nothing.
> +   *
> +   * @param aAction         Caller must set the source.  The focus behavior and
> +   *                        the process type will be set automatically.

Why don't you just take InputContextAction::Cause here, then, and map CAUSE_UNKNOWN_* to the right value automatically?

::: dom/base/nsFocusManager.cpp
@@ +338,5 @@
>  }
>  
>  // static
> +InputContextAction
> +nsFocusManager::GetFocusMoveAction(PRUint32 aFlags)

Why not just return InputContextAction::Cause here?

::: widget/public/nsIWidget.h
@@ +294,5 @@
> +   * mCause indicates what action causes calling nsIWidget::SetInputContext().
> +   * It must be one of following values.
> +   */
> +  enum Cause {
> +    // The cause is unknown. Might be changed by chrome's script.

// The cause is unknown but originated from chrome. Focus might have been changed by chrome script.

@@ +296,5 @@
> +   */
> +  enum Cause {
> +    // The cause is unknown. Might be changed by chrome's script.
> +    CAUSE_UNKNOWN_CHROME =                  0x0,
> +    // The cause is unknown. Might be changed by content's script.

// The cause is unknown but originated from content. Focus might have been changed by content script.

@@ +301,5 @@
> +    CAUSE_UNKNOWN_CONTENT =                 0x1,
> +    // The cause is user's keyboard operation.
> +    CAUSE_KEY =                             0x2,
> +    // The cause is user's mouse operation.
> +    CAUSE_MOUSE =                           0x3

You don't actually need to assign these values, you can just use the automatic values. Same for FocusBehavior.

@@ +321,5 @@
> +    // Menu lost pseudo focus that means focused content will handle keyboard
> +    // events.
> +    MENU_LOST_PSEUDO_FOCUS =                0x4
> +  };
> +  FocusBehavior mFocusBehavior;

Let's call this FocusChange/mFocusChange.

::: widget/src/xpwidgets/nsBaseWidget.h
@@ +152,5 @@
> +  NS_IMETHOD_(InputContext) GetInputContext()
> +  {
> +    InputContext inputContext;
> +    return inputContext;
> +  }

Do we actually need default implementations of these? I assume every implementation overrides them?

Comment 35

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> What happens if content synthesizes a keyboard event or mouse event that
> changes focus? Does it get CAUSE_UNKNOWN_CONTENT or CAUSE_KEY/MOUSE?

Should be CAUSE_KEY/MOUSE, but I'll check it.

> ::: content/events/src/nsIMEStateManager.cpp
> @@ +251,5 @@
> >    // commit current composition
> >    widget->ResetInputState();
> >  
> > +  // XXX Needs additional param for this method because we don't know the cause
> > +  //     is whether by chrome script or content script here.
> 
> We'd better do something about this?
> 
> Or perhaps we could change CAUSE_UNKNOWN_CONTENT to be CAUSE_UNKNOWN which
> means we don't know whether it's from chrome or content, but we don't trust
> it in any case?

CAUSE_UNKNOWN sounds better. Yes, we shouldn't trust if the origin is unknown.

I realized that we should use nsContentUtils::IsCallerChrome() for distinguishing them. But it really changes the behavior. I think it should be done in another bug.

> ::: content/events/src/nsIMEStateManager.h
> @@ +67,5 @@
> > +   * IME enabled state is changed.  If focus isn't actually changed and IME
> > +   * enabled state isn't changed, this will do nothing.
> > +   *
> > +   * @param aAction         Caller must set the source.  The focus behavior and
> > +   *                        the process type will be set automatically.
> 
> Why don't you just take InputContextAction::Cause here, then, and map
> CAUSE_UNKNOWN_* to the right value automatically?

Ah, the latest patch includes nsIWidget.h from nsIMEStateManager.h. Yes, it should be about to do it.

> ::: widget/src/xpwidgets/nsBaseWidget.h
> @@ +152,5 @@
> > +  NS_IMETHOD_(InputContext) GetInputContext()
> > +  {
> > +    InputContext inputContext;
> > +    return inputContext;
> > +  }
> 
> Do we actually need default implementations of these? I assume every
> implementation overrides them?

OS/2 doesn't have it, but okay, it can have this method.

Comment 36

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

Attachment: Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

Sorry for the delay.

> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
>> What happens if content synthesizes a keyboard event or mouse event that
>> changes focus? Does it get CAUSE_UNKNOWN_CONTENT or CAUSE_KEY/MOUSE?
> 
> Should be CAUSE_KEY/MOUSE, but I'll check it.

I confirmed that a tab key event which is caused by nsIDOMWindowUtils::SendNativeKeyEvent() causes CAUSE_KEY.

Comment 37

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

Attachment: Patch part.4 IME open state should be able to set/get by InputContext

This patch merges IME open state into InputContext. And I think we should use IMEState struct rather than a bit field for IME enabled state and open state. This struct will make XP code simple in next patch.

Comment 38

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

Attachment: Patch part.5 Reomve IME state in nsIContent

This patch removes another IME state which is defined in nsIContent. This can prevent some developers be confused by the difference. (E.g., TabParent::RecvSetInputContext() in the previous patch -- https://bugzilla.mozilla.org/attachment.cgi?id=566415&action=diff#a/dom/ipc/TabParent.cpp_sec1 ).

Comment 39

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

> This can prevent some developers be confused by the difference.

I meant that that helps some developers who are confused by the difference.

Comment 40

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

Comment on attachment 566413 [details] [diff] [review]
Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

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

::: widget/public/nsIWidget.h
@@ +294,5 @@
> +   * mCause indicates what action causes calling nsIWidget::SetInputContext().
> +   * It must be one of following values.
> +   */
> +  enum Cause {
> +    // The cause is unknown but originated from content. Focus might have been

... originated from content or chrome.

Comment 41

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

Comment on attachment 566417 [details] [diff] [review]
Patch part.5 Reomve IME state in nsIContent

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

::: content/base/public/nsIContent.h
@@ +624,2 @@
>     */
> +  virtual mozilla::widget::IMEState GetDesiredIMEState();

add
  typedef mozilla::widget::IMEState IMEState;
to this class to avoid mozilla::widget:: prefixes.

::: content/base/src/nsGenericElement.cpp
@@ +1360,4 @@
>  nsIContent::GetDesiredIMEState()
>  {
>    if (!IsEditableInternal()) {
> +    return IMEState(IMEState::DISABLED, IMEState::DONT_CHANGE_OPEN_STATE);

You can use the default constructor parameter here and below.

Comment 42

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

Attachment: Patch part.5 Reomve IME state in nsIContent

Comment 43

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

Attachment: Patch part.6 Notify mouse click event on editor to widget

Comment 44

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

Comment on attachment 566413 [details] [diff] [review]
Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

mats sr'ed some patches in this bug, changing sr reviewer to mats.

mats:

widget doesn't need to store the last reason and it doesn't make sense that GetInputMode() returns the reason too. And we should rename the methods to GetInputContext() and SetInputContext(). The "InputMode" isn't used by others, it isn't good term for these APIs.

Comment 45

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

Comment on attachment 573424 [details] [diff] [review]
Patch part.5 Reomve IME state in nsIContent

nsIContent's IME state is removed by this patch. content can use nsIWidget's IME state. This avoids mistakes such as: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#585

Comment 46

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

Comment on attachment 573425 [details] [diff] [review]
Patch part.6 Notify mouse click event on editor to widget

When focused editor is left-clicked (only trusted event), we should notify the event to widget. And then, mobile widget should (re)open software keyboard.

This patch doesn't not check if the click event has been consumed by script. If some web applications prevent the default behavior of click event, user cannot open software keyboard. It will be a serious a11y problem on mobile platforms.

Note that this patch depends on the patch for bug 674770. The patch's going to refuse click events which are fired on non-editable element.

Comment 47

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

Smaug:

Would you review it? The point is, VKB can be opened forcibly even if the click event is consumed, for making sure of a11y.

Comment 48

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e14e141a1647
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c68967a6ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b2a561db2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/029a09f71d79
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c4d6ce4b9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/00bb426f75ef

try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ef518b90e59e

Comment 49

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e14e141a1647
https://hg.mozilla.org/mozilla-central/rev/a7c68967a6ed
https://hg.mozilla.org/mozilla-central/rev/a1b2a561db2b
https://hg.mozilla.org/mozilla-central/rev/029a09f71d79
https://hg.mozilla.org/mozilla-central/rev/98c4d6ce4b9e
https://hg.mozilla.org/mozilla-central/rev/00bb426f75ef

Comment 50

[Brad Jackson]

I'm getting an error building Firefox trunk with GCC 4.6.2 on Linux:

mozilla/widget/src/gtk2/nsWindow.cpp:6589:1: error: ‘InputContext’ does not name a type

Comment 51

[Daniel Holbert [:dholbert]]

Same here...  Tried clobber build, doesn't help.

Comment 52

[Daniel Holbert [:dholbert]]

Attachment: followup to fix build failure

nsIWidget.h defines InputContext in the mozilla::widget namespace.

gtk2/nsWindow.cpp doesn't know about the mozilla::widget namespace.

This patch fixes that.

Comment 53

[Daniel Holbert [:dholbert]]

Landed followup on inbound:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf9f0eed660

Comment 54

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/6cf9f0eed660

Comment 55

[Walter Meinl]

Comment on attachment 566413 [details] [diff] [review]
Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

>Bug 685395 part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument
>
>diff --git a/widget/src/os2/nsWindow.h b/widget/src/os2/nsWindow.h
>--- a/widget/src/os2/nsWindow.h
>+++ b/widget/src/os2/nsWindow.h
>@@ -211,16 +211,26 @@ public:
>                                             nsIMenuRollup* aMenuRollup,
>                                             bool aDoCapture, bool aConsumeRollupEvent);
>   NS_IMETHOD            GetToggledKeyState(PRUint32 aKeyCode,
>                                            bool* aLEDState);
>   NS_IMETHOD            DispatchEvent(nsGUIEvent* event,
>                                       nsEventStatus& aStatus);
>   NS_IMETHOD            ReparentNativeWidget(nsIWidget* aNewParent);
> 
>+  NS_IMETHOD_(void)     SetInputContext(const InputContext& aContext,
>+                                        const InputContextAction& aAction)
>+  {
>+    mInputContext = aInputContext;
shouldn't this read
>+    mInputContext = aContext;

with aInputContext I get a build break on OS/2 because of not being declared in this scope
>+  }
>+  NS_IMETHOD_(InputContext) GetInputContext()
>+  {
>+    return mInputContext;
>+  }
>+
>   // nsWindow
>   static void           ReleaseGlobals();
> protected:
>   // from nsBaseWidget
>   virtual void          OnDestroy();
> 
>   // nsWindow
>   static void           InitGlobals();

Comment 56

[David Humphrey (:humph)]

My Linux build is dying on what looks like fallout from this patch:

c++ -o nsBaseWidget.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /home/dave/repos/mozilla-central/config/gcc_hidden.h -D_IMPL_NS_WIDGET -DNO_NSPR_10_SUPPORT=1  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux2.6.34.9-69.fc13\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_LINUX=1 -DOS_POSIX=1  -I/home/dave/repos/mozilla-central/widget/src/xpwidgets/../gtk2 -I/home/dave/repos/mozilla-central/widget/src/xpwidgets/../shared -I/home/dave/repos/mozilla-central/layout/forms -I/home/dave/repos/mozilla-central/layout/generic -I/home/dave/repos/mozilla-central/layout/xul/base/src -I/home/dave/repos/mozilla-central/widget/src/xpwidgets  -I/home/dave/repos/mozilla-central/ipc/chromium/src -I/home/dave/repos/mozilla-central/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/home/dave/repos/mozilla-central/widget/src/xpwidgets -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/home/dave/repos/mozilla-central/objdir-debug/dist/include/nspr -I/home/dave/repos/mozilla-central/objdir-debug/dist/include/nss      -fPIC -fno-rtti -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -fno-exceptions -fno-strict-aliasing -fshort-wchar -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0     -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/nsBaseWidget.pp /home/dave/repos/mozilla-central/widget/src/xpwidgets/nsBaseWidget.cpp
/home/dave/repos/mozilla-central/widget/src/xpwidgets/nsBaseWidget.cpp:1187: error: no ‘nsresult nsBaseWidget::SetIMEEnabled(PRUint32)’ member function declared in class ‘nsBaseWidget’
/home/dave/repos/mozilla-central/widget/src/xpwidgets/nsBaseWidget.cpp:1195: error: no ‘nsresult nsBaseWidget::GetIMEEnabled(PRUint32*)’ member function declared in class ‘nsBaseWidget’
make[3]: *** [nsBaseWidget.o] Error 1
make[3]: Leaving directory `/home/dave/repos/mozilla-central/objdir-debug/widget/src/xpwidgets'
make[2]: *** [libs] Error 2
make[2]: Leaving directory `/home/dave/repos/mozilla-central/objdir-debug/widget/src'
make[1]: *** [libs] Error 2
make[1]: Leaving directory `/home/dave/repos/mozilla-central/objdir-debug/widget'
make: *** [default] Error 2
make: Leaving directory `/home/dave/repos/mozilla-central/objdir-debug/widget'

Comment 57

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

Attachment: Fix bustage on OS/2

Comment 58

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

:humph

Did you build it cleanly?

There are no implementation SetIMEEnabled() and GetIMEEnabled() now.
http://mxr.mozilla.org/mozilla-central/search?string=SetIMEEnabled
http://mxr.mozilla.org/mozilla-central/search?string=GetIMEEnabled

Comment 59

[Walter Meinl]

Comment on attachment 577866 [details] [diff] [review]
Fix bustage on OS/2

yep, un-breaker thanks

Comment 60

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

OS/2 patch is landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d99b67f57d

Comment 61

[David Humphrey (:humph)]

> Did you build it cleanly?

Thanks, it was a bad merge on my part, leaving in some of the old stuff.

Comment 62

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/49d99b67f57d