Last Comment Bug 532738 - Do not open the virtual keyboard on untrusted focus (caused by content page scripts)
: Do not open the virtual keyboard on untrusted focus (caused by content page s...
Status: VERIFIED FIXED
[VKB] [fennec-4.1?][sg:want]
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 6
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
:
Mentors:
: 586618 638778 (view as bug list)
Depends on: 588313 669995
Blocks: 623624 680496
  Show dependency treegraph
 
Reported: 2009-12-03 13:16 PST by Madhava Enros [:madhava]
Modified: 2011-08-22 06:58 PDT (History)
19 users (show)
mounir: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.44 KB, patch)
2009-12-16 02:02 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch (m-c) (1.48 KB, patch)
2011-03-24 18:34 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch (m-b) (4.19 KB, patch)
2011-03-24 18:36 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.2 (m-c) (18.52 KB, patch)
2011-03-31 08:20 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch (m-b) - updated on trunk (4.28 KB, patch)
2011-03-31 08:21 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
mark.finkle: review+
Details | Diff | Splinter Review
Patch v0.2 (m-c) (18.73 KB, patch)
2011-03-31 08:32 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.3 (19.76 KB, patch)
2011-04-12 07:51 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.4 (23.69 KB, patch)
2011-04-14 09:37 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
mobile patch (3.98 KB, patch)
2011-04-14 09:39 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
mark.finkle: review+
Details | Diff | Splinter Review
Patch v0.4 (24.78 KB, patch)
2011-04-14 09:40 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.4 (26.33 KB, patch)
2011-04-14 10:19 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.5 (38.53 KB, patch)
2011-04-15 09:40 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.6 (14.15 KB, patch)
2011-04-19 11:04 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
masayuki: review+
Details | Diff | Splinter Review

Description Madhava Enros [:madhava] 2009-12-03 13:16:15 PST
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.
Comment 1 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2009-12-03 19:31:09 PST
I have a hack for that but it relies on playing with the isOffScreenBrowser property which i'm a bit worried about.
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2009-12-16 02:02:01 PST
Created attachment 417893 [details] [diff] [review]
Patch

I'm not very convinced that we should land that just before RC, i'm not sure of the side impact.
Comment 3 Juha Seppänen 2010-03-23 09:59:57 PDT
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?
Comment 4 Tony Chung [:tchung] 2010-05-03 10:00:41 PDT
any decision on how we want to proceed with this fix/feature?
Comment 5 Oleg Romashin (:romaxa) 2010-06-23 23:09:17 PDT
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
Comment 6 Doug Turner (:dougt) 2010-06-30 12:17:36 PDT
[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)
Comment 7 Juha Seppänen 2010-07-24 13:13:03 PDT
What's happening here. Shall we change this to what is discussed in comment #6 or how do we do it?
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2010-08-16 23:00:16 PDT
*** Bug 586618 has been marked as a duplicate of this bug. ***
Comment 9 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-02 15:31:07 PDT
Comment on attachment 417893 [details] [diff] [review]
Patch

My patch is not the way to go. Removing r?.
Comment 10 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-01-06 09:38:25 PST
I just filed bug 623624, which is sort of in contradiction with the summary of this bug.
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-02-17 07:06:57 PST
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.
Comment 12 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-04 08:19:41 PST
*** Bug 638778 has been marked as a duplicate of this bug. ***
Comment 13 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-24 18:34:51 PDT
Created attachment 521703 [details] [diff] [review]
Patch (m-c)

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
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-24 18:36:16 PDT
Created attachment 521704 [details] [diff] [review]
Patch (m-b)

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...
Comment 15 Mounir Lamouri (:mounir) 2011-03-25 03:52:27 PDT
I guess it shouldn't be that hard to test, right?
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-25 04:31:43 PDT
(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.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-25 09:42:00 PDT
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 18 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-25 09:57:25 PDT
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.
Comment 20 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-31 08:20:20 PDT
Created attachment 523310 [details] [diff] [review]
Patch v0.2 (m-c)
Comment 21 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-31 08:21:04 PDT
Created attachment 523311 [details] [diff] [review]
Patch (m-b) - updated on trunk
Comment 22 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-31 08:32:53 PDT
Created attachment 523313 [details] [diff] [review]
Patch v0.2 (m-c)

Oups, I've forgot to update the MAEMO code path
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-31 11:38:09 PDT
Comment on attachment 523311 [details] [diff] [review]
Patch (m-b) - updated on trunk

Do we have any tests for this?
Comment 24 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-31 15:40:27 PDT
(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 25 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-31 19:26:46 PDT
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 26 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-11 08:16:39 PDT
Comment on attachment 523313 [details] [diff] [review]
Patch v0.2 (m-c)

Removing r? until I have updated the patch.
Comment 27 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-12 07:51:21 PDT
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.


> >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?
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-12 14:35:37 PDT
(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 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-12 15:20:47 PDT
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.
Comment 30 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-14 09:37:42 PDT
Created attachment 526024 [details] [diff] [review]
Patch v0.4

(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?
Comment 31 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-14 09:39:30 PDT
Created attachment 526026 [details] [diff] [review]
mobile patch

Updated on trunk and removing the need to cycle focus.
Comment 32 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-14 09:40:25 PDT
Created attachment 526027 [details] [diff] [review]
Patch v0.4

Right patch this time for the platform's focus/ime part!
Comment 33 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-14 09:41:05 PDT
Comment on attachment 526026 [details] [diff] [review]
mobile patch

Make sure browser-chrome tests stay "green"
Comment 34 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-14 10:19:01 PDT
Created attachment 526042 [details] [diff] [review]
Patch v0.4

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.
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-14 21:00:56 PDT
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?
Comment 36 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-15 09:40:46 PDT
Created attachment 526284 [details] [diff] [review]
Patch v0.5

(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?
Comment 37 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-15 14:31:44 PDT
(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.
Comment 38 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-18 06:03:18 PDT
(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.
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-18 19:16:07 PDT
(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.
Comment 40 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-19 11:04:10 PDT
Created attachment 527046 [details] [diff] [review]
Patch v0.6

Ok, I've removed all the code relative to the opening/closing state of the keyboard.
Comment 41 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-19 15:55:15 PDT
Comment on attachment 527046 [details] [diff] [review]
Patch v0.6

Thank you!
Comment 42 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-20 05:50:06 PDT
(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.
Comment 43 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-20 05:51:42 PDT
http://hg.mozilla.org/mozilla-central/rev/aed1a67f1738
Comment 44 JP Rosevear [:jpr] 2011-05-04 11:44:54 PDT
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.
Comment 45 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-05-06 10:45:33 PDT
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

Note You need to log in before you can comment on or make changes to this bug.