1211805 - Improve Surface Pro 3 dock + USB keyboard detection

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[bvanoudtshoorn]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151005004046

Steps to reproduce:

(This issue has been broken out from #1192573).

1. On a Surface Pro 3 with Windows 10, enable the "Automatically show the touch keyboard in windowed apps when there's no keyboard attached to your device" option.
2. Connect the Surface to a dock, with a USB keyboard attached.
3. Focus any text-entry field in the Firefox chrome or within a web page.


Actual results:

The on-screen keyboard is shown.


Expected results:

Firefox should a) detect the presence of a dock, and b) the presence of a physical keyboard attached to the dock, and not show the on-screen keyboard.

Comment 1

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

Hi bvanoudtshoorn, I have added code to Firefox Nightly which will report the reason why the on-screen keyboard is being display or not displayed. This code will be in Firefox Nightly starting tomorrow (10/9/2015).

Can you download and run Nightly tomorrow, go to about:support, and copy the rows of "Important Modified Preferences" that start with "ui.osk" and paste them here?

Comment 2

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

Attachment: Patch

Gijs, what do you think about this? We may have been bailing out early when we found that the device was in slate mode before we noticed that there was a connected keyboard. This refactoring moves the check for a connected keyboard to occur before we check if we are in a slate or mobile role.

With this moved, this function was always going to return false after checking the attached devices, which seemed wrong to me as it basically removed any usefulness of checking the platform power role. To try to make this check still robust I modified it to return `true` if the power role is not mobile or slate.

Comment 3

[:Gijs (he/him)]

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

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

How much of this code used to be in line with the chromium version?

The change generally looks good to me, but I have a number of general issues with the code (which predates this patch, so if you prefer please tell me you want to land anyway and fix properly in a followup):

Reading https://msdn.microsoft.com/en-us/library/windows/desktop/aa372728%28v=vs.85%29.aspx it seems that:

1) the call as-is will never return PlatformRoleSlate.
2) we should be using PowerDeterminePlatformRoleEx, because this code is guaranteed to run on win8 and later (right??)
3) we should cache the result of that function because it looks like this is reading the ACPI table and that result will be constant per machine (on a single Firefox run)

and furthermore, if the role is not mobile/slate, I think we don't need to check for attached devices every time - we should assume that non-slate/mobile things (like desktops) have keyboards attached, because the current implementation here assumes that anyway (in that it'll still return true even if we can't find a keyboard. :-)

IOW, I think a better approach would be to use a different API, cache the result, and bail early if *not* slate/mobile (and thereby not show the OSK), and then do the more laborious work of checking attached devices after we've established we're on a Mobile/Slate device.

Does that make sense?

::: widget/windows/WinIMEHandler.cpp
@@ +713,5 @@
> +    reinterpret_cast<PowerDeterminePlatformRole>(::GetProcAddress(
> +      ::LoadLibraryW(L"PowrProf.dll"), "PowerDeterminePlatformRole"));
> +  if (power_determine_platform_role) {
> +    POWER_PLATFORM_ROLE role = power_determine_platform_role();
> +    if (((role == PlatformRoleMobile) || (role == PlatformRoleSlate)) &&

Nit: Are the extra parens around the == clauses really necessary here? I think just:

if ((foo == x || foo == y) && bar == 0)

would be OK here (plus indentation and the necessary actual values instead of "foo" and "bar" and "x" and "y" :-)

@@ +717,5 @@
> +    if (((role == PlatformRoleMobile) || (role == PlatformRoleSlate)) &&
> +         (::GetSystemMetrics(SM_CONVERTIBLESLATEMODE) == 0)) {
> +      if (role == PlatformRoleMobile) {
> +        Preferences::SetString(kOskDebugReason, L"IKPOS: PlatformRoleMobile.");
> +      } else if (role == PlatformRoleSlate) {

This is guaranteed to be the case. Could just do:

Preferences::SetString(kOskDebugReason, role == PlatformRoleMobile ? 
                                        L"IKPOS: PlatformRoleMobile" :
                                        L"IKPOS: PlatformRoleSlate");

@@ +721,5 @@
> +      } else if (role == PlatformRoleSlate) {
> +        Preferences::SetString(kOskDebugReason, L"IKPOS: PlatformRoleSlate.");
> +      }
> +      return false;
> +    } else {

Nit: No else after return.

Comment 4

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1211805 - rework keyboard detection for on-screen keyboard, r?jaws

Bug 1211805 - rework keyboard detection for on-screen keyboard, r?jaws

Comment 5

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

Comment on attachment 8675024 [details]
MozReview Request: Bug 1211805 - rework keyboard detection for on-screen keyboard, r?jaws

https://reviewboard.mozilla.org/r/22319/#review19919

::: widget/windows/WinIMEHandler.cpp:665
(Diff revision 1)
> -  PowerDeterminePlatformRole power_determine_platform_role =
> -    reinterpret_cast<PowerDeterminePlatformRole>(::GetProcAddress(
> +  static POWER_PLATFORM_ROLE role;
> +  static bool determinedPowerPlatformRole;

Both of these variables should be initialized.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/10415ea81de6

Comment 7

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/10415ea81de6

Comment 8

[:Gijs (he/him)]

Comment on attachment 8675024 [details]
MozReview Request: Bug 1211805 - rework keyboard detection for on-screen keyboard, r?jaws

Approval Request Comment
[Feature/regressing bug #]: better keyboard (or lack thereof) detection for the on-screen keyboard support
[User impact if declined]: sometimes the on-screen keyboard shows when it shouldn't. This is the only known bug that stops us from shipping this in release, and all other browsers have supported on-screen keyboard (touch) for a while now.
[Describe test coverage new/current, TreeHerder]: no, because this depends on system hardware configuration, so we can't easily test in automation
[Risks and why]: pretty limited risk. Similar code is in use in Chromium, and this code should only affect when or whether the on screen keyboard shows. There are both operating system prefs and a number of Firefox prefs that we can use to disable this feature if need me.
[String/UUID change made/needed]: no

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this adds new support for hardware, somewhat experimentally, and we're uplifting the work to aurora.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8675024 [details]
MozReview Request: Bug 1211805 - rework keyboard detection for on-screen keyboard, r?jaws

Can be easily preffed off, adds support for major platform. Approved for uplift to aurora.

Comment 11

[Carsten Book [:Tomcat]]

this cause problems on uplifting to aurora, jaws could you take a look:

grafting 309614:10415ea81de6 "Bug 1211805 - rework keyboard detection for on-screen keyboard, r=jaws"
merging widget/windows/WinIMEHandler.cpp
warning: conflicts during merge.
merging widget/windows/WinIMEHandler.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')

Comment 12

[:Gijs (he/him)]

Seeing as I wrote the patch, I'd better fix the uplift...

Comment 13

[:Gijs (he/him)]

This depends on bug 1210033 being uplifted.

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ebb2c91a702