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.

Sign up for GitHub

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

Jump to bottom

fix(developer): don't use osk-always-visible on touch devices #9917

Merged
merged 1 commit into from
Nov 3, 2023
Merged

fix(developer): don't use osk-always-visible on touch devices #9917

merged 1 commit into from
Nov 3, 2023

Conversation

mcdurdin
Copy link
Member

Fixes #9845.

It seems that the logic for osk-always-visible is not quite right on touch devices -- the OSK disappears on blur but remains touchable -- so presses in the OSK region emit key events. For Keyman Developer Server, the simple workaround is to only use <body class="osk-always-visible"> when on desktop devices.

We should review the logic for osk-always-visible in KeymanWeb, so that this issue does not arise on touch devices. This patch addresses the issue in Keyman Developer Server, and matches the behaviour we want on touch devices in any case, as we don't really want the OSK visible when blurred, unlike on desktop.

User Testing

  • TEST_INVISIBLE_OSK_ON_TOUCH_DEVICE: Open Keyman Developer Server on a touch device (or in touch device emulation in Chrome); ensure there is a Keyman keyboard active for testing. Touch on the touch OSK to input some text into the test box. Then touch outside the text box to hide the OSK. Then, touch where the OSK was visible. This should no longer cause text to be inserted into the test box.

@mcdurdin
fix(developer): don't use osk-always-visible on touch devices
4ce16a0 
Fixes #9845.

It seems that the logic for `osk-always-visible` is not quite right on
touch devices -- the OSK disappears on blur but remains touchable -- so
presses in the OSK region emit key events. For Keyman Developer Server,
the simple workaround is to only use `<body class="osk-always-visible">`
when on desktop devices.

We should review the logic for `osk-always-visible` in KeymanWeb, so
that this issue does not arise on touch devices. This patch addresses
the issue in Keyman Developer Server, and matches the behaviour we want
on touch devices in any case, as we don't really want the OSK visible
when blurred, unlike on desktop.
@mcdurdin mcdurdin requested a review from darcywong00 as a code owner October 31, 2023 23:05
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Oct 31, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Oct 31, 2023 •
edited
Loading

User Test Results

Test specification and instructions

  • ✅ TEST_INVISIBLE_OSK_ON_TOUCH_DEVICE (PASSED): Retested as per your instructions and here is my observation: 1. Selected a keyboard from the Keyboard list. (eg., Khmer_Angkor). 2. Typed some text using the OSK. 3. Clicked outside of the textbox. 4. Then, I clicked a letter key from the OSK. Verified that no letter appears on the text box. (notes)

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S25 milestone Oct 31, 2023
@mcdurdin mcdurdin mentioned this pull request Oct 31, 2023
Closed
rc-swag
rc-swag approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@rc-swag rc-swag left a comment

Choose a reason for hiding this comment

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

lgtm

@bharanidharanj
Copy link

Test Results

  • TEST_INVISIBLE_OSK_ON_TOUCH_DEVICE (FAILED): Tested with Keyman Developer 17.0.204-alpha-test-9917 on Windows 11 OS (VM) and here is what I did: 1. Opened the Keyman Developer Server in a touch device emulation in Chrome browser. 2. Select Khmer_Angkor keyboard from the Keyboard list box. Able to see the OSK appear on the screen. 3. Typed some letters using the OSK. Then, touched outside of the the text box. But the OSK is still visible on the screen. 4. Again, typed some texts using OSK. The typed text appeared in the text box. Seems to be an issue.

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Nov 2, 2023
@mcdurdin
Copy link
Member Author

mcdurdin commented Nov 2, 2023

@bharanidharanj can you retest please? I don't think the mobile emulation mode took effect. For mobile emulation in Chrome, you need to press F12 to activate the Developer Tools, then click the
image button to activate mobile mode. In the mobile toolbar, then select iPhone or similar.

Then, press F5 to reload the page so mobile mode takes full effect.

And then you can do the test 😁

@keymanapp-test-bot retest

@keymanapp-test-bot keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels Nov 2, 2023
@bharanidharanj
Copy link

Test Results

  • TEST_INVISIBLE_OSK_ON_TOUCH_DEVICE (PASSED): Retested as per your instructions and here is my observation: 1. Selected a keyboard from the Keyboard list. (eg., Khmer_Angkor). 2. Typed some text using the OSK. 3. Clicked outside of the textbox. 4. Then, I clicked a letter key from the OSK. Verified that no letter appears on the text box.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 2, 2023
@mcdurdin
Copy link
Member Author

mcdurdin commented Nov 3, 2023

@bharanidharanj I think you might have missed the step on using mobile emulation mode. But I just retested that here so I think we are good to go.

@mcdurdin mcdurdin merged commit cd5be3d into master Nov 3, 2023
3 checks passed
@mcdurdin mcdurdin deleted the fix/developer/9845-server-no-osk-always-visible-on-touch branch November 3, 2023 01:22
@keyman-server
Copy link
Collaborator

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

@ermshiperete ermshiperete mentioned this pull request Nov 7, 2023
Merged
@bharanidharanj
Copy link

@bharanidharanj I think you might have missed the step on using mobile emulation mode. But I just retested that here so I think we are good to go.

@mcdurdin Oops! I have done this test on the Mozilla Firefox browser. However, I retested this in Chrome browser and it seems to be working as expected. Thanks for your guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rc-swag rc-swag rc-swag approved these changes

@darcywong00 darcywong00 Awaiting requested review from darcywong00 darcywong00 is a code owner

Assignees
No one assigned
Labels
developer/ide/ developer/ fix has-user-test
Projects
None yet
Milestone
A17S25
Development

Successfully merging this pull request may close these issues.

bug(developer): server-trying to select a keyboard down where keyboard is shown fails
4 participants
@mcdurdin @bharanidharanj @keyman-server @rc-swag