1007063 - Virtual (on-screen) keyboard does not appear in Windows 8 desktop mode and windows 10 tablet mode

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[discd23-mich]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

I installed Firefox 29 on my Windows tablet.
When touching the address bar the keyboard does not pop up.


Actual results:

Nothing, the keyboard does not pop up.


Expected results:

The keyboard should pop up, like it does in Chrome even in Desktop mode.

Chrome has implemented this functionality and so should Firefox.

 Firefox metro is no more, but Mozilla should still enable users to use Firefox on Windows tablets.

Comment 7

[:Gijs (he/him)]

(AIUI this needs us to implement enough of UI Accessibility for windows to realize focus is in a textfield)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #7)
> (AIUI this needs us to implement enough of UI Accessibility for windows to
> realize focus is in a textfield)

My plan was to add a touch event listener to the xul:textbox binding, check that it wasn't default prevented + the element was in chrome + and it is not readonly, then call a special (new) function that will show the onscreen keyboard for Windows 8 and higher only.

Comment 9

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > (AIUI this needs us to implement enough of UI Accessibility for windows to
> > realize focus is in a textfield)
> 
> My plan was to add a touch event listener to the xul:textbox binding, check
> that it wasn't default prevented + the element was in chrome + and it is not
> readonly, then call a special (new) function that will show the onscreen
> keyboard for Windows 8 and higher only.

When I looked before, there was no manual way to force the keyboard to be shown.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This code borrows heavily from the Chromium code[1] that is designed to achieve the same effect when a text input field gets a touch event.

Tested with the location bar and search box in the navigation toolbar. This does not work for content, as it is just changing the XUL textbox control.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/base/win/win_util.cc&ct=rc&cd=2&q=displayvirtualkeyboard&sq=package:chromium&l=393&dr=CSs

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8630471 [details] [diff] [review]
Patch

Review of attachment 8630471 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/WindowsUIUtils.cpp
@@ +182,5 @@
> +  }
> +
> +  wchar_t path[MAX_PATH];
> +  HKEY key;
> +  // The path to TabTip.exe is defined at the following registry key.

I'll have to look at this, I'm not convinced this is the correct way to do this.

@@ +203,5 @@
> +      size == 0 || size % 2 != 0)
> +    return NS_OK;
> +
> +  std::wstring wstrpath(path);
> +  // The path provided by the registry will often contain %CommonProgramFiles%,

Can't we use something like ExpandEnvironmentStrings here?

https://msdn.microsoft.com/en-us/library/windows/desktop/ms724265%28v=vs.85%29.aspx

Comment 12

[:Gijs (he/him)]

Comment on attachment 8630471 [details] [diff] [review]
Patch

Review of attachment 8630471 [details] [diff] [review]:
-----------------------------------------------------------------

The XBL part of this looks alright, but I'm worried about Jim's reluctance regarding the native part here, and it's sad that this approach doesn't work for regular input controls.

Could we call the same API from somewhere in core's editor implementation instead?

::: toolkit/content/widgets/textbox.xml
@@ +250,5 @@
>  
> +#ifdef XP_WIN
> +      <handler event="pointerup">
> +        <![CDATA[
> +          if (!event.isPrimary || event.pointerType != "touch" ||

I'm not sure if your assumption that we don't need to show the OSK if you have a mouse is valid. It'd be better if we detected if a hardware keyboard is there or not, but I guess that could be a followup.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Mathies [:jimm] from comment #11)
> @@ +203,5 @@
> > +      size == 0 || size % 2 != 0)
> > +    return NS_OK;
> > +
> > +  std::wstring wstrpath(path);
> > +  // The path provided by the registry will often contain %CommonProgramFiles%,
> 
> Can't we use something like ExpandEnvironmentStrings here?
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms724265%28v=vs.
> 85%29.aspx

ExpandEnvironmentStrings here gives us "Program Files (x86)", but the TabTip.exe that is needed is found in "Program Files". The comment in the patch is supposed to explain this, but I guess it could use some re-wording.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8630471 [details] [diff] [review]
> Patch
> 
> Review of attachment 8630471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The XBL part of this looks alright, but I'm worried about Jim's reluctance
> regarding the native part here, and it's sad that this approach doesn't work
> for regular input controls.
> 
> Could we call the same API from somewhere in core's editor implementation
> instead?

Yes, moving the callsite should not be difficult. Ehsan, do you have a recommendation for where within /editor that we should show & dismiss the on-screen keyboard when a text editable control is given focus? Note that we would need to know if focus was given to via a "touch" event.
 
> ::: toolkit/content/widgets/textbox.xml
> @@ +250,5 @@
> >  
> > +#ifdef XP_WIN
> > +      <handler event="pointerup">
> > +        <![CDATA[
> > +          if (!event.isPrimary || event.pointerType != "touch" ||
> 
> I'm not sure if your assumption that we don't need to show the OSK if you
> have a mouse is valid. It'd be better if we detected if a hardware keyboard
> is there or not, but I guess that could be a followup.

Yes, I would like to handle this in a follow-up.

Comment 15

[(no longer active)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> (In reply to :Gijs Kruitbosch from comment #12)
> > Comment on attachment 8630471 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 8630471 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The XBL part of this looks alright, but I'm worried about Jim's reluctance
> > regarding the native part here, and it's sad that this approach doesn't work
> > for regular input controls.
> > 
> > Could we call the same API from somewhere in core's editor implementation
> > instead?
> 
> Yes, moving the callsite should not be difficult. Ehsan, do you have a
> recommendation for where within /editor that we should show & dismiss the
> on-screen keyboard when a text editable control is given focus? Note that we
> would need to know if focus was given to via a "touch" event.

I guess you want nsEditorEventListener::Focus/Blur.  Both of those functions receive an nsIDOMEvent event, but I'm not sure how to get from that to whether the focus is caused by a touch event.  I _think_ that information is long gone by that time.

But why do you need to know anything about the touch event here?  Surely we also want to display the virtual keyboard when for example the page script calls .focus() on an input node?

The fact that the current patch only deals with XUL textboxes really confuses me as to what the goal here is...

Comment 16

[:Gijs (he/him)]

Comment on attachment 8630471 [details] [diff] [review]
Patch

Review of attachment 8630471 [details] [diff] [review]:
-----------------------------------------------------------------

Going to clear this while we wait for Ehsan.

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #16)
> Comment on attachment 8630471 [details] [diff] [review]
> Patch
> 
> Review of attachment 8630471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Going to clear this while we wait for Ehsan.

Eh, came here from the requests page, so I hadn't seen the comment, sorry - still leaving the review cleared for now! :-)

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 8630471 [details] [diff] [review]
Patch

Review of attachment 8630471 [details] [diff] [review]:
-----------------------------------------------------------------

Various nits need addressing. I understand the weird env stuff now and unfortunately I didn't find a better way around it. All in all this change really unfortunate for a number of reasons:

1) the overhead of each call, Maybe we can cache some of the data we query for?
2) launching a background process to bring focus to text edits - omg, yuck!
3) limited application across all text inputs.

I think we might want to address some of the above before landing this. We should also consider opening up a MS ticket on this to find out if ms knows of a better way to do things.

::: widget/windows/WindowsUIUtils.cpp
@@ +185,5 @@
> +  HKEY key;
> +  // The path to TabTip.exe is defined at the following registry key.
> +  const wchar_t regKeyPath[] = L"Software\\Classes\\CLSID\\"
> +                               L"{054AAE20-4BEA-4347-8A35-64A533254A9D}\\LocalServer32";
> +  DWORD result = ::RegOpenKeyExW(HKEY_LOCAL_MACHINE,

We have a helper for this - WinUtils::GetRegistryKey. Please make sure aBufferLength is set to MAX_PATH before you call it. Also please mention this is pulled out of the 64-bit registry hive directly.

@@ +205,5 @@
> +
> +  std::wstring wstrpath(path);
> +  // The path provided by the registry will often contain %CommonProgramFiles%,
> +  // which will need to be replaced if it is present.
> +  size_t commonProgramFilesOffset = wstrpath.find(L"%CommonProgramFiles%");

I can't find any other way of dealing with this so.. I guess we'll have to live with it.

@@ +210,5 @@
> +  if (commonProgramFilesOffset != std::wstring::npos) {
> +    // The path read from the registry contains the %CommonProgramFiles%
> +    // environment variable prefix. On 64 bit Windows the SHGetKnownFolderPath
> +    // function returns the common program files path with the X86 suffix for
> +    // the FOLDERID_ProgramFilesCommon value.

Is this true if firefox is 64-bit? I can't find anything on it. You should test this on 64-bit builds to see.

@@ +218,5 @@
> +
> +    // We then replace the %CommonProgramFiles% value with the actual common
> +    // files path found in the process.
> +    std::wstring commonProgramFilesPath;
> +    wchar_t commonProgramFilesPathW6432[MAX_PATH];

MAX_PATH is around 256 chars on most systems...

@@ +219,5 @@
> +    // We then replace the %CommonProgramFiles% value with the actual common
> +    // files path found in the process.
> +    std::wstring commonProgramFilesPath;
> +    wchar_t commonProgramFilesPathW6432[MAX_PATH];
> +    DWORD bufferSize = GetEnvironmentVariable(L"CommonProgramW6432", NULL, 0);

this can return a value up to 32K in length.

@@ +223,5 @@
> +    DWORD bufferSize = GetEnvironmentVariable(L"CommonProgramW6432", NULL, 0);
> +    if (bufferSize) {
> +      GetEnvironmentVariableW(L"CommonProgramW6432",
> +                              commonProgramFilesPathW6432,
> +                              bufferSize);

this is a buffer over write waiting to happen. :)

@@ +232,5 @@
> +                                            &path);
> +      if (FAILED(result) || !path) {
> +        return NS_OK;
> +      }
> +      commonProgramFilesPath = nsDependentString(path).get();

path needs to be freed using CoTaskMemFree, you've introduced a memory leak here.

@@ +238,5 @@
> +    wstrpath.replace(commonProgramFilesOffset, wcslen(L"%CommonProgramFiles%"),
> +                     commonProgramFilesPath);
> +  }
> +
> +  HINSTANCE ret = ShellExecuteW(NULL,

With e10s does this call happens in the content process? Looks like it does and will fail with sandboxing. I think this needs to get forwarded over to the chrome process. 

If we do this then I think we need to figure out how to avoid allowing a compromised content process from using this to DOS the user. Maybe we should restrict how often this cann can be made or something? Honestly I'm not sure what do do here.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Mathies [:jimm] from comment #19)
> @@ +238,5 @@
> > +    wstrpath.replace(commonProgramFilesOffset, wcslen(L"%CommonProgramFiles%"),
> > +                     commonProgramFilesPath);
> > +  }
> > +
> > +  HINSTANCE ret = ShellExecuteW(NULL,
> 
> With e10s does this call happens in the content process? Looks like it does
> and will fail with sandboxing. I think this needs to get forwarded over to
> the chrome process. 
> 
> If we do this then I think we need to figure out how to avoid allowing a
> compromised content process from using this to DOS the user. Maybe we should
> restrict how often this cann can be made or something? Honestly I'm not sure
> what do do here.

This is the only bit that the patch doesn't handle right now. I think I need to bounce some questions off of some e10s people first.

Comment 22

[Jim Mathies [:jimm]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> (In reply to Jim Mathies [:jimm] from comment #19)
> > @@ +238,5 @@
> > > +    wstrpath.replace(commonProgramFilesOffset, wcslen(L"%CommonProgramFiles%"),
> > > +                     commonProgramFilesPath);
> > > +  }
> > > +
> > > +  HINSTANCE ret = ShellExecuteW(NULL,
> > 
> > With e10s does this call happens in the content process? Looks like it does
> > and will fail with sandboxing. I think this needs to get forwarded over to
> > the chrome process. 
> > 
> > If we do this then I think we need to figure out how to avoid allowing a
> > compromised content process from using this to DOS the user. Maybe we should
> > restrict how often this cann can be made or something? Honestly I'm not sure
> > what do do here.
> 
> This is the only bit that the patch doesn't handle right now. I think I need
> to bounce some questions off of some e10s people first.

ask away, I'm on the e10s team.

Comment 23

[Jim Mathies [:jimm]]

I'm seeing odd behavior here when I open the browser:

1) disable tablet mode, open firefox

result: 
- soft keyboard displays immediately
- the bottom of the firefox window is sized smaller by the system so that the bottom of the firefox window ends up right along the top of the keyboard in the middle of the window. The search text input which has focus is down below the lower frame of the window so it's not visible. (Basically I can't see what I'm typing.)

2) click on non-input related content

result: Windows dings at me, nothing changes in the view

3) close the soft keyboard manually using the mouse

4) click on the address bar

result: soft keyboard displays, window changes size

5) click on search bar

result: softkeyboard hides briefly, window changes size, then soft keyboard displays again and the window changes size again.

lots of weirdness here moving between text edits.

When I turn tablet mode on things improve a bit, but not entirely:

1) launch firefox from the start menu

- nothing happen except the soft keyboard is displayed. I think we need to check to see if we're in the foreground before invoking this tabtip thing.

2) swipe from the right and chose the firefox window from the list of apps

result: the main search edit is focused but no soft keyboard is displayed with the window

3) tap or click the search text input again

result: nothing happens

4) click or tap the address bar

result: soft keyboard is displayed.

One common oddity in both modes: whenever the soft keyboard is invoked by the mouse, the mouse cursor briefly changes to a wait cursor. I think this is tied to our launching of the 3rd party app.

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8632097 [details] [diff] [review]
Patch v2

Review of attachment 8632097 [details] [diff] [review]:
-----------------------------------------------------------------

With the changes specified implemented r+ on the widget bits.

::: modules/libpref/init/all.js
@@ +3179,5 @@
>  // page back/forward actions, or if pinch-to-zoom does not work.
>  pref("ui.elantech_gesture_hacks.enabled", -1);
>  
> +// Show the Windows on-screen keyboard (osk.exe) when a text field is focused.
> +pref("ui.osk.enabled", true);

lets ifdef this to false for release channels so this doesn't accidentally roll out before it's ready.

::: widget/windows/WindowsUIUtils.cpp
@@ +32,3 @@
>  
>  #pragma comment(lib, "runtimeobject.lib")
> +#pragma comment(lib, "PowrProf.lib")

We should add PowrProf.dll to the delay load list so we aren't loading it until we use it.

@@ +103,5 @@
> +  // are attached to the machine.
> +  // Based on IsKeyboardPresentOnSlate() in Chromium's base/win/win_util.cc.
> +  bool IsKeyboardPresentOnSlate() {
> +    // This function is only supported for Windows 8 and up.
> +    if (!IsWin8OrLater())

nit - braces

@@ +111,5 @@
> +      // Detection for physical keyboard has been disabled for testing.
> +      return false;
> +    }
> +
> +    bool result = false;

nit - lets move this down closer to where it is used.

@@ +160,5 @@
> +    // Check if the device is being used as a laptop or a tablet. This can be
> +    // checked by first checking the role of the device and then the
> +    // corresponding system metric (SM_CONVERTIBLESLATEMODE). If it is being used
> +    // as a tablet then we want the OSK to show up.
> +    POWER_PLATFORM_ROLE role = PowerDeterminePlatformRole();

PowerDeterminePlatformRole is only available on vista and up, you'll need to grab this using GetProcAddress. Also you can use decltype in declaring the function type if you want to save some effort.

@@ +311,5 @@
> +WindowsUIUtils::ShowOnScreenKeyboard()
> +{
> +  if (!IsWin8OrLater() ||
> +      IsKeyboardPresentOnSlate() ||
> +      !Preferences::GetBool("ui.osk.enabled", true)) {

probably better to check that pref before we jump into IsKeyboardPresentOnSlate().

@@ +316,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsAutoString cachedPath;
> +  const char* kOskPathPrefName = "ui.osk.on_screen_keyboard_path";

lets move all of these pref strings up to the top of the file and declare them like you've done here.

::: widget/windows/nsLookAndFeel.h
@@ +23,5 @@
>  
> +/*
> + * Tablet mode detection
> + */
> +#ifndef SM_SYSTEMDOCKED

nit - I think this block should move into nsWindowDefs.h.

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 8632097 [details] [diff] [review]
Patch v2

Hey bob, can you look at this for issues with the sandbox? I was surprised that everything was working here, we have a ShellExecuteW call as well as a FindWindow/PostMessage calls. Is any of this going to break at some point thanks to sandbox restrictions?

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Mathies [:jimm] from comment #24)
> > +/*
> > + * Tablet mode detection
> > + */
> > +#ifndef SM_SYSTEMDOCKED
> 
> nit - I think this block should move into nsWindowDefs.h.

When I #include nsWindowDefs.h in WindowsUIUtils.cpp I get the following build error, https://pastebin.mozilla.org/8839225

It doesn't make much sense to me, and I've narrowed it down to simply including that file with no other changes to nsWindowDefs.h.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch 1.1.1

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8632290 [details] [diff] [review]
Patch 1.1.1

I wonder, is this really necessary even with TSF mode? Looks like TSF supports software keyboard: https://msdn.microsoft.com/en-us/library/windows/desktop/dd369151%28v=vs.85%29.aspx

So, I guess that you can control software keyboard without such hacky code...

And also, why don't use handle it in nsWindow::SetInputContext()? Then, you don't need to hack in nsEditor nor change the interface.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

FYI:
https://msdn.microsoft.com/en-us/library/windows/apps/hh465404.aspx
https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide

Comment 31

[:Gijs (he/him)]

Comment on attachment 8632290 [details] [diff] [review]
Patch 1.1.1

Review of attachment 8632290 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I can add much here beyond the reviews from Bob/Jim/Masayuki.

Comment 32

[Bob Owen (:bobowen)]

Comment on attachment 8632290 [details] [diff] [review]
Patch 1.1.1

Review of attachment 8632290 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jim Mathies [:jimm] from comment #25)
> Comment on attachment 8632097 [details] [diff] [review]
> Patch v2
> 
> Hey bob, can you look at this for issues with the sandbox? I was surprised
> that everything was working here, we have a ShellExecuteW call as well as a
> FindWindow/PostMessage calls. Is any of this going to break at some point
> thanks to sandbox restrictions?

In short I think that given these calls, the opening and closing of the virtual keyboard should probably be in the main process.

Low integrity still allows read access, which is probably why ShellExecuteW is working.
Eventually we want to get rid of read access in the content process, so I imagine it would then fail.

I want to turn on using an alternate Desktop soon, which I think could also cause problems.

I think PostMessage would always continue to work, but only to windows with the same or lower integrity level.
So, if the opening of the keyboard is in the main process then it would fail.
However, if we're using an alternate desktop I think the FindWindow would possibly fail anyway.

Does Chromium do all this in the main process? (I'm guessing it does)

You could try this patch with a stronger sandbox using the pref:
security.sandbox.content.level=2

This isn't as strong as the the Chromium renderer sandbox, but it's what I'm aiming for initially.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #29)
> Comment on attachment 8632290 [details] [diff] [review]
> Patch 1.1.1
> 
> I wonder, is this really necessary even with TSF mode? Looks like TSF
> supports software keyboard:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd369151%28v=vs.
> 85%29.aspx
> 
> So, I guess that you can control software keyboard without such hacky code...
> 
> And also, why don't use handle it in nsWindow::SetInputContext()? Then, you
> don't need to hack in nsEditor nor change the interface.

I would prefer to stay with the implementation that we have in the attached patch. The ISoftKbd interface will require us to select which keyboard should be shown (the documentation implies that a locale can have multiple keyboards) as well as there isn't much references online of other people using this interface. The implementation in the attached patch matches the same gross hacks that Chromium had to do to get the on-screen keyboard working for them, and I think that it would be nice to get this landed first and then we can work on making it better from there.

(In reply to Bob Owen (:bobowen) from comment #32)
> Does Chromium do all this in the main process? (I'm guessing it does)

I'm not sure how to check this.

> You could try this patch with a stronger sandbox using the pref:
> security.sandbox.content.level=2

Setting security.sandbox.content.level=2 stops the on-screen keyboard from appearing. Do you have a timeline for when level=2 will be the default?

Comment 34

[:Gijs (he/him)]

(fwiw, as noted with Jared on IRC, on my surface pro running win10, the keyboard detection doesn't work well. It seems like it might be because the screen orientation is locked. :-( )

Comment 35

[Bob Owen (:bobowen)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33)

> (In reply to Bob Owen (:bobowen) from comment #32)
> > Does Chromium do all this in the main process? (I'm guessing it does)
> 
> I'm not sure how to check this.

I just wondered if you knew, as you said some of this was borrowed from chromium.

Anyway, if this is breaking with the policy used below, it would surely break for chromium renderers as theirs is stronger, so I think it must be done in their main process.

> > You could try this patch with a stronger sandbox using the pref:
> > security.sandbox.content.level=2
> 
> Setting security.sandbox.content.level=2 stops the on-screen keyboard from
> appearing. Do you have a timeline for when level=2 will be the default?

I suspect that the alternate desktop is doing this and I want to get that into 42.

If we are desperate to get this into 40, then I would be OK with this, as long as we get a follow-up into 42, that works with the sandbox.
If this isn't going to be uplifted to Beta, then I think the changes should be made before this lands.
I'm sorry that this causes more work to get this fixed.

(In reply to :Gijs Kruitbosch from comment #34)
> (fwiw, as noted with Jared on IRC, on my surface pro running win10, the
> keyboard detection doesn't work well. It seems like it might be because the
> screen orientation is locked. :-( )

Does it work for Chrome/Chromium?
If it does, maybe the sandbox is already interfering with some of the checks.
I would expect this would be an "it works or it doesn't" situation, not an intermittent one though.
You could always try with the env var MOZ_DISABLE_CONTENT_SANDBOX=1 set to prove this.

Comment 36

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #35)
> (In reply to :Gijs Kruitbosch from comment #34)
> > (fwiw, as noted with Jared on IRC, on my surface pro running win10, the
> > keyboard detection doesn't work well. It seems like it might be because the
> > screen orientation is locked. :-( )
> 
> Does it work for Chrome/Chromium?
> If it does, maybe the sandbox is already interfering with some of the checks.
> I would expect this would be an "it works or it doesn't" situation, not an
> intermittent one though.
> You could always try with the env var MOZ_DISABLE_CONTENT_SANDBOX=1 set to
> prove this.

It doesn't work reliably in either Chrome, or Firefox with this patch. The annoying thing is my win10 build no longer offers the rotation lock pref in the action center, nor can I find it anywhere in the control panel (searching the web shows win8 screenshots of the control panel with a tickbox under the current rotation state of the display - that tickbox simply doesn't exist for me, either...).

It seems like the patch would be a strict improvement for win8 tablets though, and can probably be fixed to work correctly on win10 if not here then in a later effort, so this probably shouldn't stop the changes here from landing.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8632290 [details] [diff] [review]
Patch 1.1.1

Neil, can you review the /editor changes?

I'll work on a patch in a follow-up bug targeting Firefox42 that handles the content sandboxing that Bob brought up.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

As I said above, you shouldn't change editor. Instead, you should handle NOTIFY_IME_OF_FOCUS and NOTIFY_IME_OF_BLUR in IMEHandler::NotifyIME(). At that time, if you don't want VKB to be opened with windowless plugins, you should do nothing when input context's IME state is PLUGIN.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

And you should read this document first...
https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> Created attachment 8636155 [details] [diff] [review]
> Patch v1.2

Sorry, I hit Submit too soon. Is this what you're looking for as to the changes for WinIMEHandler? I like how we don't have to check if the editor is readonly anymore.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8636155 [details] [diff] [review]
Patch v1.2

>diff --git a/editor/libeditor/nsEditorEventListener.cpp b/editor/libeditor/nsEditorEventListener.cpp
>--- a/editor/libeditor/nsEditorEventListener.cpp
>+++ b/editor/libeditor/nsEditorEventListener.cpp
>@@ -1112,17 +1112,16 @@ nsEditorEventListener::Focus(nsIDOMEvent
> 
>   mEditor->OnFocus(target);
> 
>   nsCOMPtr<nsIPresShell> ps = GetPresShell();
>   NS_ENSURE_TRUE(ps, NS_OK);
>   nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContentForIME();
>   IMEStateManager::OnFocusInEditor(ps->GetPresContext(), focusedContent,
>                                    mEditor);
>-

Remove this change from this patch.

>diff --git a/widget/windows/WinIMEHandler.cpp b/widget/windows/WinIMEHandler.cpp
>--- a/widget/windows/WinIMEHandler.cpp
>+++ b/widget/windows/WinIMEHandler.cpp
>@@ -10,16 +10,17 @@
> #include "nsWindowDefs.h"
> 
> #ifdef NS_ENABLE_TSF
> #include "nsTextStore.h"
> #endif // #ifdef NS_ENABLE_TSF
> 
> #include "nsWindow.h"
> #include "WinUtils.h"
>+#include "nsIWindowsUIUtils.h"

This is in widget/windows. So, why don't you use WindowsUIUtins directly? Then, you don't need the interface change.

>@@ -242,24 +243,42 @@ IMEHandler::NotifyIME(nsWindow* aWindow,
>       nsIMM32Handler::OnUpdateComposition(aWindow);
>       return NS_OK;
>     case NOTIFY_IME_OF_SELECTION_CHANGE:
>       nsIMM32Handler::OnSelectionChange(aWindow, aIMENotification);
>       return NS_OK;
>     case NOTIFY_IME_OF_MOUSE_BUTTON_EVENT:
>       return nsIMM32Handler::OnMouseButtonEvent(aWindow, aIMENotification);
> #ifdef NS_ENABLE_TSF
>+    case NOTIFY_IME_OF_FOCUS:
>+      if (!sPluginHasFocus) {
>+        nsresult rv;
>+        nsCOMPtr<nsIWindowsUIUtils> windowsUIUtils =
>+          do_GetService("@mozilla.org/windows-ui-utils;1", &rv);
>+        if (NS_SUCCEEDED(rv)) {
>+          windowsUIUtils->ShowOnScreenKeyboard();
>+        }
>+      }
>+      return NS_OK;

Why did you do this only in IMM mode? I think that you should create |MaybeShowOnScreenKeyboard()| and it's called both from here and TSF mode switch statement.

>     case NOTIFY_IME_OF_BLUR:
>+      if (!sPluginHasFocus) {
>+        nsresult rv;
>+        nsCOMPtr<nsIWindowsUIUtils> windowsUIUtils =
>+          do_GetService("@mozilla.org/windows-ui-utils;1", &rv);
>+        if (NS_SUCCEEDED(rv)) {
>+          windowsUIUtils->DismissOnScreenKeyboard();
>+        }
>+      }

Same.  And I think that you should use early return style. Then,  you can reduce the unnecessary indent.

FYI: I'll modify here in another bug. So, please be careful to merge here.

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.3

I've addressed most of the review comments, but I left the changes to the IDL.

I don't see why we should remove the methods from the IDL, as it just adds extra complexity for no gain. The IDL is already Windows-specific.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8636687 [details] [diff] [review]
Patch v1.3

>+namespace {
>+  void MaybeShowOnScreenKeyboard(bool pluginHasFocus) {
>+    if (pluginHasFocus) {
>+      return;
>+    }
>+    nsresult rv;
>+    nsCOMPtr<nsIWindowsUIUtils> windowsUIUtils =
>+      do_GetService("@mozilla.org/windows-ui-utils;1", &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+      windowsUIUtils->ShowOnScreenKeyboard();
>+    }
>+  }
>+
>+  void MaybeDismissOnScreenKeyboard(bool pluginHasFocus) {
>+    if (pluginHasFocus) {
>+      return;
>+    }
>+    nsresult rv;
>+    nsCOMPtr<nsIWindowsUIUtils> windowsUIUtils =
>+      do_GetService("@mozilla.org/windows-ui-utils;1", &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+      windowsUIUtils->DismissOnScreenKeyboard();
>+    }
>+  }
>+}

Please define them as private members of IMEHandler (be careful |#ifdef NS_ENABLE_TSF| when you add them in WinIMEHandler.h).

>       case NOTIFY_IME_OF_FOCUS:
>+        MaybeShowOnScreenKeyboard(sPluginHasFocus);
>         return nsTextStore::OnFocusChange(true, aWindow,
>                                           aWindow->GetInputContext());

I don't think that this is correct. VKB should be opened after TIP gets focus. So, it should be:

case NOTYFI_IME_OF_FOCUS: {
  nsresult rv =
    nsTextStore::OnFocusChange(true, aWindow,
                               aWindow->GetInputContext());
  MaybeShowOnScreenKeyboard(sPluginHasFocus);
  return rv;
}

>       case NOTIFY_IME_OF_BLUR:
>+        MaybeDismissOnScreenKeyboard(sPluginHasFocus);
>         return nsTextStore::OnFocusChange(false, aWindow,
>                                           aWindow->GetInputContext());

But I think this is correct at blur.


> I've addressed most of the review comments, but I left the changes to the IDL.
> 
> I don't see why we should remove the methods from the IDL, as it just adds extra complexity for no gain. The IDL is already Windows-specific.

I think that this should be checked by jimm again. Jimm, VKB should be controlled from widget/windows/WinIMEHandler.cpp. Therefore, neither ShowOnScreenKeyboard() nor DismissOnScreenKeyboard() needs to be interface methods (and virtual methods). How do you think about these methods? Should they stay in the interface?

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

FYI: I landed a change in WinIMEHandler.cpp. You need to merge with these patches:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4f6847517288

Comment 46

[Jim Mathies [:jimm]]

Comment on attachment 8636687 [details] [diff] [review]
Patch v1.3

Review of attachment 8636687 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #44)
> I think that this should be checked by jimm again. Jimm, VKB should be
> controlled from widget/windows/WinIMEHandler.cpp. Therefore, neither
> ShowOnScreenKeyboard() nor DismissOnScreenKeyboard() needs to be interface
> methods (and virtual methods). How do you think about these methods? Should
> they stay in the interface?

I like the idea of this behavior being controlled internally by focus and ime code.. provided it works the way front end engineers expect it to. :) Jaws can make that call. If the keyboard "just works" without front end apis calls.. great!

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

FYI: Now, nsIMM32Handler is renamed to IMMHandler:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6dfb81107960

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Thanks, I've removed the interface changes.

Comment 49

[Jim Mathies [:jimm]]

fyi, the release version of 10 doesn't have a soft keyboard icon in the taskbar. So basically there's no way to use firefox in tablet mode currently. I might be missing a work around? If not we should push the priority of this up.

Comment 50

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #49)
> fyi, the release version of 10 doesn't have a soft keyboard icon in the
> taskbar. So basically there's no way to use firefox in tablet mode
> currently. I might be missing a work around? If not we should push the
> priority of this up.

Found the work around: press-hold on the taskbar, you'll get an option to display the touch keyboard in the task tray.

Comment 51

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Mathies [:jimm] from comment #50)
> (In reply to Jim Mathies [:jimm] from comment #49)
> > fyi, the release version of 10 doesn't have a soft keyboard icon in the
> > taskbar. So basically there's no way to use firefox in tablet mode
> > currently. I might be missing a work around? If not we should push the
> > priority of this up.
> 
> Found the work around: press-hold on the taskbar, you'll get an option to
> display the touch keyboard in the task tray.

Thanks for finding the work-around. I do think though that this should get higher priority since the button is now in secondary UI.

Comment 52

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8639383 [details] [diff] [review]
Patch v2

Review of attachment 8639383 [details] [diff] [review]:
-----------------------------------------------------------------

Makoto, are you able to review this patch? Masayuki is away and we'd like to get this landed sooner so that we can potentially uplift this.

Comment 53

[Makoto Kato [:m_kato]]

Comment on attachment 8639383 [details] [diff] [review]
Patch v2

Review of attachment 8639383 [details] [diff] [review]:
-----------------------------------------------------------------

You should use UNICODE API (suffix is W) correctly if using unicode string.  Chromium code defines UNICODE=1, but Gecko isn't.

::: widget/windows/WinIMEHandler.cpp
@@ +22,5 @@
> +#include "PowrProf.h"
> +#include "Setupapi.h"
> +#include "cfgmgr32.h"
> +
> +#pragma comment(lib, "PowrProf.lib")

Could you move this to toolkit/library/moz.build ?

@@ +83,5 @@
> +
> +    typedef BOOL (WINAPI* GetAutoRotationState)(PAR_STATE state);
> +    GetAutoRotationState get_rotation_state =
> +        reinterpret_cast<GetAutoRotationState>(::GetProcAddress(
> +            GetModuleHandle(L"user32.dll"), "GetAutoRotationState"));

GetModuleHandleW

@@ +104,5 @@
> +    // as a tablet then we want the OSK to show up.
> +    typedef POWER_PLATFORM_ROLE (WINAPI* PowerDeterminePlatformRole)();
> +    PowerDeterminePlatformRole power_determine_platform_role =
> +      reinterpret_cast<PowerDeterminePlatformRole>(::GetProcAddress(
> +        LoadLibrary(L"PowrProf.dll"), "PowerDeterminePlatformRole"));

LoadLibraryW?

@@ +120,5 @@
> +
> +    bool result = false;
> +    // Query for all the keyboard devices.
> +    HDEVINFO device_info =
> +        SetupDiGetClassDevs(&KEYBOARD_CLASS_GUID, NULL, NULL, DIGCF_PRESENT);

nullptr instead of NULL

@@ +713,5 @@
> +      // We then replace the %CommonProgramFiles% value with the actual common
> +      // files path found in the process.
> +      std::wstring commonProgramFilesPath;
> +      std::vector<wchar_t> commonProgramFilesPathW6432;
> +      DWORD bufferSize = GetEnvironmentVariable(L"CommonProgramW6432", NULL, 0);

GetEnvironmentVariableW?

@@ +745,5 @@
> +                                L"",
> +                                cachedPathPtr,
> +                                NULL,
> +                                NULL,
> +                                SW_SHOW);

nullptr instead of NULL

@@ +764,5 @@
> +
> +  // Dismiss the virtual keyboard by generating the ESC keystroke
> +  // programmatically.
> +  const wchar_t kOSKClassName[] = L"IPTip_Main_Window";
> +  HWND osk = ::FindWindow(kOSKClassName, NULL);

FindWindowW?

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8639383 [details] [diff] [review]
Patch v2

>+namespace {
>+  bool WStringStartsWithCaseInsensitive(const std::wstring& haystack, const std::wstring& needle) {
>+    std::wstring lowerCaseHaystack(haystack);
>+    std::wstring lowerCaseNeedle(needle);
>+    std::transform(lowerCaseHaystack.begin(), lowerCaseHaystack.end(), lowerCaseHaystack.begin(), ::tolower);
>+    std::transform(lowerCaseNeedle.begin(), lowerCaseNeedle.end(), lowerCaseNeedle.begin(), ::tolower);
>+    return wcsstr(lowerCaseHaystack.c_str(), lowerCaseNeedle.c_str()) == lowerCaseHaystack.c_str();
>+  }

If this is moved to WinIMEHandler.cpp, please implement this and following methods as private member of IMEHandler. And please keep the 80 char per line rule.

>+    GetAutoRotationState get_rotation_state =
>+        reinterpret_cast<GetAutoRotationState>(::GetProcAddress(
>+            GetModuleHandle(L"user32.dll"), "GetAutoRotationState"));

Odd indent. Please use 2 spaces.

>+    // corresponding system metric (SM_CONVERTIBLESLATEMODE). If it is being used

Over 80 chars.

>+    const GUID KEYBOARD_CLASS_GUID =
>+        { 0x4D36E96B, 0xE325,  0x11CE,
>+            { 0xBF, 0xC1, 0x08, 0x00, 0x2B, 0xE1, 0x03, 0x18 } };

Use 2 spaces to indent.

>+    HDEVINFO device_info =
>+        SetupDiGetClassDevs(&KEYBOARD_CLASS_GUID, NULL, NULL, DIGCF_PRESENT);

Ditto.

>+      if (!SetupDiEnumDeviceInfo(device_info, i, &device_info_data))
>+        break;

Please use {} even if the block contains only one line.

>+#ifdef NS_ENABLE_TSF

I don't understand why you implement the following methods with NS_ENABLE_TSF. They are not related to TSF and actually your patch calls them outside of this #ifdef.

>+// static
>+void
>+IMEHandler::MaybeShowOnScreenKeyboard(bool pluginHasFocus) {

Put the { to the nexe line.

>+// static
>+void
>+IMEHandler::MaybeDismissOnScreenKeyboard(bool pluginHasFocus) {

ditto.

>+  nsresult result = Preferences::GetString(kOskPathPrefName, &cachedPath);
>+  if (FAILED(result) || cachedPath.IsEmpty()) {

NS_FAILED()?

>+    if (!WinUtils::GetRegistryKey(HKEY_LOCAL_MACHINE,
>+                                  L"",

Over 80 chars. You should make this const and separate two lines at its definition like:

const wchar_t kFooBar[] =
  L"Software\\Classes\\CLSID\\"
  L"{054AAE20-4BEA-4347-8A35-64A533254A9D}\\LocalServer32";

>+    std::wstring wstrpath(path);
>+    // The path provided by the registry will often contain %CommonProgramFiles%,

Over 80 chars.

>+      DWORD bufferSize = GetEnvironmentVariable(L"CommonProgramW6432", NULL, 0);
>+      if (bufferSize) {
>+        commonProgramFilesPathW6432.resize(bufferSize);
>+        GetEnvironmentVariableW(L"CommonProgramW6432",
>+                                commonProgramFilesPathW6432.data(),
>+                                bufferSize);
>+        commonProgramFilesPath = std::wstring(commonProgramFilesPathW6432.data());

Over 80 chars.

>+        HRESULT hres = SHGetKnownFolderPath(FOLDERID_ProgramFilesCommon, 0, NULL,
>+                                              &path);

Odd indent. You should remove 2 spaces. And the first line is over 80 chars.

>+        if (FAILED(hres) || !path) {
>+          return;
>+        }
>+        commonProgramFilesPath = nsDependentString(path).get();
>+        CoTaskMemFree(path);

Could you append "::" to the head when you use Win32 API?

>+      }
>+      wstrpath.replace(commonProgramFilesOffset, wcslen(L"%CommonProgramFiles%"),

over 80 chars.

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

>>+    if (!WinUtils::GetRegistryKey(HKEY_LOCAL_MACHINE,
>>+                                  L"",
> 
> Over 80 chars. You should make this const and separate two lines at its definition like:
> 
> const wchar_t kFooBar[] =
>   L"Software\\Classes\\CLSID\\"
>   L"{054AAE20-4BEA-4347-8A35-64A533254A9D}\\LocalServer32";

Oops, the L"" includes "Software\\Classes\\CLSID\\{054AAE20-4BEA-4347-8A35-64A533254A9D}\\LocalServer32", then, the line becomes over 80 characters. I cut it for the example :-(

Comment 56

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8643501 [details] [diff] [review]
Patch v3

Looks good except some nits (sorry, I should've realized that at the previous review).

>+// static
>+void
>+IMEHandler::MaybeShowOnScreenKeyboard(bool pluginHasFocus)

Should be aPluginHasFocus. But why don't you check sPluginHasFocus directly?

>+// static
>+void
>+IMEHandler::MaybeDismissOnScreenKeyboard(bool pluginHasFocus)

Same. I believe that it's enough to check sPluginHasFocus directly.

>+// static
>+bool
>+IMEHandler::WStringStartsWithCaseInsensitive(const std::wstring& haystack,
>+                                             const std::wstring& needle)

aHaystack and aNeedle.

>+  static void MaybeShowOnScreenKeyboard(bool pluginHasFocus);
>+  static void MaybeDismissOnScreenKeyboard(bool pluginHasFocus);
>+  static bool WStringStartsWithCaseInsensitive(const std::wstring& haystack,
>+                                               const std::wstring& needle);

Same, use a prefix.

>+typedef enum _AR_STATE {

Put the { to the next line.

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/621a6c6aa375

Comment 59

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Backed out for mass WinXP issues.
https://hg.mozilla.org/integration/fx-team/rev/d9c2db12593c

Comment 61

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6e3d40edf181

Comment 62

[Jim Mathies [:jimm]]

What broke on xp?

Comment 63

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Mathies [:jimm] from comment #62)
> What broke on xp?

Using ::SHGetKnownFolderPath which doesn't exist pre-Vista. I used WinUtils::SHGetKnownFolderPath to fix it.

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e3d40edf181

Comment 65

[Thomas]

Now it is completely broken.
Every time when you enter an editable field the on screen keyboard pops up.
Even if I deactivate the Windows 10 service for that.

I looked up the about config an found this one:
ui.osk.on_screen_keyboard_path
Content of the field is: "C:\Program Files\Common Files\microsoft shared\ink\TabTip.exe"
The entry is highlighted. i got edited somehow.
So I did a reset on that one but as soon as I click an editable field: the entry is back again and the on screeen keyboard pops up.

Quite annoyig

Comment 66

[Thomas]

If you modify this entry to something else (I tried a -) the problem is gone

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Jared:

If you cannot work on the regressions now or need much time to fix them, please back the patch out or disable the feature in default settings before next merge.

Comment 68

[:Gijs (he/him)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #67)
> Jared:
> 
> If you cannot work on the regressions now or need much time to fix them,
> please back the patch out or disable the feature in default settings before
> next merge.

I think we should disable on aurora immediately after the uplift/merge, but not before - we should keep this on on Nightly in order to collect more feedback about regressions and other issues with it.

Comment 69

[Jared Wein [:jaws] (please needinfo? me)]

I agree with Gijs, we should leave this enabled to get more feedback. The patch that landed already is set to disable this feature on release builds (release builds = Beta and Release).

Comment 70

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Yeah, I agree with Gijs's idea. However, I still think that this is not enough stable for Aurora for now because even if you'll fix all reported bugs in next 6 weeks, Aurora (Dev-Edition) users need to wait Aurora 43. So, I believe that we should disable the pref on Aurora 42.

Comment 71

[Jim Mathies [:jimm]]

This currently isn't working for my in today's nightly, win10 release. Is there something I need to flip?

Comment 72

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from comment #71)
> This currently isn't working for my in today's nightly, win10 release. Is
> there something I need to flip?

It's been disabled outside of tablet mode, which may be what you're running into.

Comment 73

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Mathies [:jimm] from comment #71)
> This currently isn't working for my in today's nightly, win10 release. Is
> there something I need to flip?

You will need to set the `ui.osk.require_tablet_mode` pref to false to use the OSK in non-tablet mode.

Comment 74

[matthew.leather]

There's a setting in the keyboard options in Windows 10 (Devices -> Typing -> Automatically show the touch keyboard...) that enables tablet mode touch keyboard behaviour when tablet mode is disabled, FF nightly doesn't currently respect that setting as far as I can tell.

I use my touch device mostly in non-tablet mode using this setting so it would be nice if FF supported the option.

Comment 75

[:Gijs (he/him)]

(In reply to matthew.leather from comment #74)
> There's a setting in the keyboard options in Windows 10 (Devices -> Typing
> -> Automatically show the touch keyboard...) that enables tablet mode touch
> keyboard behaviour when tablet mode is disabled, FF nightly doesn't
> currently respect that setting as far as I can tell.
> 
> I use my touch device mostly in non-tablet mode using this setting so it
> would be nice if FF supported the option.

Filed bug 1197722 about this.

Comment 76

[yoonhoe]

Hello,

I am trying to test this virtual keyboard feature on Firefox 43.0a2 (2015-09-22) on a windows 8.1 tablet. Is this feature meant to work or has it been disabled for Aurora 43 ?

Thanks.

Comment 77

[Jared Wein [:jaws] (please needinfo? me)]

In bug 1192573 we changed the feature to require Windows 10+ tablet mode, so it currently won't appear on Windows 8.1 by default. You could go to about:config and disable the `ui.osk.require_tablet_mode` to remove this requirement.

Comment 78

[Ken]

Hi,

I've disabled `ui.osk.require_tablet_mode' but the osk doesn't appear. Using Aurora 43.a2 (2015-10-09) 64bit running on Windows 8.1 Pro on a Surface Pro 3. Any suggestions? thx

Comment 79

[:Gijs (he/him)]

(In reply to Ken from comment #78)
> Hi,
> 
> I've disabled `ui.osk.require_tablet_mode' but the osk doesn't appear. Using
> Aurora 43.a2 (2015-10-09) 64bit running on Windows 8.1 Pro on a Surface Pro
> 3. Any suggestions? thx

Comment #77 was slightly inaccurate in that the patch that landed in bug 1192573 actually required windows 10 *and* tablet mode. If you flip the pref, we won't require tablet mode to be turned on - but we'll still require windows 10. I filed bug 1213845 to address this.

Comment 80

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ken from comment #78)
> Hi,
> 
> I've disabled `ui.osk.require_tablet_mode' but the osk doesn't appear. Using
> Aurora 43.a2 (2015-10-09) 64bit running on Windows 8.1 Pro on a Surface Pro
> 3. Any suggestions? thx

Gijs has answered your direct question, but you can also go to about:support and look for "ui.osk.debug.keyboardDisplayReason" and you will see why the keyboard was or was-not displayed.

Comment 81

[Ken]

(In reply to :Gijs Kruitbosch from comment #79)
 
> Comment #77 was slightly inaccurate in that the patch that landed in bug
> 1192573 actually required windows 10 *and* tablet mode. If you flip the
> pref, we won't require tablet mode to be turned on - but we'll still require
> windows 10. I filed bug 1213845 to address this.

Thanks for the clarification. I decided to upgrade to Windows 10 Pro and I'm happy to report it's working as expected.

Comment 83

[intijk]

I am using Surface Pro 4 with Windows 10, when the Settings -> Devices -> Typing -> Automatically show the touch keyboard in windowed apps when there's no keyboard attached to your device to On. Firefox supposed to pop-up the keyboard when I click the text area, but it doesn't work. 

Tied to change to tablet mode, still doesn't work.

Comment 84

[:Gijs (he/him)]

(In reply to intijk from comment #83)
> I am using Surface Pro 4 with Windows 10, when the Settings -> Devices ->
> Typing -> Automatically show the touch keyboard in windowed apps when
> there's no keyboard attached to your device to On. Firefox supposed to
> pop-up the keyboard when I click the text area, but it doesn't work. 
> 
> Tied to change to tablet mode, still doesn't work.

Using which version of Firefox? You need 43 or later (currently developer edition / will be beta in the next week or so) in order for this to work. If you're using 43 or later, can you please file a new bug with more specifics (which version of Firefox, whether you're using "standard" win10 release or an "insider" build, etc.)? Thank you!

Comment 85

[intijk]

(In reply to :Gijs Kruitbosch from comment #84)
> (In reply to intijk from comment #83)
> > I am using Surface Pro 4 with Windows 10, when the Settings -> Devices ->
> > Typing -> Automatically show the touch keyboard in windowed apps when
> > there's no keyboard attached to your device to On. Firefox supposed to
> > pop-up the keyboard when I click the text area, but it doesn't work. 
> > 
> > Tied to change to tablet mode, still doesn't work.
> 
> Using which version of Firefox? You need 43 or later (currently developer
> edition / will be beta in the next week or so) in order for this to work. If
> you're using 43 or later, can you please file a new bug with more specifics
> (which version of Firefox, whether you're using "standard" win10 release or
> an "insider" build, etc.)? Thank you!

Thank you for let me know 43 is needed. 

I downloaded the developer version 43.0a2(2015-10-29) 64-bit windows version, I found this feature works great! Both under tablet mode and with Settings -> Devices -> Typing -> Automatically show the touch keyboard in windowed apps when there's no keyboard attached to your device to On.

My system is factory Windows 10 Pro delivered with the Surface Pro 4 with updates to today.

Comment 86

[:Gijs (he/him)]

(In reply to intijk from comment #85)
> (In reply to :Gijs Kruitbosch from comment #84)
> > (In reply to intijk from comment #83)
> > > I am using Surface Pro 4 with Windows 10, when the Settings -> Devices ->
> > > Typing -> Automatically show the touch keyboard in windowed apps when
> > > there's no keyboard attached to your device to On. Firefox supposed to
> > > pop-up the keyboard when I click the text area, but it doesn't work. 
> > > 
> > > Tied to change to tablet mode, still doesn't work.
> > 
> > Using which version of Firefox? You need 43 or later (currently developer
> > edition / will be beta in the next week or so) in order for this to work. If
> > you're using 43 or later, can you please file a new bug with more specifics
> > (which version of Firefox, whether you're using "standard" win10 release or
> > an "insider" build, etc.)? Thank you!
> 
> Thank you for let me know 43 is needed. 
> 
> I downloaded the developer version 43.0a2(2015-10-29) 64-bit windows
> version, I found this feature works great! Both under tablet mode and with
> Settings -> Devices -> Typing -> Automatically show the touch keyboard in
> windowed apps when there's no keyboard attached to your device to On.
> 
> My system is factory Windows 10 Pro delivered with the Surface Pro 4 with
> updates to today.

Great! I'll update some flags on this bug so hopefully it will be slightly more clear for others finding this bug...

Comment 87

[:Gijs (he/him)]

Vasilica, we talked about some SP2 and Dell XPS issues yesterday. Did you file those?

Comment 88

[Vasilica Tamas, QA [:vtamas]]

(In reply to :Gijs Kruitbosch from comment #87)
> Vasilica, we talked about some SP2 and Dell XPS issues yesterday. Did you
> file those?

Sorry, I’ve been busy with beta testing. 
We filed today: Bug 1221947 and Bug 1221961.

Comment 89

[cjwalle]

To contribute to this a little bit: I am running a Surface Pro 3 with the latest beta build of firefox, and standard Windows 10 Pro. The feature now works great when the keyboard is physically disconnected. Good job! However: It still does not work when the keyboard is simply folded back behind the device (which deactivates the keyboard keys). In this state, Firefox will not display the on-screen keyboard. Chrome manages this fine, for what it's worth.

Comment 90

[moyofalaye]

Thank you to everyone that has worked to get a fix up. There is still this pending issue with surface devices that have the keyboard attached, but folded back.

When the Keyboard is attached, and folded back (using the computer in tablet mode) the virtual keyboard dosent popup automatically. But when the keyboard is completely removed from the surface pro tablet, then the virtual keyboard works as it supposed to.

My system is a surface pro 4.

Thank you

Comment 91

[CB]

Hello, for what it is worth, I am still not getting any On Screen Keyboard when selecting a text field while using Aurora 45.0a1. The hardware in question is a Microsoft Surface Pro 2 running Windows 10 Pro. I have activated the Win10 feature to do so and it works fine with Edge, Chrome, and Office Suite, just not Firefox yet.

Comment 92

[moyofalaye]

@CB
please try it with battery saver OFF.

Inoticed, when battery saver is on, in Firefox,the automatic virtual keyboard stops working. About 5secs after turning it off, it starts working again.

Comment 93

[CB]

@Moyofalaye Thanks for the feedback, but I am still not having any luck, no virtual keyboard with battery saver on, battery saver off regardless of whether the keyboard is attached or not. I am sorry I am not more code savvy, I don't know how better to provide actually useful info to the project.

Comment 94

[:Gijs (he/him)]

(In reply to moyofalaye from comment #90)
> Thank you to everyone that has worked to get a fix up. There is still this
> pending issue with surface devices that have the keyboard attached, but
> folded back.
> 
> When the Keyboard is attached, and folded back (using the computer in tablet
> mode) the virtual keyboard dosent popup automatically.

This is tracked in bug 1221947.

(In reply to CB from comment #91)
> Hello, for what it is worth, I am still not getting any On Screen Keyboard
> when selecting a text field while using Aurora 45.0a1. The hardware in
> question is a Microsoft Surface Pro 2 running Windows 10 Pro. I have
> activated the Win10 feature to do so and it works fine with Edge, Chrome,
> and Office Suite, just not Firefox yet.

If you go to "about:config" in Firefox, what does the "ui.osk.debug.keyboardDisplayReason" pref says? (setting "needinfo" for this question)

The most likely explanation is that we think you have a keyboard attached to the device. Could be a wireless keyboard whose USB dongle is still in there, a folded back type cover / touch cover keyboard, etc. It's... difficult... to detect those situations as different from the case where you have a 'real' keyboard plugged in.

You can work around the current situation by flipping the "ui.osk.detect_physical_keyboard" pref to "false", which would disable the keyboard detection - of course, that will keep the feature enabled even when you do have a physical keyboard attached, and we will do our best to improve here, but it might help you as a workaround for now.

Comment 95

[jasiek.b]

Just like in case of CB onscreen keyboard does not show up automatically for me either. I'm working now on pure windows 10 tablet w/o any physical keyboard, or bluetooth, usb dongle, or anything like this - moreover nothing like this was ever connected. But from the other side Win in device manager show two "keyboard" devices ('KMDF HID Minidriver for Touch I2C Device' and 'driver for buttons of GPIO interface'). Firefox behavior is the same regardless of value in ui.osk.detect_physical_keyboard. Battery saver is of.
But now a surprise: the keyboard shows up on Google login page, when password is prompted. 
Perhaps this will give you some hint for permanent fix.

Comment 96

[:Gijs (he/him)]

(In reply to jasiek.b from comment #95)
> Just like in case of CB onscreen keyboard does not show up automatically for
> me either. I'm working now on pure windows 10 tablet w/o any physical
> keyboard, or bluetooth, usb dongle, or anything like this - moreover nothing
> like this was ever connected. But from the other side Win in device manager
> show two "keyboard" devices ('KMDF HID Minidriver for Touch I2C Device' and
> 'driver for buttons of GPIO interface'). Firefox behavior is the same
> regardless of value in ui.osk.detect_physical_keyboard. Battery saver is of.
> But now a surprise: the keyboard shows up on Google login page, when
> password is prompted. 
> Perhaps this will give you some hint for permanent fix.

What build are you testing with? The password input box is bug 1211234.

Comment 97

[jasiek.b]

(In reply to :Gijs Kruitbosch from comment #96)
> (In reply to jasiek.b from comment #95)

> > Perhaps this will give you some hint for permanent fix.
> 
> What build are you testing with? The password input box is bug 1211234.

20151029151421

Comment 98

[:Gijs (he/him)]

(In reply to jasiek.b from comment #97)
> (In reply to :Gijs Kruitbosch from comment #96)
> > (In reply to jasiek.b from comment #95)
> 
> > > Perhaps this will give you some hint for permanent fix.
> > 
> > What build are you testing with? The password input box is bug 1211234.
> 
> 20151029151421

That's 42.0, right? This feature is only available in 43 and later (currently in beta). So what you're seeing is expected.

Comment 99

[Vasilica Tamas, QA [:vtamas]]

The on-screen keyboard is successfully displayed on Firefox 43 beta 8 (20151201152349) under Windows 8.1 64-bit desktop mode and Windows 10 64-bit tablet mode, using a Dell Xps 12.

During testing, I've encountered a few known issues: https://goo.gl/NPP8jQ

Based on the above mentions, I am marking this bug as Verified since the other issues are tracked separately.

Comment 100

[(not reading bugmail) Nick Desaulniers [:\n]]

This seems to be causing an issue for users who have bluetooth keyboards connects who DO NOT WANT the virtual keyboard to be open.

Comment 101

[Klemens]

(In reply to Nick Desaulniers [:\n] from comment #100)
> This seems to be causing an issue for users who have bluetooth keyboards
> connects who DO NOT WANT the virtual keyboard to be open.

I can confirm that it caused an issue of that kind on my machine.
Suddenly appearing some day in Dec 2015, rendering smooth work with Firefox nearly impossible. Very annoying.
Fixed it finally following Thomas' comment #65 and #66. Thanks so much! But this is a work-around surely not addressed to all users...

FF version was 43.0.3 on Win 8.1 with all updates, tablet PC with BT keyboard attached.

Comment 102

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Klemens from comment #101)
> Fixed it finally following Thomas' comment #65 and #66. Thanks so much! But
> this is a work-around surely not addressed to all users...

Note that while this works, setting `ui.osk.enabled` to `false` is a much better way and will stop Firefox from trying to load the on-screen keyboard. The method described earlier will still have Firefox trying to load the keyboard, but the load will just fail.

Comment 103

[:Gijs (he/him)]

(In reply to Klemens from comment #101)
> (In reply to Nick Desaulniers [:\n] from comment #100)
> > This seems to be causing an issue for users who have bluetooth keyboards
> > connects who DO NOT WANT the virtual keyboard to be open.
> 
> I can confirm that it caused an issue of that kind on my machine.
> Suddenly appearing some day in Dec 2015, rendering smooth work with Firefox
> nearly impossible. Very annoying.
> Fixed it finally following Thomas' comment #65 and #66. Thanks so much! But
> this is a work-around surely not addressed to all users...
> 
> FF version was 43.0.3 on Win 8.1 with all updates, tablet PC with BT
> keyboard attached.

Please can you add exact details about the brand and model "tablet PC" you're using, as well as those of the BT keyboard to bug 1236058 ? We would love to fix this for everyone, but to do that we need to be able to reproduce the issue, which is highly hardware-dependent (for obvious reasons, we do hardware detection to determine whether or not a keyboard is present etc.).

Comment 104

[jigar shah]

is this really fixed ? i am on FF43...Lenovo Yoga 3 pro. win 10.it does not trigger...even on 44b4

Comment 105

[:Gijs (he/him)]

(In reply to jigar shah from comment #104)
> is this really fixed ? i am on FF43...Lenovo Yoga 3 pro. win 10.it does not
> trigger...even on 44b4

Yes. If it's not working for you, please file a new bug with more details about your system and the value of "ui.osk.debug.keyboardDisplayReason" in about:config.

Comment 106

[jigar shah]

ui.osk.debug.keyboardDisplayReason = IKPOS: ConvertibleSlateMode is non-zero

Yoga 3 Pro

Comment 107

[:Gijs (he/him)]

(In reply to jigar shah from comment #106)
> ui.osk.debug.keyboardDisplayReason = IKPOS: ConvertibleSlateMode is non-zero
> 
> Yoga 3 Pro

Your device claims it's configured as a laptop, rather than a tablet, and so we're not showing the keyboard automatically. See e.g. https://msdn.microsoft.com/en-us/library/windows/hardware/dn922653%28v=vs.85%29.aspx .

If we're making different decisions from other applications (specifically things like Edge or other "modern" Windows 10 applications), can you file a new bug with more specifics about how you're using the device when it's not working (ie is the keyboard flipped back or not?), whether you're in Windows 10 software tablet mode (turned on/off via the notification tray and/or automatically when you switch from hardware laptop to tablet mode if you opt in to that), etc.

Comment 108

[jigar shah]

its tent mode and same in tablet mode (keyboard hidden)
ui.osk.debug.keyboardDisplayReason;IITM: GetInTabletMode=true.

weird thing is..it recognizes it correctly when i click text boxes in web pages.

steps.
1. collapse keyboard. 
2.new tab. default mozilla tab screen
3. click on address-bar..no keyboard 
4.click on search box inside tab
5.on screen keyboard shows.
6. click on anywhere. it hides on screen
7. click address again. it shows keyboard
6.

Comment 109

[dbondo]

NOT FIXED!!!!!
Had touch keyboard working in Taablet mode before being updated to Firefox 4.0.1.  Now the Touch Keyboard no longer works for the address and search windows in Tablet mode.  It does come up for Websites.  This is a Dell Laptop Inspiron 13-7353, running Windows 10 version 10.0.10586, 64 bit, 8GB RAM.  This occurred about the beginning of April 2016.

Comment 110

[Matt Brubeck (:mbrubeck)]

(In reply to dbondo from comment #109)
> Had touch keyboard working in Taablet mode before being updated to Firefox
> 4.0.1.

The latest version of Firefox is Firefox 45.  If you are still having problems after installing the latest version from http://firefox.com/ please file a new bug with details.  Thanks!

Comment 111

[dbondo]

Sorry, I made a typo,  It's 45.o.1.

Comment 112

[Matt Brubeck (:mbrubeck)]

(In reply to dbondo from comment #111)
> Sorry, I made a typo,  It's 45.o.1.

In that case, please file a new bug with details about your system.  (Or see if one of the bugs linked above in the "Depends on" section describes your problem.)  Thanks!