search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

change(web): rotation polish - OSK reload no longer needed 🍒 #7188

Closed darcywong00 closed 1 year ago

darcywong00 commented 1 year ago

:cherries: pick of #6787 and #6979 and fixes #7169 to stable-15.0 (16.0 master already contains the fix)

User Testing

Test environments

Tests

(edit) Additional user tests from #6979

SUITE_ROTATION

Test environments

Tests

keymanapp-test-bot[bot] commented 1 year ago

User Test Results

Test specification and instructions

🟥 SUITE_ROTATION:

Retesting Template ``` @keymanapp-test-bot retest SUITE_ROTATION GROUP_OLD_IOS TEST_BROWSER_ROTATE_L-TO-P ```
bharanidharanj commented 1 year ago

GROUP_IOS: - any iOS device should suffice

bharanidharanj commented 1 year ago

GROUP_ANDROID: - any Android device should suffice

mcdurdin commented 1 year ago
  • TEST_BROWSER_ROTATE_L-TO-P (FAILED): Tested this with the attach PR build (15.0.270-test-7188) in iOS 15.5 / iPhone 13 Pro max simulator and the OSK appears partially on the Screen.

@jahorton and @darcywong00 can you review this issue?

jahorton commented 1 year ago
  • TEST_BROWSER_ROTATE_L-TO-P (FAILED): Tested this with the attach PR build (15.0.270-test-7188) in iOS 15.5 / iPhone 13 Pro max simulator and the OSK appears partially on the Screen.

@jahorton and @darcywong00 can you review this issue?

I am able to reproduce the test failure on my personal iPhone, though not in Chrome emulation.

For comparison, and to ensure that it isn't broken on master, I double-checked on the same device using the test build product from #7142, which includes the already-merged 16.0-alpha version of these changes. (I double-checked that aspect - that the changes are there - as well.)

This test result does not reproduce on 16.0-alpha; somehow, there's... something different that's specific to 15.0-stable. What "something"? Good question! That'll take time to investigate.

The most obvious thing to note for investigation: the 16.0-alpha version landed in 16.0.43. Since the tests were passed then, chances are that the divergence is due to a change on 16.0-alpha before that point. Such a change would (naturally) have been missed by this cherrypick since we didn't know it to be related.

jahorton commented 1 year ago

Well, that's probably related: we should probably cherry-pick the "patch PR" followup as part of this, too. See #6979.

The original version had similarly-noted test issues, but since it'd already undergone significant review at the time, I made a "patch PR" that could be reviewed separately, in case reviewers didn't like the "patch" changes. It was fine and got included, but there's something noteworthy about how it got merged:

image

darcywong00 commented 1 year ago

With the changes from #6979 now :cherries:-picked,

@keymanapp-test-bot retest GROUP_IOS TEST_BROWSER_ROTATE_L-TO-P

bharanidharanj commented 1 year ago

GROUP_IOS: - any iOS device should suffice

SUITE_ROTATION:

GROUP_IOS_PHONE: - any modern iOS phone should suffice

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_IOS_TABLET: - any modern iOS tablet should suffice

bharanidharanj commented 1 year ago

GROUP_IOS: - any iOS device should suffice

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_OLD_IOS: - try to find an iOS 12 or iOS 13 device to test with

darcywong00 commented 1 year ago

@bharanidharanj TEST_BROWSER_ROTATE_L-TO-P should have rotated from landscape to portrait orientation, and the screenshot looks like it's still landscape. Are you saying the OSK failed to display in the original landscape orientation?

See the screenshot from the original PR

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_PHONE: - any modern Android phone should suffice

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_TABLET: - any modern Android tablet should suffice

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_7: - run tests against early Android if possible (Android 7.0)

bharanidharanj commented 1 year ago

@bharanidharanj TEST_BROWSER_ROTATE_L-TO-P should have rotated from landscape to portrait orientation, and the screenshot looks like it's still landscape. Are you saying the OSK failed to display in the original landscape orientation?

See the screenshot from the original PR

@darcywong00 The attached Screenshot looks like it's in Portrait orientation. (from Landscape). Actually, I am confused which emulator you are asking about.

When I tested 15.0.270-test-7188 in iOS 15.5 / iPhone 13 Pro Max Simulator, after rotating the device from Landscape to Portrait, the OSK appears partially on the Screen. (Please, see my attached Screenshot) screenshot

When I tested the same (TEST_BROWSER_ROTATE_P-TO-L) in iOS 12.4 / iPhone 7 Simulator, the OSK is failed to display in the original landscape orientation. (Please, see the attached Screenshot) screenshot

Please, let me know if further clarification is needed. Thanks.

darcywong00 commented 1 year ago

@bharanidharanj I was asking about your test result from here:

  • TEST_BROWSER_ROTATE_L-TO-P (FAILED): Tested this with the attach PR build (15.0.271-test-7188) in iOS 12.4 / iPhone 7 Simulator and I noticed that the OSK flashes on the screen and it does not appear on the View.

Are you saying the OSK flashed and disappeared before rotating from landscape to portrait orientation?

@jahorton - is the browser OSK expected to intermittently disappear?

bharanidharanj commented 1 year ago

@bharanidharanj I was asking about your test result from here:

  • TEST_BROWSER_ROTATE_L-TO-P (FAILED): Tested this with the attach PR build (15.0.271-test-7188) in iOS 12.4 / iPhone 7 Simulator and I noticed that the OSK flashes on the screen and it does not appear on the View.

Are you saying the OSK flashed and disappeared before rotating from landscape to portrait orientation?

@jahorton - is the browser OSK expected to intermittently disappear?

@darcywong00 Yes, That's right.

darcywong00 commented 1 year ago

From Friday standup, @mcdurdin said we'll hold off on this PR till @jahorton can investigate the browser OSK rotation issues.

jahorton commented 1 year ago

@jahorton - is the browser OSK expected to intermittently disappear?

I'll need to revisit it to be sure, but my memory is that KMW will usually hide the OSK pre-rotation and re-display it afterward. So, this suggests that the 'redisplay' part isn't happening for whatever reason.

jahorton commented 1 year ago

While certainly not an answer to the lingering issues from user testing mentioned above, there were a few oddities noted here on one of the original PRs: https://github.com/keymanapp/keyman/pull/6979#issuecomment-1203688781

It does make me wonder if there may be a bit more to address there as well.

jahorton commented 1 year ago

Unlike before, I'm having trouble attempting to repro the user test failures. That's going to make investigation at least a bit tricky.

MakaraSok commented 1 year ago

@keymanapp-test-bot retest SUITE_ROTATION GROUP_OLD_IOS TEST_BROWSER_ROTATE_L-TO-P GROUP_ANDROID_PHONE TEST_BROWSER_ROTATE_P-TO-L

MakaraSok commented 1 year ago

Self assigned to retest and double check the failed cases

MakaraSok commented 1 year ago

Test Results

SUITE_ROTATION:

GROUP_OLD_IOS: - try to find an iOS 12 or iOS 13 device to test with

If this is out of the scope of this test case, please ignore the this result.

GROUP_ANDROID_PHONE: - any modern Android phone should suffice

jahorton commented 1 year ago

Test Results

SUITE_ROTATION:

GROUP_OLD_IOS: - try to find an iOS 12 or iOS 13 device to test with

* **TEST_BROWSER_ROTATE_L-TO-P (FAILED):** Tested with the [build off of this PR](https://build.palaso.org/repository/download/Keymanweb_TestPullRequests/339125:id/index.html):

  * on iOS 12.4 iPhone Xs Max simulator. There is no way to open the language menu as the globe key is nowhere to be found.

  <img alt="image" width="500" src="https://user-images.githubusercontent.com/28331388/192683244-910cbb16-41b6-40f6-9eab-26734beb8cfe.png">

I may need to double-check, but I think this is the same Safari issue as noted on one of the other tests previously. Older iOS devices put the nav bar at the top rather than the bottom, but the in-page side effects were the same. If the nav bar hadn't appeared, the globe key would likely be in view.

jahorton commented 1 year ago

Come to think on it... if the 'worst' user test failure we're getting is that the OSK isn't showing after a rotation... as long as it shows again when the element is reselected, that doesn't seem too bad of an issue. Also, even that is only happening intermittently at that, so far as we've seen, right?

bharanidharanj commented 1 year ago

As per Makara's suggestion I have retested this (GROUP_OLD_IOS: - try to find an iOS 12 or iOS 13 device to test with- TEST_BROWSER_ROTATE_L-TO-P )in iOS 12.4 and here is my observation:

  1. Tested the Landscape to Portrait mode in iPhone 7 Plus Simulator and I was able to see the OSK on the Screen in the landscape mode. However, the bottom of the OSK has been cropped.

  2. In iPhone X Simulator after clicking the text area, the globe key is not visible on the Screen.

  3. In iPhone xs Max Simulator, after scrolling down the page I could able to see the globe key. However, scrolling up the page will hide the globe key on the screen.

https://user-images.githubusercontent.com/19683143/194248997-3a4ad5b3-ad2f-4239-9540-cd2f613ef109.mov

  1. Tested the same in iOS 16.0, the OSK is fully visible on the Screen. Seems to be working fine in the latest iOS verion.

mcdurdin commented 1 year ago

My feeling on this, is if the issue is less severe than previously, we should merge this. In particular, if this rotation issue only impacts Keyman Web on mobile, and only impacts rotation, and is easily resolved by the user by refocusing the edit control, that's a much smaller user base, with a much less severe impact, so we should merge.

Then we should open a new issue to correct the remaining rotation issues!

mcdurdin commented 1 year ago

One fail on the old version of iOS, modern iOS versions seem to work fine.

keyman-server commented 1 year ago

Changes in this pull request will be available for download in Keyman version 15.0.271