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)
Firefox for Android Graveyard
General
Tracking
(firefox5- affected, firefox6 fixed, fennec6+)
VERIFIED
FIXED
Firefox 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.
Reporter | ||
Updated•15 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 1•15 years ago
|
||
I have a hack for that but it relies on playing with the isOffScreenBrowser property which i'm a bit worried about.
Assignee | ||
Comment 2•15 years ago
|
||
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)
Updated•14 years ago
|
tracking-fennec: ? → 1.0-
Comment 3•14 years ago
|
||
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•14 years ago
|
||
any decision on how we want to proceed with this fix/feature?
Comment 5•14 years ago
|
||
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•14 years ago
|
||
[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•14 years ago
|
||
What's happening here. Shall we change this to what is discussed in comment #6 or how do we do it?
Assignee | ||
Comment 9•14 years ago
|
||
Comment on attachment 417893 [details] [diff] [review] Patch My patch is not the way to go. Removing r?.
Attachment #417893 -
Flags: review?(mark.finkle)
Updated•14 years ago
|
Summary: Don't auto-focus input fields on page load → [VKB]Don't auto-focus input fields on page load
Updated•14 years ago
|
Summary: [VKB]Don't auto-focus input fields on page load → Don't auto-focus input fields on page load
Whiteboard: [VKB]
Reporter | ||
Updated•14 years ago
|
tracking-fennec: 1.0- → ?
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
tracking-fennec: ? → ---
Updated•14 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
Updated•14 years ago
|
blocking2.0: ? → ---
tracking-fennec: --- → ?
Updated•14 years ago
|
tracking-fennec: ? → 2.0-
Comment 10•13 years ago
|
||
I just filed bug 623624, which is sort of in contradiction with the summary of this bug.
Updated•13 years ago
|
Priority: -- → P3
Comment 11•13 years ago
|
||
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)
Updated•13 years ago
|
tracking-fennec: 2.0- → ---
Whiteboard: [VKB] → [VKB] [fennec-4.1?]
Assignee | ||
Comment 13•13 years ago
|
||
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
Assignee | ||
Comment 14•13 years ago
|
||
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...
Assignee | ||
Updated•13 years ago
|
Attachment #521703 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•13 years ago
|
Attachment #521704 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 16•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #521703 -
Flags: review?(masayuki)
Comment 17•13 years ago
|
||
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.
Assignee | ||
Comment 18•13 years ago
|
||
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)
Comment 19•13 years ago
|
||
Just as a note, when this bug is fixed, the testcases for bug 623624 should work: https://bugzilla.mozilla.org/attachment.cgi?id=513139 https://bugzilla.mozilla.org/attachment.cgi?id=513141 https://bugzilla.mozilla.org/attachment.cgi?id=513142 https://bugzilla.mozilla.org/attachment.cgi?id=513143
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #521703 -
Attachment is obsolete: true
Attachment #523310 -
Flags: review?(masayuki)
Assignee | ||
Comment 21•13 years ago
|
||
Attachment #521704 -
Attachment is obsolete: true
Attachment #523311 -
Flags: review?(mark.finkle)
Attachment #521704 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 22•13 years ago
|
||
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 23•13 years ago
|
||
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+
Assignee | ||
Comment 24•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 26•13 years ago
|
||
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)
Assignee | ||
Comment 27•13 years ago
|
||
(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)
Comment 28•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 30•13 years ago
|
||
(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)
Assignee | ||
Comment 31•13 years ago
|
||
Updated on trunk and removing the need to cycle focus.
Attachment #523311 -
Attachment is obsolete: true
Attachment #526026 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 32•13 years ago
|
||
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 33•13 years ago
|
||
Comment on attachment 526026 [details] [diff] [review] mobile patch Make sure browser-chrome tests stay "green"
Attachment #526026 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 34•13 years ago
|
||
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 35•13 years ago
|
||
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?
Assignee | ||
Comment 36•13 years ago
|
||
(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)
Comment 37•13 years ago
|
||
(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.
Assignee | ||
Comment 38•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 40•13 years ago
|
||
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 41•13 years ago
|
||
Comment on attachment 527046 [details] [diff] [review] Patch v0.6 Thank you!
Attachment #527046 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 42•13 years ago
|
||
(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.
Assignee | ||
Comment 43•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/aed1a67f1738
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
tracking-fennec: --- → ?
status-firefox5:
--- → affected
tracking-firefox5:
--- → ?
Whiteboard: [VKB] [fennec-4.1?] → [VKB] [fennec-4.1?][sg:want]
Comment 44•13 years ago
|
||
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
Updated•13 years ago
|
Target Milestone: --- → Firefox 6
Updated•13 years ago
|
tracking-fennec: ? → 6+
Updated•13 years ago
|
status-firefox6:
--- → fixed
Updated•13 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•