Redesign IME APIs of nsIWidget for mobile devices

RESOLVED FIXED in mozilla11

Status

()

Core
Widget
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({feature, inputmethod})

Trunk
mozilla11
feature, inputmethod
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 18 obsolete attachments)

5.12 KB, patch
Details | Diff | Splinter Review
64.64 KB, patch
Details | Diff | Splinter Review
66.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
58.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
33.81 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.85 KB, patch
dougt
: review+
smaug
: review+
roc
: review+
Details | Diff | Splinter Review
1.55 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.07 KB, patch
roc
: review+
Walter Meinl
: feedback+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 559358 [details] [diff] [review]
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.
Attachment #559358 - Flags: superreview?(matspal)
Attachment #559358 - Flags: review?(roc)
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
Attachment #559358 - Flags: superreview?(matspal) → superreview+
Attachment #559358 - Flags: review?(roc) → review+
(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.
Created attachment 560364 [details] [diff] [review]
Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())
Attachment #559358 - Attachment is obsolete: true
Created attachment 560369 [details] [diff] [review]
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.
Attachment #560369 - Flags: review?(roc)
Created attachment 560370 [details] [diff] [review]
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.
Attachment #560370 - Flags: superreview?(matspal)
Attachment #560370 - Flags: review?(roc)
Created attachment 560372 [details] [diff] [review]
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.
Attachment #560372 - Flags: review?(roc)
Created attachment 560374 [details] [diff] [review]
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.
Attachment #560374 - Flags: review?(roc)
Created attachment 560378 [details] [diff] [review]
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.
Created attachment 560380 [details] [diff] [review]
part.7 android widget should check software keyboard open state in SetInputMode()
Created attachment 560381 [details] [diff] [review]
Patch part.8 gtk widget should check software keyboard open state in SetInputMode()
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
Attachment #560370 - Flags: superreview?(matspal) → superreview+
Created attachment 560425 [details] [diff] [review]
Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())

oops, I forgot to remove the comment in mozqtwidget.
Attachment #560364 - Attachment is obsolete: true
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.
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.
Created attachment 563287 [details] [diff] [review]
Patch part.2 Move IMEContext to mozilla::widget::InputContext
Attachment #560369 - Attachment is obsolete: true
Attachment #560370 - Attachment is obsolete: true
Attachment #560372 - Attachment is obsolete: true
Attachment #560374 - Attachment is obsolete: true
Attachment #560378 - Attachment is obsolete: true
Attachment #560380 - Attachment is obsolete: true
Attachment #560381 - Attachment is obsolete: true
Attachment #560369 - Flags: review?(roc)
Attachment #560370 - Flags: review?(roc)
Attachment #560372 - Flags: review?(roc)
Attachment #560374 - Flags: review?(roc)
Attachment #563287 - Flags: review?(roc)
Created 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
Attachment #563290 - Flags: review?(roc)
Created attachment 563291 [details] [diff] [review]
Patch part.4 IME open state should be able to set/get by InputContext
Attachment #563291 - Flags: review?(roc)
Created attachment 563293 [details] [diff] [review]
Patch part4 (-w)
Created attachment 563294 [details] [diff] [review]
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.
Attachment #563294 - Flags: review?(roc)
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
Attachment #563287 - Flags: review?(roc) → review+
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?
Created attachment 564464 [details] [diff] [review]
Patch part.1 Remove obsolete APIs (GetIMEEnabled() and SetIMEEnabled())
Attachment #560425 - Attachment is obsolete: true
Created attachment 564465 [details] [diff] [review]
Patch part.2 Move IMEContext to mozilla::widget::InputContext
Attachment #563287 - Attachment is obsolete: true
(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.
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#338

set by this logic.
(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.
(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.
Created attachment 564740 [details] [diff] [review]
Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument

Here is the latest patch.
Attachment #563290 - Attachment is obsolete: true
Attachment #563290 - Flags: review?(roc)
Attachment #564740 - Flags: review?(roc)
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.
Attachment #563291 - Flags: review?(roc) → review-
Attachment #563294 - Flags: review?(roc) → review-
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.
(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.
Created attachment 564861 [details] [diff] [review]
Patch part.3 Rename SetInputMode()/GetInputMode() to SetInputContext()/GetInputContext() and make SetInputContext() take the reason by a separated argument
Attachment #563291 - Attachment is obsolete: true
Attachment #563293 - Attachment is obsolete: true
Attachment #563294 - Attachment is obsolete: true
Attachment #564740 - Attachment is obsolete: true
Attachment #564740 - Flags: review?(roc)
Attachment #564861 - Flags: review?(roc)
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?
(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.
Created attachment 566413 [details] [diff] [review]
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.
Attachment #564861 - Attachment is obsolete: true
Attachment #564861 - Flags: review?(roc)
Attachment #566413 - Flags: review?(roc)
Created attachment 566415 [details] [diff] [review]
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.
Attachment #566415 - Flags: review?(roc)
Created attachment 566417 [details] [diff] [review]
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 ).
Attachment #566417 - Flags: review?(roc)
> This can prevent some developers be confused by the difference.

I meant that that helps some developers who are confused by the difference.
Blocks: 674770
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.
Attachment #566413 - Flags: superreview?(Olli.Pettay)
Attachment #566413 - Flags: review?(roc)
Attachment #566413 - Flags: review+
Attachment #566415 - Flags: review?(roc) → review+
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.
No longer blocks: 674770
Depends on: 674770
Created attachment 573424 [details] [diff] [review]
Patch part.5 Reomve IME state in nsIContent
Attachment #566417 - Attachment is obsolete: true
Attachment #566417 - Flags: review?(roc)
Attachment #573424 - Flags: review?(roc)
Created attachment 573425 [details] [diff] [review]
Patch part.6 Notify mouse click event on editor to widget
Attachment #573424 - Flags: review?(roc) → review+
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.
Attachment #566413 - Flags: superreview?(bugs) → superreview?(matspal)
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
Attachment #573424 - Flags: superreview?(matspal)
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.
Attachment #573425 - Flags: review?(roc)
Attachment #573425 - Flags: review?(doug.turner)
Attachment #573425 - Flags: review?(bugs)

Updated

6 years ago
Attachment #566413 - Flags: superreview?(matspal) → superreview+

Updated

6 years ago
Attachment #573424 - Flags: superreview?(matspal) → superreview+
Attachment #573425 - Flags: review?(roc) → review+

Updated

6 years ago
Attachment #573425 - Flags: review?(doug.turner) → review+
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.

Updated

6 years ago
Attachment #573425 - Flags: review?(bugs) → review+
Blocks: 507987
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
Flags: in-testsuite-
Whiteboard: [inbound]
Target Milestone: --- → mozilla11

Updated

6 years ago
Depends on: 705589
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]

Comment 50

6 years ago
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
Same here...  Tried clobber build, doesn't help.
Created attachment 577323 [details] [diff] [review]
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.
Attachment #577323 - Flags: review?(roc)
Attachment #577323 - Flags: review?(roc) → review+
Landed followup on inbound:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf9f0eed660
https://hg.mozilla.org/mozilla-central/rev/6cf9f0eed660

Comment 55

6 years ago
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();
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'
Created attachment 577866 [details] [diff] [review]
Fix bustage on OS/2
Attachment #577866 - Flags: review?(roc)
Attachment #577866 - Flags: feedback?(wuno)
: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

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

yep, un-breaker thanks
Attachment #577866 - Flags: feedback?(wuno) → feedback+
Attachment #577866 - Flags: review?(roc) → review+
OS/2 patch is landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d99b67f57d
> Did you build it cleanly?

Thanks, it was a bad merge on my part, leaving in some of the old stuff.
https://hg.mozilla.org/mozilla-central/rev/49d99b67f57d

Updated

6 years ago
Keywords: feature
You need to log in before you can comment on or make changes to this bug.