Closed Bug 1266683 Opened 4 years ago Closed 2 years ago

Send IME_FLAG_NO_PERSONALIZED_LEARNING in private browsing (URL bar and content)

Categories

(Firefox for Android :: Keyboards and IME, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
relnote-firefox --- 57+
firefox57 --- fixed

People

(Reporter: cousteaulecommandant, Assigned: JanH)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 2 obsolete files)

Some Android keyboards have a "learning" feature where they learn the user's frequently used words to improve autocorrection and autocompletion.
When in private browsing, the URL bar (and maybe all other text fields as well) should not learn new keyboard predictions/suggestions/autocorrections.  This way private browsing would also mean "private typing".
This might be possible to do by using android:inputType="textNoSuggestions" which according to the documentation "indicates that the IME should not show any dictionary-based word suggestions" (and I guess disables word learning as well).

This feature might be popular: there is already an entry in SwiftKey's help https://support.swiftkey.com/hc/en-us/articles/202437682-Can-I-turn-off-learning-in-incognito-mode-with-SwiftKey-Keyboard-for-Android- which is answered as "we can't do this".
Firefox for Android asks the keyboard not to show suggestions any time you interact with the URL bar. There are some keyboards such as the Samsung one that seem to ignore this. However if you use the Google Keyboard you will get this behavior. I don't know that we want to put a lot of effort into blocking every keyboard's autocomplete (assuming that we can block the autocomplete in the first place).
My keyboard (SwiftKey) disables suggestions in certain fields (not only in passwords); I don't know which criteria it uses, maybe I should research this.  I thought it might be the textNoSuggestions attribute which caused this.
Autocompletion and autosuggestion in the URL bar is usually good because of its use as a search bar; it's only in private browsing where I think this should be avoided.
See Also: → 1320449
@cousteau

If you install "Hacker's Keyboard" and then open its settings while the keyboard is up, it will tell you the inputType's used for that field.

However in any case, after a bit of searching around, it seems that the standard workaround to disabling keyboard prediction for certain on *all* keyboards is to set the InputType to "textVisiblePassword".
Assignee: nobody → jh+bugzilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Although marking the URL bar as containing passwords in private mode is unfortunately incompatible with TYPE_TEXT_VARIATION_URI, meaning we can either get a special URI entering mode (although not all keyboards support that - the Samsung keyboard on my old S3 Mini had a special URI mode, whereas on my new phone neither the Google keyboard nor Swiftkey don't seem to do anything special there) *or* reliably turn off text prediction everywhere.
Comment on attachment 8814616 [details]
Bug 1266683 - Part 1 - Try harder to prevent awesome bar keyboard text prediction during private browsing.

Alternatively, if we *really* care about avoiding breaking InputType.TYPE_TEXT_VARIATION_URI during private browsing as far as possible, we could attempt to blacklist just affected keyboards via InputMethods.java - although I think I've heard that only e.g. older versions of the Samsung keyboard ignore TYPE_TEXT_FLAG_NO_SUGGESTIONS, and we can't distinguish different IME versions if they're still using the same package and service name.

Also, I don't know which other keyboards apart from Swiftkey we'd have to block in that case...
Attachment #8814616 - Flags: feedback?(s.kaspari)
(In reply to Jan Henning [:JanH] from comment #4)
> whereas on my new phone neither the Google keyboard

I need to correct myself: The Google keyboard does turn on a dedicated "/" key in URI mode - which would be an argument in favour of using blacklisting for affected keyboards only if we want to do this at all.
Duplicate of this bug: 1295018
Okay, judging from bug 1295018 it looks like somewhat newer Samsung phones could be affected as well...

(In reply to ItielMaN from bug 1295018 comment #0)
> Note that in the built-in Android's browser (Samsung Internet for Android,
> version 4.0.20-17, on my phone), when Secret Mode is enabled, the Words
> Autocomplete feature is automatically off, thus the keyboard doesn't learn
> any new words during the Secret Mode session.

As described above, Firefox has been turning off input suggestions from the keyboard for ages by styling the awesome bar as "textNoSuggestions", however some keyboards ignore that. Therefore I'm curious what the Samsung browser is using in that case to turn input suggestions off.

If you install Hacker's Keyboard, enter Secret Mode in the Samsung browser and enter some text into the URL bar, what does it show you if you enter the Hacker Keyboard's settings (that dial button between ?123 and /) and then scroll down to Input connection details?
Flags: needinfo?(itiel_yn8)
(In reply to Jan Henning [:JanH] from comment #9)

> As described above, Firefox has been turning off input suggestions from the
> keyboard for ages by styling the awesome bar as "textNoSuggestions", however
> some keyboards ignore that. Therefore I'm curious what the Samsung browser
> is using in that case to turn input suggestions off.
> 
> If you install Hacker's Keyboard, enter Secret Mode in the Samsung browser
> and enter some text into the URL bar, what does it show you if you enter the
> Hacker Keyboard's settings (that dial button between ?123 and /) and then
> scroll down to Input connection details?

com.sec.android.app.sbrowser type=TEXT.URI|NO_SUGGESTIONS

Hope this helps.
Flags: needinfo?(itiel_yn8)
Hmm, that's the same as what Firefox currently uses both in normal and private mode. Does the keyboard really behave differently depending on the currently active app?

I guess I should give the Samsung Remote Test Lab mentioned in the other bug a try to see this for myself.
(In reply to Jan Henning [:JanH] from comment #11)
> Hmm, that's the same as what Firefox currently uses both in normal and
> private mode. Does the keyboard really behave differently depending on the
> currently active app?

The only app in which the keyboard behaves differently is Samsung built-in browser, and only in Secret Mode.
Comment on attachment 8814616 [details]
Bug 1266683 - Part 1 - Try harder to prevent awesome bar keyboard text prediction during private browsing.

Yeah, the incompatibility with TYPE_TEXT_VARIATION_URI is a problem. On the Sony phone I lose a bunch of helpful buttons like "/" and ".com".
Attachment #8814616 - Flags: feedback?(s.kaspari) → feedback-
Comment on attachment 8814616 [details]
Bug 1266683 - Part 1 - Try harder to prevent awesome bar keyboard text prediction during private browsing.

Updated to blacklist selected keyboards only, which at the moment means Swiftkey and Samsung.
Swiftkey really doesn't have any special URI input mode as far as I can tell (unless it somehow affects the prediction logic in the background). The Samsung keyboard does still have a special URI input mode with "/" and "www"/".com" keys, so if we really want to pursue this we have to basically decide between either usability or increased privacy during private browsing with the stock keyboard on Samsung devices. [1]

I've also tested it with Samsung's remote devices and while my internet connection is borderline slow for that, it does seem to work. I couldn't try the Samsung browser's secret mode to compare, though - when checking out a device you only get warned not to activate KNOX because it breaks the remote connection, but the browser's secret mode seems to do something similar. I've tried to enter it twice, and each time the device stopped responding.

If somebody wants to give this a try on an actual Samsung phone as well, here's a test build:
https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-c752ca3ca7b5f757e75c026e24afdc952020c5ac/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk

[1] Ideally we'd of course be able to reach out to Samsung and convince them to start respecting the NO_SUGGESTIONS flag...
Attachment #8814616 - Flags: feedback?(s.kaspari)
(In reply to Jan Henning [:JanH] from comment #16)

> If somebody wants to give this a try on an actual Samsung phone as well,
> here's a test build:
> https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-
> c752ca3ca7b5f757e75c026e24afdc952020c5ac/try-android-api-15/fennec-53.0a1.en-
> US.android-arm.apk

I can confirm no suggestions are being offered with this test build (running on Samsung Galaxy S5 G900F Android 6.0.1), but as you said, the special keys such as "/", "www." and ".com" are missing from the keyboard, whereas they are being offered on Samsung Browser in Secret Mode.
Comment on attachment 8814616 [details]
Bug 1266683 - Part 1 - Try harder to prevent awesome bar keyboard text prediction during private browsing.

Interesting approach. I wonder why Samsung can still have both in their browser: No suggestions AND special keys.

Flagging antlam to get his opinion on usability vs. increased privacy (See comment 16).
Flags: needinfo?(alam)
Attachment #8814616 - Flags: feedback?(s.kaspari) → feedback+
I think it makes sense to disable these types of behaviors in our Private Browsing mode. 

But I think it's important to point this out somewhere if we're going to make this change. For example, in the Private Browsing mode's new tab/empty state.
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #19)
> But I think it's important to point this out somewhere if we're going to
> make this change. For example, in the Private Browsing mode's new tab/empty
> state.

Good idea, although I wonder how to point out that we're only doing this for the URL bar without getting too verbose? Or maybe we should extend this behaviour to the whole page content?

To further confuse matters, it seems like Swiftkey has now gained it's own private mode, but unfortunately Android provides no way of hinting to the keyboard that it may provide suggestions, but shouldn't collect any new data, which means you have to enter and exit private mode twice, both within the browser and the IME. I've opened an Android issue for this (https://code.google.com/p/android/issues/detail?id=230720), but even if this does get indeed picked up, it'll probably be quite a while before we could actually use something like this.
A couple of things about SwiftKey:
* SwiftKey does indeed not only predict words in Firefox's URL bar, but also learn them, even despite of the NO_SUGGESTIONS flag.
* The link to the SwiftKey FAQ I provided in comment #0 seems to no longer work; it seems that they've hidden it for some reason.
* At least SwiftKey now has an Incognito mode as well, where it still predicts words but at least doesn't learn them (but this mode needs to be enabled manually).

So if the blacklist approach is used I guess SwiftKey could get on it, as it doesn't cooperate properly.  Also this keyboard doesn't change in URL mode anyway; the keys are the same (not a problem since '/' is accessible by long-pressing 'M').
Assignee: jh+bugzilla → nobody
Attachment #8815440 - Attachment is obsolete: true
Attachment #8814616 - Attachment is obsolete: true
While I never received any response to that Android issue I opened, I've now learned that the recent Gboard update introduced an Incognito mode as well, which as of now only works automatically while using Chrome's private browsing mode.

After a litte further research this turned out to be implemented via a new IME options flag called IME_FLAG_NO_PERSONALIZED_LEARNING (https://developer.android.com/reference/android/support/v13/view/inputmethod/EditorInfoCompat.html#IME_FLAG_NO_PERSONALIZED_LEARNING), which is basically just what I had in mind.

Given this, I want to try and see how easily my previous patch can be extended to cover not only our URL bar, but web content as well.

Technically, supporting this feature depends on us building with v26 of the support library (bug 1385464), but as all we need is the definition of this flag value, we can easily get by with copying that definition ourselves and replacing that with the official reference later on, once we actually build with the new support library.
Assignee: nobody → jh+bugzilla
Depends on: 1385464
OS: Unspecified → Android
Hardware: Unspecified → All
Summary: Keyboard predictions in URL bar should be disabled in private browsing → Send IME_FLAG_NO_PERSONALIZED_LEARNING in private browsing (URL bar and content)
Blocks: 1385726
No longer depends on: 1385464
Comment on attachment 8893033 [details]
Bug 1266683 - Part 0 - Add TargetApi annotation to silence linter.

https://reviewboard.mozilla.org/r/162844/#review169442

We actually prefer version numbers over `Build.VERSION_CODES` constants, because the Android docs use numbers and not codenames.
Attachment #8893033 - Flags: review?(nchen) → review-
Comment on attachment 8893034 [details]
Bug 1266683 - Part 1 - Support IME private mode in address bar.

https://reviewboard.mozilla.org/r/162846/#review169448
Attachment #8893034 - Flags: review?(nchen) → review+
Comment on attachment 8893036 [details]
Bug 1266683 - Part 3 - Update notifyIMEContext JNI bindings to include private mode info.

https://reviewboard.mozilla.org/r/163148/#review169450
Attachment #8893036 - Flags: review?(nchen) → review+
Comment on attachment 8893037 [details]
Bug 1266683 - Part 4 - Forward private browsing state to GeckoInputConnection and use it there.

https://reviewboard.mozilla.org/r/163596/#review169452
Attachment #8893037 - Flags: review?(nchen) → review+
Comment on attachment 8893035 [details]
Bug 1266683 - Part 2 - Add private browsing mode info to InputContext.

https://reviewboard.mozilla.org/r/162848/#review169560

::: dom/events/IMEStateManager.cpp:1117
(Diff revision 1)
>       aTabParent, GetIMEStateEnabledName(aInputContext.mIMEState.mEnabled),
>       GetIMEStateSetOpenName(aInputContext.mIMEState.mOpen),
>       NS_ConvertUTF16toUTF8(aInputContext.mHTMLInputType).get(),
>       NS_ConvertUTF16toUTF8(aInputContext.mHTMLInputInputmode).get(),
>       NS_ConvertUTF16toUTF8(aInputContext.mActionHint).get(),
> +     aInputContext.mInPrivateBrowsing ? "true" : "false",

Use GetBoolName().

::: dom/events/IMEStateManager.cpp:1181
(Diff revision 1)
> -  if (aContent &&
> -      aContent->IsAnyOfHTMLElements(nsGkAtoms::input, nsGkAtoms::textarea)) {
> +  if (aContent) {
> +    context.mInPrivateBrowsing =
> +      nsContentUtils::IsInPrivateBrowsing(aContent->OwnerDoc());

aContent can be nullptr if focused document is in designMode. So, this patch doesn't check if it's in private browsing when focus is in designMode editor.

I think that you need to make this method take nsPresContext* at second argument and then, it should always use aPresContext's document to set mInPrivateBrowsing, then, you don't need to change indent level of this block.

::: dom/events/IMEStateManager.cpp:1277
(Diff revision 1)
>       GetIMEStateEnabledName(aInputContext.mIMEState.mEnabled),
>       GetIMEStateSetOpenName(aInputContext.mIMEState.mOpen),
>       NS_ConvertUTF16toUTF8(aInputContext.mHTMLInputType).get(),
>       NS_ConvertUTF16toUTF8(aInputContext.mHTMLInputInputmode).get(),
>       NS_ConvertUTF16toUTF8(aInputContext.mActionHint).get(),
> +     aInputContext.mInPrivateBrowsing ? "true" : "false",

Use GetBoolName().

::: widget/IMEData.h:294
(Diff revision 1)
> +  /* Whether the owning document of the input element has been loaded
> +   * in private browsing mode. */
> +  bool mInPrivateBrowsing;

This increases the instance size. Please move this after mMayBeIMEUnaware.
Attachment #8893035 - Flags: review?(masayuki) → review-
Comment on attachment 8893035 [details]
Bug 1266683 - Part 2 - Add private browsing mode info to InputContext.

https://reviewboard.mozilla.org/r/162848/#review169560

> aContent can be nullptr if focused document is in designMode. So, this patch doesn't check if it's in private browsing when focus is in designMode editor.
> 
> I think that you need to make this method take nsPresContext* at second argument and then, it should always use aPresContext's document to set mInPrivateBrowsing, then, you don't need to change indent level of this block.

Thanks for pointing this out. I've implemented your suggestion and tested it with [http://www-archive.mozilla.org/editor/midasdemo/](http://www-archive.mozilla.org/editor/midasdemo/), and it's working fine that way.

> This increases the instance size. Please move this after mMayBeIMEUnaware.

Done, but so I can understand it better, what's the reasoning behind this?
Comment on attachment 8893033 [details]
Bug 1266683 - Part 0 - Add TargetApi annotation to silence linter.

https://reviewboard.mozilla.org/r/162844/#review170372
Attachment #8893033 - Flags: review?(nchen) → review+
Comment on attachment 8893035 [details]
Bug 1266683 - Part 2 - Add private browsing mode info to InputContext.

https://reviewboard.mozilla.org/r/162848/#review169560

> Done, but so I can understand it better, what's the reasoning behind this?

The reason is memory alignment (depending on compiler, though). Typically, when member size is less than 4 bytes on x86 build or 8 bytes on x64 build, they should be moved to the last of struct/class.
Comment on attachment 8893035 [details]
Bug 1266683 - Part 2 - Add private browsing mode info to InputContext.

https://reviewboard.mozilla.org/r/162848/#review170472

::: dom/events/IMEStateManager.cpp:350
(Diff revision 2)
>      IMEState newState = GetNewIMEState(sPresContext, nullptr);
>      InputContextAction action(InputContextAction::CAUSE_UNKNOWN,
>                                InputContextAction::LOST_FOCUS);
>      InputContext::Origin origin =
>        sActiveTabParent ? InputContext::ORIGIN_CONTENT : sOrigin;
> -    SetIMEState(newState, nullptr, sWidget, action, origin);
> +    SetIMEState(newState, aPresContext, nullptr, sWidget, action, origin);

No, aPresContext is being destroyed now. Please set it to nullptr here.

::: dom/events/IMEStateManager.cpp:407
(Diff revision 2)
>      IMEState newState = GetNewIMEState(sPresContext, nullptr);
>      InputContextAction action(InputContextAction::CAUSE_UNKNOWN,
>                                InputContextAction::LOST_FOCUS);
>      InputContext::Origin origin =
>        sActiveTabParent ? InputContext::ORIGIN_CONTENT : sOrigin;
> -    SetIMEState(newState, nullptr, sWidget, action, origin);
> +    SetIMEState(newState, aPresContext, nullptr, sWidget, action, origin);

Although, focused element becomes "unknown" temporarily. So, using aPresContext here is odd (see below, sPresContext becomes nullptr).

However, in most cases, focus will be back to the document immediately. So, even if it's very short period, we should keep private mode. So, this is fine.

::: dom/events/IMEStateManager.cpp:1109
(Diff revision 2)
>  {
>    MOZ_LOG(sISMLog, LogLevel::Info,
>      ("SetInputContextForChildProcess(aTabParent=0x%p, "
>       "aInputContext={ mIMEState={ mEnabled=%s, mOpen=%s }, "
>       "mHTMLInputType=\"%s\", mHTMLInputInputmode=\"%s\", mActionHint=\"%s\" }, "
> -     "aAction={ mCause=%s, mAction=%s }), "
> +     "mInPrivateBrowsing=%s, aAction={ mCause=%s, mAction=%s }), "

Move previous line's |}, | to after |minPrivateBrowing=%s |. Then, put |, | to the end of the previous line. (data in {} means a struct or class members.)

::: dom/events/IMEStateManager.cpp:1183
(Diff revision 2)
>    context.mOrigin = aOrigin;
>    context.mMayBeIMEUnaware = context.mIMEState.IsEditable() &&
>      sCheckForIMEUnawareWebApps && MayBeIMEUnawareWebApp(aContent);
>  
> +  context.mInPrivateBrowsing =
> +    nsContentUtils::IsInPrivateBrowsing(aPresContext->Document());

So, aPresContext may be nullptr. So,

context.mInPrivateBrowsing =
  aPresContext &&
  nsContentUtils::IsInPrivateBrowsing(aPresContext->Document());

::: dom/events/IMEStateManager.cpp:1270
(Diff revision 2)
>  {
>    MOZ_LOG(sISMLog, LogLevel::Info,
>      ("SetInputContext(aWidget=0x%p, aInputContext={ "
>       "mIMEState={ mEnabled=%s, mOpen=%s }, mHTMLInputType=\"%s\", "
>       "mHTMLInputInputmode=\"%s\", mActionHint=\"%s\" }, "
> +     "mInPrivateBrowsing=%s, "

Move previous line's |}, | to the end of this line and put |, | at the end of the previous line.
Attachment #8893035 - Flags: review?(masayuki) → review+
Comment on attachment 8893035 [details]
Bug 1266683 - Part 2 - Add private browsing mode info to InputContext.

https://reviewboard.mozilla.org/r/162848/#review170472

> No, aPresContext is being destroyed now. Please set it to nullptr here.

Yes, I wasn't quite sure what to do about these two cases SetIMEState is called with aContent = nullptr, either. But you're right, if the presentation is going away then the private browsing state won't matter much anymore, either.

> Move previous line's |}, | to after |minPrivateBrowing=%s |. Then, put |, | to the end of the previous line. (data in {} means a struct or class members.)

Duh, I should have realised that myself. Thanks for catching this.
Comment on attachment 8893035 [details]
Bug 1266683 - Part 2 - Add private browsing mode info to InputContext.

https://reviewboard.mozilla.org/r/162848/#review169560

> The reason is memory alignment (depending on compiler, though). Typically, when member size is less than 4 bytes on x86 build or 8 bytes on x64 build, they should be moved to the last of struct/class.

Ah right, that makes sense.
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/245b0b11e970
Part 0 - Add TargetApi annotation to silence linter. r=jchen
https://hg.mozilla.org/integration/autoland/rev/225fa7371233
Part 1 - Support IME private mode in address bar. r=jchen
https://hg.mozilla.org/integration/autoland/rev/b68bb4d30318
Part 2 - Add private browsing mode info to InputContext. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/dcec0d71c411
Part 3 - Update notifyIMEContext JNI bindings to include private mode info. r=jchen
https://hg.mozilla.org/integration/autoland/rev/ff9b9d879201
Part 4 - Forward private browsing state to GeckoInputConnection and use it there. r=jchen
Release Note Request (optional, but appreciated)
[Why is this notable]: We now support a new Android feature that enables apps to tell the IME that it shouldn't store user input into its typing history/user language model/etc. So far, Chrome has already implemented this for their Incognito tabs, although at the moment Gboard is the only keyboard that I know of which supports this.
[Affects Firefox for Android]: Only.
[Suggested wording]: Automatically enable private mode on compatible keyboards during private browsing.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Blocks: 1388260
This was added to 57 beta relnotes.
Duplicate of this bug: 1425256
You need to log in before you can comment on or make changes to this bug.