1210033 - Add on-screen keyboard diagnostic information to about:support

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

We should show the values of the on-screen keyboard preferences as well as a reason, if one exists, for showing or not showing the on-screen keyboard.

Comment 1

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

Attachment: Patch

I'd like to land this to help us figure out why some people are having issues with the keyboard either not appearing or appearing when it shouldn't.

This patch adds a preference that caches the reasons why the keyboard was or was not shown. It also makes this available in about:support, as well as any other on-screen keyboard-related prefs that were changed.

about:support has a 120-character limit when displaying the pref values, so I abbreviated the function names to keep the strings short.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could you explain how do users access the recorded information? I'm not sure if this is valuable fix or not.

Additionally, the text is always written in English. I don't think that this helps non-English users if the value should be read by users.

Comment 3

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> Could you explain how do users access the recorded information? I'm not sure
> if this is valuable fix or not.
> 
> Additionally, the text is always written in English. I don't think that this
> helps non-English users if the value should be read by users.

The text will be visible on about:support, which can help when users report bugs. We can ask for them to send us the contents of about:support and we will have a clear understanding of the code path that is being taken.

Also, if you would like I can remove the AutoWriteStringPref class and just pass a std::wstring reference around and write to the prefs at the three places that the function returns.

Comment 4

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

Attachment: Simpler patch without AutoWritePrefString class

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8668522 [details] [diff] [review]
Simpler patch without AutoWritePrefString class

>diff --git a/toolkit/modules/Troubleshoot.jsm b/toolkit/modules/Troubleshoot.jsm
>--- a/toolkit/modules/Troubleshoot.jsm
>+++ b/toolkit/modules/Troubleshoot.jsm
>@@ -84,16 +84,17 @@ const PREFS_WHITELIST = [
>   "plugins.",
>   "print.",
>   "privacy.",
>   "security.",
>   "social.enabled",
>   "storage.vacuum.last.",
>   "svg.",
>   "toolkit.startup.recent_crashes",
>+  "ui.osk.",
>   "webgl.",
> ];
> 
> // The blacklist, unlike the whitelist, is a list of regular expressions.
> const PREFS_BLACKLIST = [
>   /^network[.]proxy[.]/,
>   /[.]print_to_filename$/,
>   /^print[.]macosx[.]pagesetup/,

I think that this part should be reviewed by UI team or somebody. Although, I'm not sure who manages about:supports.

>@@ -523,36 +524,46 @@ IMEHandler::SetInputScopeForIMM32(nsWind
>                     nullptr, nullptr);
>   }
> }
> 
> // static
> void
> IMEHandler::MaybeShowOnScreenKeyboard()
> {
>+  std::wstring reason;

I don't understand why you use std::wstring instead of nsAutoCString.

>   if (sPluginHasFocus ||
>       !IsWin10OrLater() ||
>       !Preferences::GetBool(kOskEnabled, true) ||
>       sShowingOnScreenKeyboard ||
>-      IMEHandler::IsKeyboardPresentOnSlate()) {
>+      IMEHandler::IsKeyboardPresentOnSlate(reason)) {
>+    if (reason.length() > 0) {
>+      Preferences::SetString(kOskDebugReason, reason.c_str());
>+    }
>     return;
>   }

I don't like to add |reason| argument for IsKeyboardPresentOnSlate().

> 
>   // Tablet Mode is only supported on Windows 10 and higher.
>   // When touch-event detection within IME is better supported
>   // this check may be removed, and ShowOnScreenKeyboard can
>   // run on Windows 8 and higher (adjusting the IsWin10OrLater
>   // guard above and within MaybeDismissOnScreenKeyboard).
>-  if (!IsInTabletMode() &&
>+  if (!IsInTabletMode(reason) &&

Ditto.

>       Preferences::GetBool(kOskRequireTabletMode, true) &&
>-      !AutoInvokeOnScreenKeyboardInDesktopMode()) {
>+      !AutoInvokeOnScreenKeyboardInDesktopMode(reason)) {
>+    if (reason.length() > 0) {
>+      Preferences::SetString(kOskDebugReason, reason.c_str());
>+    }
>     return;
>   }
> 
>- IMEHandler::ShowOnScreenKeyboard();
>+  if (reason.length() > 0) {
>+    Preferences::SetString(kOskDebugReason, reason.c_str());
>+  }

I don't understand why this is necessary...

> bool
>-IMEHandler::IsKeyboardPresentOnSlate()
>+IMEHandler::IsKeyboardPresentOnSlate(std::wstring& reason)
> {
>   // This function is only supported for Windows 8 and up.
>   if (!IsWin8OrLater()) {
>+    reason.append(L"IKPOS: Requires Win8+. ");

I think that you should log the reason into the pref here.

>     return true;
>   }
> 
>   if (!Preferences::GetBool(kOskDetectPhysicalKeyboard, true)) {
>-    // Detection for physical keyboard has been disabled for testing.
>+    reason.append(L"IKPOS: Detection disabled. ");

Ditto.

>     return false;
>   }
> 
>   // This function should be only invoked for machines with touch screens.
>   if ((::GetSystemMetrics(SM_DIGITIZER) & NID_INTEGRATED_TOUCH)
>         != NID_INTEGRATED_TOUCH) {
>+    reason.append(L"IKPOS: Touch screen not found. ");

Ditto.

>     return true;
>   }
> 
>   // If the device is docked, the user is treating the device as a PC.
>   if (::GetSystemMetrics(SM_SYSTEMDOCKED) != 0) {
>+    reason.append(L"IKPOS: System docked. ");

Ditto.

>   if (get_rotation_state) {
>     AR_STATE auto_rotation_state = AR_ENABLED;
>     get_rotation_state(&auto_rotation_state);
>-    if ((auto_rotation_state & AR_NOSENSOR) ||
>-        (auto_rotation_state & AR_NOT_SUPPORTED)) {
>-      // If there is no auto rotation sensor or rotation is not supported in
>-      // the current configuration, then we can assume that this is a desktop
>-      // or a traditional laptop.
>+    // If there is no auto rotation sensor or rotation is not supported in
>+    // the current configuration, then we can assume that this is a desktop
>+    // or a traditional laptop.
>+    if (auto_rotation_state & AR_NOSENSOR) {
>+      reason.append(L"IKPOS: Rotation sensor not found. ");
>+      return true;
>+    } else if (auto_rotation_state & AR_NOT_SUPPORTED) {
>+      reason.append(L"IKPOS: Auto-rotation not supported. ");

Ditto.

>       return true;
>     }
>   }
> 
>   // 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.
>   typedef POWER_PLATFORM_ROLE (WINAPI* PowerDeterminePlatformRole)();
>   PowerDeterminePlatformRole power_determine_platform_role =
>     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)) &&
>          (::GetSystemMetrics(SM_CONVERTIBLESLATEMODE) == 0)) {
>+      if (role == PlatformRoleMobile) {
>+        reason.append(L"IKPOS: PlatformRoleMobile. ");
>+      } else if (role == PlatformRoleSlate) {
>+        reason.append(L"IKPOS: PlatformRoleSlate. ");

Ditto.


>   // Query for all the keyboard devices.
>   HDEVINFO device_info =
>     ::SetupDiGetClassDevs(&KEYBOARD_CLASS_GUID, nullptr,
>                           nullptr, DIGCF_PRESENT);
>   if (device_info == INVALID_HANDLE_VALUE) {
>+    reason.append(L"IKPOS: No keyboard info. ");

Ditto.

>@@ -691,63 +714,80 @@ IMEHandler::IsKeyboardPresentOnSlate()
>       if (IMEHandler::WStringStartsWithCaseInsensitive(device_id,
>                                                        L"ACPI") ||
>           IMEHandler::WStringStartsWithCaseInsensitive(device_id,
>                                                        L"HID\\VID")) {
>         // The heuristic we are using is to check the count of keyboards and
>         // return true if the API's report one or more keyboards. Please note
>         // that this will break for non keyboard devices which expose a
>         // keyboard PDO.
>+        reason.append(L"IKPOS: Keyboard presence confirmed. ");

Ditto.

>         return true;
>       }
>     }
>   }
>+  reason.append(L"IKPOS: Lack of keyboard confirmed. ");

Ditto.

> bool
>-IMEHandler::IsInTabletMode()
>+IMEHandler::IsInTabletMode(std::wstring& reason)
> {
>   nsCOMPtr<nsIWindowsUIUtils>
>     uiUtils(do_GetService("@mozilla.org/windows-ui-utils;1"));
>   if (NS_WARN_IF(!uiUtils)) {
>+    reason.append(L"IITM: nsIWindowsUIUtils not available. ");

Ditto.

>     return false;
>   }
>   bool isInTabletMode = false;
>   uiUtils->GetInTabletMode(&isInTabletMode);
>+  if (isInTabletMode) {
>+    reason.append(L"IITM: GetInTabletMode=true. ");
>+  } else {
>+    reason.append(L"IITM: GetInTabletMode=false. ");
>+  }

Ditto.

>   return isInTabletMode;
> }
> 
> // static
> bool
>-IMEHandler::AutoInvokeOnScreenKeyboardInDesktopMode()
>+IMEHandler::AutoInvokeOnScreenKeyboardInDesktopMode(std::wstring& reason)
> {
>   nsresult rv;
>   nsCOMPtr<nsIWindowsRegKey> regKey
>     (do_CreateInstance("@mozilla.org/windows-registry-key;1", &rv));
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>+    reason.append(L"AIOSKIDM: "
>+                     L"nsIWindowsRegKey not available");

Ditto.

>     return false;
>   }
>   rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_CURRENT_USER,
>                     NS_LITERAL_STRING("SOFTWARE\\Microsoft\\TabletTip\\1.7"),
>                     nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>   if (NS_FAILED(rv)) {
>+    reason.append(L"AIOSKIDM: failed opening regkey. ");

Ditto.

>     return false;
>   }
>   // EnableDesktopModeAutoInvoke is an opt-in option from the Windows
>   // Settings to "Automatically show the touch keyboard in windowed apps
>   // when there's no keyboard attached to your device." If the user has
>   // opted-in to this behavior, the tablet-mode requirement is skipped.
>   uint32_t value;
>   rv = regKey->ReadIntValue(NS_LITERAL_STRING("EnableDesktopModeAutoInvoke"),
>                             &value);
>   if (NS_FAILED(rv)) {
>+    reason.append(L"AIOSKIDM: failed reading value of regkey. ");
>     return false;
>   }
>+  if (!!value) {
>+    reason.append(L"AIOSKIDM: regkey value=true. ");
>+  } else {
>+    reason.append(L"AIOSKIDM: regkey value=false. ");

Ditto.

Comment 6

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

Attachment: Patch

Thanks for the feedback, I've made the adjustments you requested. I changed the Troubleshoot.jsm part to only display the prefs that we should be interested in (this leaves out the ui.osk.on_screen_keyboard_path pref, which doesn't help much and may include some potentially identifying information).

I looked at other bugs that added information to about:support (bug 114171 and 1151611) and neither of those bugs mentioned needing approval before landing changes to about:support. As none of the prefs that we are adding to about:support including personally identifying information we should be OK to land this.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

>@@ -523,16 +524,17 @@ IMEHandler::SetInputScopeForIMM32(nsWind
>                     nullptr, nullptr);
>   }
> }
> 
> // static
> void
> IMEHandler::MaybeShowOnScreenKeyboard()
> {
>+  std::wstring reason;

I think that you don't need this.

> bool
> IMEHandler::IsKeyboardPresentOnSlate()
> {
>   // This function is only supported for Windows 8 and up.
>   if (!IsWin8OrLater()) {
>+    Preferences::SetString(kOskDebugReason, L"IKPOS: Requires Win8+. ");

I wonder why do you need the last " "? If it's unnecessary, please remove from all calls of Preferences::SetString().

>@@ -627,50 +632,59 @@ IMEHandler::IsKeyboardPresentOnSlate()
>   typedef BOOL (WINAPI* GetAutoRotationState)(PAR_STATE state);
>   GetAutoRotationState get_rotation_state =
>     reinterpret_cast<GetAutoRotationState>(::GetProcAddress(
>       ::GetModuleHandleW(L"user32.dll"), "GetAutoRotationState"));
> 
>   if (get_rotation_state) {
>     AR_STATE auto_rotation_state = AR_ENABLED;
>     get_rotation_state(&auto_rotation_state);
>@@ -691,63 +705,80 @@ IMEHandler::IsKeyboardPresentOnSlate()
>       if (IMEHandler::WStringStartsWithCaseInsensitive(device_id,
>                                                        L"ACPI") ||
>           IMEHandler::WStringStartsWithCaseInsensitive(device_id,
>                                                        L"HID\\VID")) {
>         // The heuristic we are using is to check the count of keyboards and
>         // return true if the API's report one or more keyboards. Please note
>         // that this will break for non keyboard devices which expose a
>         // keyboard PDO.
>+        Preferences::SetString(kOskDebugReason, L"IKPOS: Keyboard presence confirmed. ");

Over 80 characters.

>         return true;
>       }
>     }
>   }
>+  Preferences::SetString(kOskDebugReason, L"IKPOS: Lack of keyboard confirmed. ");

Ditto.
>   return false;
> }
> 
> // static
> bool
> IMEHandler::IsInTabletMode()
> {
>   nsCOMPtr<nsIWindowsUIUtils>
>     uiUtils(do_GetService("@mozilla.org/windows-ui-utils;1"));
>   if (NS_WARN_IF(!uiUtils)) {
>+    Preferences::SetString(kOskDebugReason, L"IITM: nsIWindowsUIUtils not available. ");

Ditto.

>   // EnableDesktopModeAutoInvoke is an opt-in option from the Windows
>   // Settings to "Automatically show the touch keyboard in windowed apps
>   // when there's no keyboard attached to your device." If the user has
>   // opted-in to this behavior, the tablet-mode requirement is skipped.
>   uint32_t value;
>   rv = regKey->ReadIntValue(NS_LITERAL_STRING("EnableDesktopModeAutoInvoke"),
>                             &value);
>   if (NS_FAILED(rv)) {
>+    Preferences::SetString(kOskDebugReason, L"AIOSKIDM: failed reading value of regkey. ");

Ditto.


Please fix the nits. Then r=masayuki only for the IMEHandler.cpp part.

Comment 8

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

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

Felipe, can you review the all.js and Troubleshoot.jsm changes?

Comment 9

[Pulsebot]

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

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/a6abbca3d2e8

Comment 11

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

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

Approval Request Comment
[Feature/regressing bug #]: extra diagnostic info for on-screen keyboard, as it will be enabled by default in 43 and this will help us troubleshoot any issues that may pop up
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 12

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

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

OK to uplift to aurora, as this will help debug any issues that come up in testing win8 touch screen keyboard support for 43.

Comment 13

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

Marking affected for 43. Not tracking this specific bug since I'm tracking bug 1213845 for 43 already to cover this feature.

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bc9b8a09107