Closed Bug 532738 Opened 15 years ago Closed 13 years ago

Do not open the virtual keyboard on untrusted focus (caused by content page scripts)

Categories

(Firefox for Android Graveyard :: General, defect, P3)

defect

Tracking

(firefox5- affected, firefox6 fixed, fennec6+)

VERIFIED FIXED
Firefox 6
Tracking Status
firefox5 - affected
firefox6 --- fixed
fennec 6+ ---

People

(Reporter: madhava, Assigned: vingtetun)

References

(Depends on 1 open bug)

Details

(Whiteboard: [VKB] [fennec-4.1?][sg:want])

Attachments

(2 files, 11 obsolete files)

3.98 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
14.15 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
A number of sites do this (google, bugzilla).  It's meant to make it easier to start using the page, if using the particular field is what the user meant to do.  On a mobile device with only a softkeyboard, though, focusing an input field will bring up the keyboard, really minimizing the amount of the screen available for the page, so the trade-off is different.
tracking-fennec: --- → ?
I have a hack for that but it relies on playing with the isOffScreenBrowser property which i'm a bit worried about.
Attached patch Patch (obsolete) — Splinter Review
I'm not very convinced that we should land that just before RC, i'm not sure of the side impact.
Assignee: nobody → 21
Attachment #417893 - Flags: review?(mark.finkle)
tracking-fennec: ? → 1.0-
Madhava: how this should work with devices that have on-screen and real HW keyboards? Should we still disable the auto-focus when HW keyboard is available and exposed?
any decision on how we want to proceed with this fix/feature?
can we just handle properly user-focus(click) and dom/js-focus properly, and do nothing when page is setting autofocus?

similar approach used in N900 microb
[12:10pm] dougt: madhava: question.  i go to a page in fennec.  it sets the focus to a text field.  right now we bring up the VKB.

[12:10pm] dougt: madhava: this sucks because maybe the user wanted to just look at the page before entering anything.

[12:11pm] dougt: madhava: what do you think about only bringing up the vkb if the user clicks on the edit box

[12:11pm] madhava: dougt - yes - we should not be auto-focusing the field

[12:11pm] dougt: madhava: yes, it does have the drawback of not being able to quickly start typing into a text field.

[12:11pm] madhava: dougt - secondary consideration
[12:11pm] madhava: for us

[12:11pm] dougt: madhava: we don't auto-focus, the page does.

[12:11pm] madhava: given the drawback
[12:11pm] madhava: we should override
[12:11pm] madhava: if we can
[12:11pm] madhava: mobile safari does this
[12:12pm] madhava: dougt - or yes, we can just not show the keyboard until the user taps
[12:12pm] madhava: either's fine by me

[12:12pm] jbos: whats about the case where the autofocus is the entering point for a script / framework what ever.
[12:12pm] madhava: from a users perspective, those are the same, pretty much
[12:13pm] madhava: jbos - example?
[12:13pm] madhava: (it will help me think about that case)
What's happening here. Shall we change this to what is discussed in comment #6 or how do we do it?
Comment on attachment 417893 [details] [diff] [review]
Patch

My patch is not the way to go. Removing r?.
Attachment #417893 - Flags: review?(mark.finkle)
Summary: Don't auto-focus input fields on page load → [VKB]Don't auto-focus input fields on page load
Summary: [VKB]Don't auto-focus input fields on page load → Don't auto-focus input fields on page load
Whiteboard: [VKB]
tracking-fennec: 1.0- → ?
blocking2.0: --- → ?
tracking-fennec: ? → ---
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
blocking2.0: ? → ---
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0-
I just filed bug 623624, which is sort of in contradiction with the summary of this bug.
Vivian on irc explained this bug to me:
[15:35]	<vingtetun>	but the bug's name can be reformulate to " do not open the virtual keyboard on untrusted focus"
[15:36]	<vingtetun>	so to prevent that there is actually a "hack" in the code to remove the focus during load

I'm updating the summary of this bug, to reflect the goal of this bug (instead of the current state of the code).

This is the hack in the code that removes focus during pageshow event:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.js#206

Anyway, this hack is probably causing a couple of mochitest failures.
Summary: Don't auto-focus input fields on page load → Do not open the virtual keyboard on untrusted focus (caused by content page scripts)
Blocks: 623624
Depends on: 588313
tracking-fennec: 2.0- → ---
Whiteboard: [VKB] → [VKB] [fennec-4.1?]
Attached patch Patch (m-c) (obsolete) — Splinter Review
This patch prevent the IME to be changed (and so the VKB to popup) if the focus is not set with the mouse or a key
Attachment #417893 - Attachment is obsolete: true
Attached patch Patch (m-b) (obsolete) — Splinter Review
This patch add the necessary changes to m-b to work with the m-c patch. Mostly remove the old hack of clearing focus during a pageshow...
I guess it shouldn't be that hard to test, right?
Flags: in-testsuite?
(In reply to comment #15)
> I guess it shouldn't be that hard to test, right?

This is true, a browser-chrome test that check the ime-enabled-state-changed observer should be enough to do it. I will add that.
Attachment #521703 - Flags: review?(masayuki)
Comment on attachment 521703 [details] [diff] [review]
Patch (m-c)

I don't understand well about the condition, but I don't think that this is right fix. IME state must be modified every focus move.

I think that IMEContext should have a PRUint32 member for flags which indicate why nsIWidget::SetInputMode() is called. And widget should decide the behavior for the reasons.
Comment on attachment 521703 [details] [diff] [review]
Patch (m-c)

(In reply to comment #17)
> Comment on attachment 521703 [details] [diff] [review]
> I think that IMEContext should have a PRUint32 member for flags which indicate
> why nsIWidget::SetInputMode() is called. And widget should decide the behavior
> for the reasons.

This make sense to me, I will update the patch to reflect that.
Attachment #521703 - Flags: review?(masayuki)
Attachment #521703 - Flags: review?(Olli.Pettay)
Attached patch Patch v0.2 (m-c) (obsolete) — Splinter Review
Attachment #521703 - Attachment is obsolete: true
Attachment #523310 - Flags: review?(masayuki)
Attached patch Patch (m-b) - updated on trunk (obsolete) — Splinter Review
Attachment #521704 - Attachment is obsolete: true
Attachment #523311 - Flags: review?(mark.finkle)
Attachment #521704 - Flags: review?(mark.finkle)
Attached patch Patch v0.2 (m-c) (obsolete) — Splinter Review
Oups, I've forgot to update the MAEMO code path
Attachment #523310 - Attachment is obsolete: true
Attachment #523313 - Flags: review?(masayuki)
Attachment #523310 - Flags: review?(masayuki)
Comment on attachment 523311 [details] [diff] [review]
Patch (m-b) - updated on trunk

Do we have any tests for this?
Attachment #523311 - Flags: review?(mark.finkle) → review+
(In reply to comment #23)
> Comment on attachment 523311 [details] [diff] [review]
> Patch (m-b) - updated on trunk
> 
> Do we have any tests for this?

I will add some
Comment on attachment 523313 [details] [diff] [review]
Patch v0.2 (m-c)

>diff --git a/widget/public/nsIWidget.h b/widget/public/nsIWidget.h
>--- a/widget/public/nsIWidget.h
>+++ b/widget/public/nsIWidget.h
>@@ -230,16 +230,19 @@ struct nsIMEUpdatePreference {
> 
> /* 
>  * Contains IMEStatus plus information about the current 
>  * input context that the IME can use as hints if desired.
>  */
> struct IMEContext {
>   PRUint32 mStatus;
> 
>+  /* Does the change come from a trusted source */
>+  PRUint32 mTrusted;
>+
>   /* The type of the input if the input is a html input field */
>   nsString mHTMLInputType;
> 
>   /* A hint for the action that is performed when the input is submitted */
>   nsString mActionHint;
> };

I think that "mTrusted" isn't good name for me because IMEStateManager's . I think that you want to prevent to open VKB when the focus is changed by user actually. Not when the focus is moved by untrusted reason. How about:

enum {
  UNKNOWN = 0,
  FOCUS_MOVED_BY_MOUSE,
  FOCUS_MOVED_BY_KEY,
  FOCUS_MOVED_TO_MENU,
  FOCUS_MOVED_FROM_MENU,
  EDITOR_STATE_MODIFIED
}
PRUint32 mReason;

This is useful for other purpose too.

>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>--- a/content/events/src/nsIMEStateManager.cpp
>+++ b/content/events/src/nsIMEStateManager.cpp
>@@ -185,31 +187,39 @@ nsIMEStateManager::OnChangeFocus(nsPresC
>       oldWidget = widget;
>     else
>       oldWidget = GetWidget(sPresContext);
>     if (oldWidget)
>       oldWidget->ResetInputState();
>   }
> 
>   if (newState != nsIContent::IME_STATUS_NONE) {
>+    // Request to affect the IME from the content process not marked
>+    // started by the user are considered as untrusted
>+    PRBool isTrusted = PR_FALSE;
>+    if ((aReasons & nsIFocusManager::FLAG_BYMOUSE) ||
>+        (aReasons & nsIFocusManager::FLAG_BYKEY) ||
>+        (XRE_GetProcessType() == GeckoProcessType_Default))
>+      isTrusted = PR_TRUE;
>+

Would you explain what means (XRE_GetProcessType() == GeckoProcessType_Default)?

> void
> nsIMEStateManager::OnInstalledMenuKeyboardListener(PRBool aInstalling)
> {
>   sInstalledMenuKeyboardListener = aInstalling;
>-  OnChangeFocus(sPresContext, sContent);
>+  OnChangeFocus(sPresContext, sContent, nsnull);
> }

OnInstalledMenuKeyboardListener() is called when pseudo-focus is set to menu bar or popup menu.
If aInstalling is TRUE, the pesudo-focus is moved to menu, otherwise, the focus is back from menu.

>@@ -231,17 +241,17 @@ nsIMEStateManager::UpdateIMEState(PRUint
>   if (context.mStatus ==
>         nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {
>     return;
>   }
> 
>   // commit current composition
>   widget->ResetInputState();
> 
>-  SetIMEState(aNewIMEState, aContent, widget);
>+  SetIMEState(aNewIMEState, aContent, widget, PR_TRUE);
> }

UpdateIMEState() is called when focus isn't moved but IME state might be changed (enabled, disabled or password).

>diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp
>--- a/dom/base/nsFocusManager.cpp
>+++ b/dom/base/nsFocusManager.cpp
>@@ -936,17 +936,17 @@ nsFocusManager::WindowHidden(nsIDOMWindo
>   }
> 
>   nsCOMPtr<nsIDocShell> focusedDocShell = mFocusedWindow->GetDocShell();
>   nsCOMPtr<nsIPresShell> presShell;
>   focusedDocShell->GetPresShell(getter_AddRefs(presShell));
> 
>   nsIMEStateManager::OnTextStateBlur(nsnull, nsnull);
>   if (presShell) {
>-    nsIMEStateManager::OnChangeFocus(presShell->GetPresContext(), nsnull);
>+    nsIMEStateManager::OnChangeFocus(presShell->GetPresContext(), nsnull, nsnull);

Why you used nsnull rather than 0? It's not pointer.

>@@ -1477,17 +1477,17 @@ nsFocusManager::Blur(nsPIDOMWindow* aWin
>     clearFirstBlurEvent = PR_TRUE;
>   }
> 
>   // if there is still an active window, adjust the IME state.
>   // This has to happen before the focus is cleared below, otherwise, the IME
>   // compositionend event won't get fired at the element being blurred.
>   nsIMEStateManager::OnTextStateBlur(nsnull, nsnull);
>   if (mActiveWindow)
>-    nsIMEStateManager::OnChangeFocus(presShell->GetPresContext(), nsnull);
>+    nsIMEStateManager::OnChangeFocus(presShell->GetPresContext(), nsnull, nsnull);

Same.

>@@ -1720,33 +1720,33 @@ nsFocusManager::Focus(nsPIDOMWindow* aWi

>-      nsIMEStateManager::OnChangeFocus(presContext, nsnull);
>+      nsIMEStateManager::OnChangeFocus(presContext, nsnull, nsnull);

Same.

>@@ -1759,17 +1759,17 @@ nsFocusManager::Focus(nsPIDOMWindow* aWi
>         vm->GetRootWidget(getter_AddRefs(widget));
>         if (widget)
>           widget->SetFocus(PR_FALSE);
>       }
>     }
> 
>     nsPresContext* presContext = presShell->GetPresContext();
>     nsIMEStateManager::OnTextStateBlur(presContext, nsnull);
>-    nsIMEStateManager::OnChangeFocus(presContext, nsnull);
>+    nsIMEStateManager::OnChangeFocus(presContext, nsnull, nsnull);

Same.

>diff --git a/widget/src/android/nsWindow.cpp b/widget/src/android/nsWindow.cpp
>--- a/widget/src/android/nsWindow.cpp
>+++ b/widget/src/android/nsWindow.cpp
>@@ -49,16 +49,17 @@ using mozilla::dom::ContentParent;
> using mozilla::dom::ContentChild;
> using mozilla::unused;
> #endif
> 
> #include "nsAppShell.h"
> #include "nsIdleService.h"
> #include "nsWindow.h"
> #include "nsIObserverService.h"
>+#include "nsContentUtils.h"

I think that we shouldn't use nsContentUtils.h in widget. It hasn't been used in widget yet. And it depends on very many modules. I think that mozilla::widget::WidgetUtils should have same function. It's in widget/src/shared.

>@@ -1744,19 +1745,27 @@ nsWindow::ResetInputState()
> 
>     AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_RESETINPUTSTATE, 0);
>     return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsWindow::SetInputMode(const IMEContext& aContext)
> {
>-    ALOGIME("IME: SetInputMode: s=%d", aContext.mStatus);
>+    ALOGIME("IME: SetInputMode: s=%d trusted=%d", aContext.mStatus, aContext.mTrusted);
>+
>+    // Ensure that opening the virtual keyboard is allowed for this specific
>+    // IMEContext depending on the content.ime.strict.policy pref
>+    if (aContext.mStatus != nsIWidget::IME_STATUS_DISABLED &&
>+        !aContext.mTrusted &&
>+        nsContentUtils::GetBoolPref("content.ime.strict_policy"))
>+      return NS_OK;
> 
>     mIMEContext = aContext;
>+
>     AndroidBridge::NotifyIMEEnabled(int(aContext.mStatus), aContext.mHTMLInputType, aContext.mActionHint);
>     return NS_OK;
> }

It seems that if focus is moved to plugin, VKB is opened automatically, isn't it? I don't think it's expected behavior by users. E.g., the plugin content may not have editor. If plugins can open VKB itself, we shouldn't care it.
Comment on attachment 523313 [details] [diff] [review]
Patch v0.2 (m-c)

Removing r? until I have updated the patch.
Attachment #523313 - Flags: review?(masayuki)
Attached patch Patch v0.3 (obsolete) — Splinter Review
(In reply to comment #25)
> Comment on attachment 523313 [details] [diff] [review]
> Patch v0.2 (m-c)
> 
> 
> I think that "mTrusted" isn't good name for me because IMEStateManager's . I
> think that you want to prevent to open VKB when the focus is changed by user
> actually. Not when the focus is moved by untrusted reason. 

I really want to prevent open VKB when the focus is _not_ changed by users. The goal is to prevent autofocus, or script to open/close the VKB.


> >diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
> >   if (newState != nsIContent::IME_STATUS_NONE) {
> >+    // Request to affect the IME from the content process not marked
> >+    // started by the user are considered as untrusted
> >+    PRBool isTrusted = PR_FALSE;
> >+    if ((aReasons & nsIFocusManager::FLAG_BYMOUSE) ||
> >+        (aReasons & nsIFocusManager::FLAG_BYKEY) ||
> >+        (XRE_GetProcessType() == GeckoProcessType_Default))
> >+      isTrusted = PR_TRUE;
> >+
> 
> Would you explain what means (XRE_GetProcessType() ==
> GeckoProcessType_Default)?

This is to check if the focus change come from the chrome or content process, I only want to prevent focus changes from the content side to open the VKB. I've now added a flag to the enum you proposed.

> >diff --git a/widget/src/android/nsWindow.cpp b/widget/src/android/nsWindow.cpp
> >--- a/widget/src/android/nsWindow.cpp
> >+++ b/widget/src/android/nsWindow.cpp
> >@@ -49,16 +49,17 @@ using mozilla::dom::ContentParent;
> > using mozilla::dom::ContentChild;
> > using mozilla::unused;
> > #endif
> > 
> > #include "nsAppShell.h"
> > #include "nsIdleService.h"
> > #include "nsWindow.h"
> > #include "nsIObserverService.h"
> >+#include "nsContentUtils.h"
> 
> I think that we shouldn't use nsContentUtils.h in widget. It hasn't been used
> in widget yet. And it depends on very many modules. I think that
> mozilla::widget::WidgetUtils should have same function. It's in
> widget/src/shared.

I've removed the nsContentUtils.h dependency and manually call the pref service instead. I can open a followup to add getBoolPref/getIntPref/... to WidgetUtils if needed?


> >+    // Ensure that opening the virtual keyboard is allowed for this specific
> >+    // IMEContext depending on the content.ime.strict.policy pref
> >+    if (aContext.mStatus != nsIWidget::IME_STATUS_DISABLED &&
> >+        !aContext.mTrusted &&
> >+        nsContentUtils::GetBoolPref("content.ime.strict_policy"))
> >+      return NS_OK;
> > 
> >     mIMEContext = aContext;
> >+
> >     AndroidBridge::NotifyIMEEnabled(int(aContext.mStatus), aContext.mHTMLInputType, aContext.mActionHint);
> >     return NS_OK;
> > }
> 
> It seems that if focus is moved to plugin, VKB is opened automatically, isn't
> it? I don't think it's expected behavior by users. E.g., the plugin content may
> not have editor. If plugins can open VKB itself, we shouldn't care it.

If I understand correctly the code it sounds like you're right, do you mean we need to be more selective here: http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoAppShell.java#477 and do not open the VKB for plugins?
Attachment #523313 - Attachment is obsolete: true
Attachment #525380 - Flags: review?(masayuki)
(In reply to comment #27)
> Created attachment 525380 [details] [diff] [review]
> Patch v0.3
> 
> (In reply to comment #25)
> > Comment on attachment 523313 [details] [diff] [review]
> > Patch v0.2 (m-c)
> > 
> > 
> > I think that "mTrusted" isn't good name for me because IMEStateManager's . I
> > think that you want to prevent to open VKB when the focus is changed by user
> > actually. Not when the focus is moved by untrusted reason. 
> 
> I really want to prevent open VKB when the focus is _not_ changed by users. The
> goal is to prevent autofocus, or script to open/close the VKB.

Oh. It was just my typo. I agree with you.

> > >diff --git a/widget/src/android/nsWindow.cpp b/widget/src/android/nsWindow.cpp
> > >--- a/widget/src/android/nsWindow.cpp
> > >+++ b/widget/src/android/nsWindow.cpp
> > >@@ -49,16 +49,17 @@ using mozilla::dom::ContentParent;
> > > using mozilla::dom::ContentChild;
> > > using mozilla::unused;
> > > #endif
> > > 
> > > #include "nsAppShell.h"
> > > #include "nsIdleService.h"
> > > #include "nsWindow.h"
> > > #include "nsIObserverService.h"
> > >+#include "nsContentUtils.h"
> > 
> > I think that we shouldn't use nsContentUtils.h in widget. It hasn't been used
> > in widget yet. And it depends on very many modules. I think that
> > mozilla::widget::WidgetUtils should have same function. It's in
> > widget/src/shared.
> 
> I've removed the nsContentUtils.h dependency and manually call the pref service
> instead. I can open a followup to add getBoolPref/getIntPref/... to WidgetUtils
> if needed?

Yeah, but you don't need to work for it if you're busy.

> > >+    // Ensure that opening the virtual keyboard is allowed for this specific
> > >+    // IMEContext depending on the content.ime.strict.policy pref
> > >+    if (aContext.mStatus != nsIWidget::IME_STATUS_DISABLED &&
> > >+        !aContext.mTrusted &&
> > >+        nsContentUtils::GetBoolPref("content.ime.strict_policy"))
> > >+      return NS_OK;
> > > 
> > >     mIMEContext = aContext;
> > >+
> > >     AndroidBridge::NotifyIMEEnabled(int(aContext.mStatus), aContext.mHTMLInputType, aContext.mActionHint);
> > >     return NS_OK;
> > > }
> > 
> > It seems that if focus is moved to plugin, VKB is opened automatically, isn't
> > it? I don't think it's expected behavior by users. E.g., the plugin content may
> > not have editor. If plugins can open VKB itself, we shouldn't care it.
> 
> If I understand correctly the code it sounds like you're right, do you mean we
> need to be more selective here:
> http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoAppShell.java#477
> and do not open the VKB for plugins?

I think so. Plugins should open VKB for their users by themselves. We shouldn't do anything.
Comment on attachment 525380 [details] [diff] [review]
Patch v0.3

>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>@@ -185,31 +187,37 @@ nsIMEStateManager::OnChangeFocus(nsPresC
>       oldWidget = widget;
>     else
>       oldWidget = GetWidget(sPresContext);
>     if (oldWidget)
>       oldWidget->ResetInputState();
>   }
> 
>   if (newState != nsIContent::IME_STATUS_NONE) {
>+    // Focus changes coming from the content process may not open the VKB
>+    PRUint32 reason = aReason;
>+    if (XRE_GetProcessType() == GeckoProcessType_Content)
>+      reason &= FOCUS_FROM_CONTENT_PROCESS;
>+

Please add {} for the if statement even if it has only one line.

>@@ -1720,33 +1720,40 @@ nsFocusManager::Focus(nsPIDOMWindow* aWi
>       // if this is an object/plug-in, focus the plugin's widget.  Note that we might
>       // no longer be in the same document, due to the events we fired above when
>       // aIsNewDocument.
>       if (aAdjustWidgets && presShell->GetDocument() == aContent->GetDocument()) {
>         if (objectFrameWidget)
>           objectFrameWidget->SetFocus(PR_FALSE);
>       }
> 
>-      nsIMEStateManager::OnChangeFocus(presContext, aContent);
>+      PRUint32 reason = FOCUS_MOVED_UNKNOWN;
>+      if (aFlags & nsIFocusManager::FLAG_BYMOUSE)
>+        reason = FOCUS_MOVED_BY_MOUSE;
>+      else if (aFlags & nsIFocusManager::FLAG_BYKEY)
>+        reason = FOCUS_MOVED_BY_KEY;
>+      else if (aFlags & nsIFocusManager::FLAG_BYMOVEFOCUS)
>+        reason = FOCUS_MOVED_BY_MOVEFOCUS;
>+      nsIMEStateManager::OnChangeFocus(presContext, aContent, reason);

Please add {} for every if (or else if).

And I guess that this should be in static method like nsFocusManager::GetFocusMoveReason(PRUint32 aFlags).

>diff --git a/widget/public/nsIWidget.h b/widget/public/nsIWidget.h
> /* 
>  * Contains IMEStatus plus information about the current 
>  * input context that the IME can use as hints if desired.
>  */
>+enum {
>+  FOCUS_REMOVED       = 0x0001,
>+  FOCUS_MOVED_UNKNOWN = 0x0002,
>+  FOCUS_MOVED_BY_MOVEFOCUS = 0x0004,
>+  FOCUS_MOVED_BY_MOUSE = 0x0008,
>+  FOCUS_MOVED_BY_KEY = 0x0010,
>+  FOCUS_MOVED_TO_MENU = 0x0020,
>+  FOCUS_MOVED_FROM_MENU = 0x0040,
>+  EDITOR_STATE_MODIFIED = 0x0080,
>+  FOCUS_FROM_CONTENT_PROCESS = 0x0100
>+};
>+

I think that this enum should be defined in IMEContext because here is global namespace.

>@@ -1744,19 +1745,36 @@ nsWindow::ResetInputState()
> 
>     AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_RESETINPUTSTATE, 0);
>     return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsWindow::SetInputMode(const IMEContext& aContext)
> {
>-    ALOGIME("IME: SetInputMode: s=%d", aContext.mStatus);
>+    ALOGIME("IME: SetInputMode: s=%d trusted=%d", aContext.mStatus, aContext.mReason);
>+
>+    // Ensure that opening the virtual keyboard is allowed for this specific
>+    // IMEContext depending on the content.ime.strict.policy pref
>+    if (aContext.mStatus != nsIWidget::IME_STATUS_DISABLED) {
>+      nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+
>+      PRBool useStrictPolicy = PR_FALSE;
>+      nsresult rv = prefs->GetBoolPref("content.ime.strict_policy", &useStrictPolicy);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      PRBool isUserFocus = ((aContext.mReason & FOCUS_MOVED_BY_MOUSE) ||
>+                            (aContext.mReason & FOCUS_MOVED_BY_KEY));
>+      if (useStrictPolicy && 
>+          isUserFocus && (aContext.mReason & FOCUS_FROM_CONTENT_PROCESS))
>+        return NS_OK;
>+    }
> 
>     mIMEContext = aContext;
>+
>     AndroidBridge::NotifyIMEEnabled(int(aContext.mStatus), aContext.mHTMLInputType, aContext.mActionHint);
>     return NS_OK;
> }

So, I think |mIMEContext = aContext;| should be done always. And at |if (aContext.mStatus == nsIWidget::IME_STATUS_PLUGIN)|, you don't need to check the pref and reason.

And please add {} for every new if statement.

>diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp
>--- a/widget/src/gtk2/nsWindow.cpp
>+++ b/widget/src/gtk2/nsWindow.cpp
>@@ -6401,16 +6401,33 @@ NS_IMETHODIMP
> nsWindow::ResetInputState()
> {
>     return mIMModule ? mIMModule->ResetInputState(this) : NS_OK;
> }
> 
> NS_IMETHODIMP
> nsWindow::SetInputMode(const IMEContext& aContext)
> {
>+#ifdef MOZ_PLATFORM_MAEMO
>+    // Ensure that opening the virtual keyboard is allowed for this specific
>+    // IMEContext depending on the content.ime.strict.policy pref
>+    if (aContext.mStatus != nsIWidget::IME_STATUS_DISABLED) {
>+      nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+
>+      PRBool useStrictPolicy = PR_FALSE;
>+      nsresult rv = prefs->GetBoolPref("content.ime.strict_policy", &useStrictPolicy);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      PRBool isUserFocus = ((aContext.mReason & FOCUS_MOVED_BY_MOUSE) ||
>+                            (aContext.mReason & FOCUS_MOVED_BY_KEY));
>+      if (useStrictPolicy && 
>+          isUserFocus && (aContext.mReason & FOCUS_FROM_CONTENT_PROCESS))
>+        return NS_OK;
>+    }
>+#endif
>     return mIMModule ? mIMModule->SetInputMode(this, &aContext) : NS_OK;
> }

This is wrong, you should do it in nsGtkIMModule::SetInputMode().

I think that you should remove |aContext->mStatus == mIMEContext.mStatus| check in nsGtkIMModule::SetInputMode() because the reason might be different.

And you should add the code is in IsEnabled() block which is in |#if (MOZ_PLATFORM_MAEMO == 5)|.
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsGtkIMModule.cpp#590

Please don't break setting HILDON_GTK_INPUT_MODE_INVISIBLE case.
Attached patch Patch v0.4 (obsolete) — Splinter Review
(In reply to comment #29)>  
> So, I think |mIMEContext = aContext;| should be done always.

I have done that in order to know if the VKB has been closed by the user or not and to cycle the focus in order to re-show the VKB if needed. I have added some code to SetFocusInner to cycle the IME state internally if the element is still the same but I'm not sure if that the right things to do or if it's not breaking some security rules?
Attachment #525380 - Attachment is obsolete: true
Attachment #526024 - Flags: review?(masayuki)
Attachment #525380 - Flags: review?(masayuki)
Attached patch mobile patchSplinter Review
Updated on trunk and removing the need to cycle focus.
Attachment #523311 - Attachment is obsolete: true
Attachment #526026 - Flags: review?(mark.finkle)
Attached patch Patch v0.4 (obsolete) — Splinter Review
Right patch this time for the platform's focus/ime part!
Attachment #526024 - Attachment is obsolete: true
Attachment #526027 - Flags: review?(masayuki)
Attachment #526024 - Flags: review?(masayuki)
Comment on attachment 526026 [details] [diff] [review]
mobile patch

Make sure browser-chrome tests stay "green"
Attachment #526026 - Flags: review?(mark.finkle) → review+
Attached patch Patch v0.4 (obsolete) — Splinter Review
Fix an issue while the element is the same where the virtual keyboard was opening/closing/reopening each time the focus was set on it.
Attachment #526027 - Attachment is obsolete: true
Attachment #526042 - Flags: review?(masayuki)
Attachment #526027 - Flags: review?(masayuki)
Comment on attachment 526042 [details] [diff] [review]
Patch v0.4

>diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp
>@@ -1065,21 +1098,43 @@ nsFocusManager::SetFocusInner(nsIContent
>     // caret position isn't updated.
>     aFocusChanged = PR_FALSE;
>   }
> 
>   // unless it was set above, retrieve the window for the element to focus
>   if (!newWindow)
>     newWindow = GetCurrentWindow(contentToFocus);
> 
>-  // if the element is already focused, just return. Note that this happens
>-  // after the frame check above so that we compare the element that will be
>-  // focused rather than the frame it is in.
>-  if (!newWindow || (newWindow == mFocusedWindow && contentToFocus == mFocusedContent))
>+  // if the element is already focused, just return but update the IME state 
>+  // if needed. Note that this happens after the frame check above so that we
>+  // compare the element that will be focused rather than the frame it is in.
>+  if (!newWindow || (newWindow == mFocusedWindow && contentToFocus == mFocusedContent)) {
>+    nsCOMPtr<nsIDocShell> docShell = newWindow->GetDocShell();
>+
>+    // If the IMEContext is still enabled there is no need to cycle the IME
>+    // state since probably nothing has changed
>+    nsCOMPtr<nsIWidget> widget = GetWidget(docShell);
>+    IMEContext context;
>+    nsresult rv = widget->GetInputMode(context);
>+    if (NS_SUCCEEDED(rv) && context.mStatus == nsIWidget::IME_STATUS_ENABLED)
>+      return;
>+
>+    // Even if the element was the previously focused element, the IME state
>+    // could have changed, if virtual keyboard has been closed for example. Also
>+    // the reason of the focus can be different is the focus has first been by
>+    // a script and the second initiated by a mouse or a keyboard event.
>+    nsCOMPtr<nsIPresShell> presShell;
>+    docShell->GetPresShell(getter_AddRefs(presShell));
>+    if (presShell) {
>+      PRUint32 reason = GetFocusMoveReason(aFlags);
>+      nsIMEStateManager::OnChangeFocus(presShell->GetPresContext(), mFocusedContent, reason);
>+    }
>+
>     return;
>+  }

Is this for bug 507987? If so, please remove this from this patch.

>+      PRBool userFocus = (aContext.mReason & IMEContext::FOCUS_MOVED_BY_MOUSE) ||
>+                          (aContext.mReason & IMEContext::FOCUS_MOVED_BY_KEY);

How about to create FocusMovedByUser() or something on IMEContext? Then, you can use it on nsGtkIMModule.cpp too.

>@@ -583,16 +584,36 @@ nsGtkIMModule::SetInputMode(nsWindow* aC
>     // Be aware, don't use aWindow here because this method shouldn't move
>     // focus actually.
>     Focus();
> 
> #if (MOZ_PLATFORM_MAEMO == 5)
>     GtkIMContext *im = GetContext();
>     if (im) {
>         if (IsEnabled()) {
>+            // Ensure that opening the virtual keyboard is allowed for this specific
>+            // IMEContext depending on the content.ime.strict.policy pref
>+            if (mIMEContext.mStatus != nsIWidget::IME_STATUS_DISABLED && 
>+                mIMEContext.mStatus != nsIWidget::IME_STATUS_PLUGIN) {
>+
>+                nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+
>+                PRBool useStrictPolicy = PR_FALSE;
>+                nsresult rv = prefs->GetBoolPref("content.ime.strict_policy", &useStrictPolicy);
>+                NS_ENSURE_SUCCESS(rv, rv);
>+
>+                PRBool userFocus = (mIMEContext.mReason & IMEContext::FOCUS_MOVED_BY_MOUSE) ||
>+                                   (mIMEContext.mReason & IMEContext::FOCUS_MOVED_BY_KEY);
>+
>+                if (useStrictPolicy && !userFocus && 
>+                    (mIMEContext.mReason & IMEContext::FOCUS_FROM_CONTENT_PROCESS)) {
>+                    return NS_OK;

Hmm... I guess that if focus is moved by script when VKB is opened, shouldn't we close the opened VKB rather than keep VKB opened? If we don't do so, doesn't the user realize to focus move? I.e., user will enter the text to unexpected text field, no?

Or is VKB closed by blur event always?
Attached patch Patch v0.5 (obsolete) — Splinter Review
(In reply to comment #35)
> 
> Is this for bug 507987? If so, please remove this from this patch.

Basically yes. 

I've removed the code but since I still need a way to know if the VKB is opened or not I have add an API to nsIDOMWindowUtils.idl to add a way to know that from the outside world (nsIDOMWindowUtils.isVirtualKeyboardOpened)

> Hmm... I guess that if focus is moved by script when VKB is opened, shouldn't
> we close the opened VKB rather than keep VKB opened? If we don't do so, doesn't
> the user realize to focus move? I.e., user will enter the text to unexpected
> text field, no?

> Or is VKB closed by blur event always?

I think so and I assume the IME state will be updated in such a case right?
Attachment #526042 - Attachment is obsolete: true
Attachment #526284 - Flags: review?(masayuki)
Attachment #526042 - Flags: review?(masayuki)
(In reply to comment #36)
> I've removed the code but since I still need a way to know if the VKB is opened
> or not I have add an API to nsIDOMWindowUtils.idl to add a way to know that
> from the outside world (nsIDOMWindowUtils.isVirtualKeyboardOpened)

Why do you need the new API for this bug? Isn't it for another bug? I don't find the caller in your patch.

> > Hmm... I guess that if focus is moved by script when VKB is opened, shouldn't
> > we close the opened VKB rather than keep VKB opened? If we don't do so, doesn't
> > the user realize to focus move? I.e., user will enter the text to unexpected
> > text field, no?
> 
> > Or is VKB closed by blur event always?
> 
> I think so and I assume the IME state will be updated in such a case right?

Right. Do you find such cases? I think that even if there are such cases, the causative class (maybe, element or editor) rather than nsFocusManager should notify it to nsIMEStateManager.
(In reply to comment #37)
> (In reply to comment #36)
> > I've removed the code but since I still need a way to know if the VKB is opened
> > or not I have add an API to nsIDOMWindowUtils.idl to add a way to know that
> > from the outside world (nsIDOMWindowUtils.isVirtualKeyboardOpened)
> 
> Why do you need the new API for this bug? Isn't it for another bug? I don't
> find the caller in your patch.

I need it for two purposes:
 1. to be able to workaround bug 507987 in Fennec front-end side which I guess request something more complicate
 2. To do tests to ensure the VKB is opened - that I will attached with an updated version of the mobile front-end patch

> 
> > > Hmm... I guess that if focus is moved by script when VKB is opened, shouldn't
> > > we close the opened VKB rather than keep VKB opened? If we don't do so, doesn't
> > > the user realize to focus move? I.e., user will enter the text to unexpected
> > > text field, no?
> > 
> > > Or is VKB closed by blur event always?
> > 
> > I think so and I assume the IME state will be updated in such a case right?
> 
> Right. Do you find such cases? I think that even if there are such cases, the
> causative class (maybe, element or editor) rather than nsFocusManager should
> notify it to nsIMEStateManager.

I will be able to add such a case in a test and see what happen.
(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > I've removed the code but since I still need a way to know if the VKB is opened
> > > or not I have add an API to nsIDOMWindowUtils.idl to add a way to know that
> > > from the outside world (nsIDOMWindowUtils.isVirtualKeyboardOpened)
> > 
> > Why do you need the new API for this bug? Isn't it for another bug? I don't
> > find the caller in your patch.
> 
> I need it for two purposes:
>  1. to be able to workaround bug 507987 in Fennec front-end side which I guess
> request something more complicate
>  2. To do tests to ensure the VKB is opened - that I will attached with an
> updated version of the mobile front-end patch

I still think that this is another bug. We should separate the fix on another bug. We should make simple patches and simple hg logs. If we fix two or more bugs by one patch, it becomes hard for other developers to understand the change reason.

> > > > Hmm... I guess that if focus is moved by script when VKB is opened, shouldn't
> > > > we close the opened VKB rather than keep VKB opened? If we don't do so, doesn't
> > > > the user realize to focus move? I.e., user will enter the text to unexpected
> > > > text field, no?
> > > 
> > > > Or is VKB closed by blur event always?
> > > 
> > > I think so and I assume the IME state will be updated in such a case right?
> > 
> > Right. Do you find such cases? I think that even if there are such cases, the
> > causative class (maybe, element or editor) rather than nsFocusManager should
> > notify it to nsIMEStateManager.
> 
> I will be able to add such a case in a test and see what happen.

Please don't add it to the patch. Let's focus on fixing the simple cases first. I'd prefer we finish attachment 526024 [details] [diff] [review] first.
Attached patch Patch v0.6Splinter Review
Ok, I've removed all the code relative to the opening/closing state of the keyboard.
Attachment #526284 - Attachment is obsolete: true
Attachment #527046 - Flags: review?(masayuki)
Attachment #526284 - Flags: review?(masayuki)
Comment on attachment 527046 [details] [diff] [review]
Patch v0.6

Thank you!
Attachment #527046 - Flags: review?(masayuki) → review+
(In reply to comment #41)
> Comment on attachment 527046 [details] [diff] [review]
> Patch v0.6
> 
> Thank you!

Thanks for your dailies reviews :)

Mark, I will push the front-end patch in bug 651116 once we have a way to know when the keyboard is opened.
http://hg.mozilla.org/mozilla-central/rev/aed1a67f1738
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
tracking-fennec: --- → ?
Whiteboard: [VKB] [fennec-4.1?] → [VKB] [fennec-4.1?][sg:want]
Will not track for 5 because its not a regression from 4.  Looks important though.  I can be approval-aurora? it.  Please outline the risk to the desktop product if you request approval.
Tested: 
Bookmarked link, undo tab, All pages, History, directly going there, long tap link

Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Thunderbolt
OS: Android 2.2
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 6
tracking-fennec: ? → 6+
Depends on: 680496
Blocks: 680496
No longer depends on: 680496
Depends on: 669995
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: