Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(mac): make modifiers operational in OSK #12556

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

sgschantz
Copy link
Contributor

@sgschantz sgschantz commented Oct 17, 2024

With different layers active in the OSK, clicking on a key should produce the character represented on the key to be output.

Fixes #6578

User Testing

SUITE_OSK_MODIFIERS: test varying outputs for different modifiers

  • GROUP_VENTURA: test with macOS Ventura

  • GROUP_SONOMA: test with macOS Sonoma

  • TEST_OSK_BASE_LAYER: Khmer Angkor uses only RALT rules

  1. Install this version of Keyman and switch to the Khmer Angkor keyboard
  2. Open the OSK
  3. Click on the 'S' key
  4. Confirm that this generates the character
  • TEST_OSK_SHIFT_LAYER: Khmer Angkor uses only RALT rules
  1. Open the OSK
  2. Make sure that the text insertion point is immediately to the right of the previously typed
  3. Click on the Shift key in the OSK (designated with the up arrow) so that the Shift layer is displayed
  4. Click on the 'S' key
  5. Confirm that the previously generated character 'ស' now has a vowel inserted to the left, resulting in សៃ
  • TEST_OSK_ALT_LAYER: Khmer Angkor uses only RALT rules
  1. Install this version of Keyman and switch to the Khmer Angkor keyboard
  2. Open the OSK
  3. Click on the Alt key in the OSK so that the Alt layer is displayed
  4. Click on the 'S' key
  5. Confirm that this generates the character -
  • TEST_OSK_ALT_SHIFT_LAYER: Khmer Angkor uses only RALT rules
  1. Install this version of Keyman and switch to the Khmer Angkor keyboard
  2. Open the OSK
  3. Click on the Alt key in the OSK so that the Alt layer is displayed
  4. Additionally click on the Shift key in the OSK so that the Alt-Shift layer is displayed
  5. Click on the 'S' key
  6. Confirm that this generates the character

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Oct 17, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Oct 17, 2024

User Test Results

Test specification and instructions

✅ SUITE_OSK_MODIFIERS: test varying outputs for different modifiers

8 tests in 2 groups PASSED
  • ✅ GROUP_VENTURA: test with macOS Ventura

    4 tests PASSED
  • ✅ GROUP_SONOMA: test with macOS Sonoma

    4 tests PASSED

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S13 milestone Oct 17, 2024
@sgschantz sgschantz self-assigned this Oct 17, 2024
@sgschantz sgschantz marked this pull request as ready for review October 17, 2024 09:54
@sgschantz sgschantz requested a review from SabineSIL as a code owner October 17, 2024 09:54
@dinakaranr
Copy link

Test Results

I tested this issue with the attached "Keyman 18.0.126-alpha-local" build on the macOS Sonoma 14.5 and Big Sur 11.0.1. I am sharing my observation.

SUITE_OSK_MODIFIERS: test varying outputs for different modifiers

GROUP_VENTURA: test with macOS Ventura

Installed "Keyman 18.0.126-alpha-local" build on the macOS Big Sur 11.0.1. I had confirmation from the developer to test this PR in the Big Sur because I did not have the macOS Ventura. Installed the Khmer Angkor keyboard

  • TEST_OSK_BASE_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Open the Notes app
  4. Click on the 'S' key in the OSK
  5. Verified that this generates the character ស
  • TEST_OSK_SHIFT_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Open the Notes app
  4. Click on the 'S' key in the OSK.
  5. Click on the Shift key in the OSK (designated with the up arrow) so that the Shift layer is displayed
  6. Click on the 'S' key
  7. Verified that the step 4 generated character 'ស' now has a vowel inserted to the left. Now the resulting in សៃ
  • TEST_OSK_ALT_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Open the Notes app
  4. Click on the Alt key in the OSK so that the Alt layer is displayed
  5. Click on the 'S' key in the OSK
  6. Verified that this generates the character -
  • TEST_OSK_ALT_SHIFT_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Click on the Alt key in the OSK so that the Alt layer is displayed
  4. Additionally click on the Shift key in the OSK so that the Alt-Shift layer is displayed
  5. Click on the 'S' key
  6. Verified that this generates the character ᧭

GROUP_SONOMA: test with macOS Sonoma

Installed "Keyman 18.0.126-alpha-local" build on the macOS Sonoma 14.5. Installed the Khmer Angkor keyboard.

  • TEST_OSK_BASE_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Open the Notes app
  4. Click on the 'S' key in the OSK
  5. Verified that this generates the character ស
  • TEST_OSK_SHIFT_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Open the Notes app
  4. Click on the 'S' key in the OSK.
  5. Click on the Shift key in the OSK (designated with the up arrow) so that the Shift layer is displayed
  6. Click on the 'S' key
  7. Verified that the step 4 generated character 'ស' now has a vowel inserted to the left. Now the resulting in សៃ
  • TEST_OSK_ALT_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Open the Notes app
  4. Click on the Alt key in the OSK so that the Alt layer is displayed
  5. Click on the 'S' key in the OSK
  6. Verified that this generates the character -
  • TEST_OSK_ALT_SHIFT_LAYER (Passed):
  1. Select the Khmer Angkor keyboard
  2. Open the OSK
  3. Click on the Alt key in the OSK so that the Alt layer is displayed
  4. Additionally, click on the Shift key in the OSK so that the Alt-Shift layer is displayed
  5. Click on the 'S' key
  6. Verified that this generates the character ᧭

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Oct 17, 2024
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good.

I had a test of the OSK on my mac, and a few things I think should happen (some of which may be addressed in #12565?):

  1. Pressing any modifier key on the hardware keyboard should reset all the OSK-selected modifier keys. This is the behaviour we settled on in Keyman for Windows, and it seems to be the most reliable option. (Testing right now, I am not sure if that is happening on Windows?)
  2. Closing the OSK should reset the modifier state to default
  3. The modifier state on the OSK should affect the hardware keys typed. Right now they seem completely independent.

@@ -17,6 +17,9 @@
#include <Carbon/Carbon.h>
#import "KMELogs.h"

const int64_t OSK_EVENT_FLAG = 0x8000000000000000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was this flag selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am copying the modifier flags into the lower 32 bits and saving the upper 32 bits to mark this as our custom user data. Since it defaults to zero otherwise, I am just setting the upper-most bit of the upper 32, and I can use the other 31 bits in the future if I need to pass any other info from the OSK through to the event tap.

@mcdurdin
Copy link
Member

One more, holding Option+Shift on the hardware keyboard shows either Alt or Shift layer, not Alt+Shift layer (e.g. test on Khmer Angkor). It seems to work correctly when clicking the keys on the OSK

@sgschantz
Copy link
Contributor Author

Code changes look good.

I had a test of the OSK on my mac, and a few things I think should happen (some of which may be addressed in #12565?):

  1. Pressing any modifier key on the hardware keyboard should reset all the OSK-selected modifier keys. This is the behaviour we settled on in Keyman for Windows, and it seems to be the most reliable option. (Testing right now, I am not sure if that is happening on Windows?)
  2. Closing the OSK should reset the modifier state to default
  3. The modifier state on the OSK should affect the hardware keys typed. Right now they seem completely independent.

I think we should document these and your other suggestion either in an issue or, probably better, as part of a Monday design meeting -- especially if it should be consistent across platforms. I didn't really try to design for these scenarios as I was just focusing on the one issue, and the correct interaction between the OSK and the physical keyboard is not intuitive to me. If we discuss in a group, we may come with some other edge cases that you haven't covered yet.

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

OK, let's follow up on the other issues separately. Will you track them?

@sgschantz
Copy link
Contributor Author

LGTM

OK, let's follow up on the other issues separately. Will you track them?

created #12582 and #12584 to track these

@sgschantz sgschantz merged commit 60d6753 into master Oct 25, 2024
6 checks passed
@sgschantz sgschantz deleted the fix/mac/6578-ineffective-osk-modifiers branch October 25, 2024 08:58
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.131-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug(mac): OSK modifiers not functioning
4 participants