-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
change(web): rotation polish - OSK reload no longer needed 🍒 #7188
Conversation
User Test ResultsTest specification and instructions
🟥 SUITE_ROTATION:
Retesting Template
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -59,13 +61,37 @@ namespace com.keyman.osk { | |||
s.position = 'fixed'; | |||
} | |||
|
|||
protected postKeyboardLoad() { | |||
// Initializes the size of a touch keyboard. | |||
public refreshLayout(pending?: boolean): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main difference is that I see we didn't 🍒 the @override
annotation; this is reflected by three less added lines in the lines-changed count.
There's also a dropped newline at line 70 compared to the original - that's the one additional line-change difference of note. The rest seems to match.
GROUP_ANDROID: - any Android device should suffice
|
@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 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. |
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. Noting my prior comment, I'm requesting that the changes from #6979 be included in this 🍒-pick, as that should address the failed unit test.
…/6787-rotation-polish
Cherry-pick of #6979 to stable-15.0
@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. |
@bharanidharanj 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. |
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. |
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: #6979 (comment) It does make me wonder if there may be a bit more to address there as well. |
Unlike before, I'm having trouble attempting to repro the user test failures. That's going to make investigation at least a bit tricky. |
@keymanapp-test-bot retest SUITE_ROTATION GROUP_OLD_IOS TEST_BROWSER_ROTATE_L-TO-P GROUP_ANDROID_PHONE TEST_BROWSER_ROTATE_P-TO-L |
Self assigned to retest and double check the failed cases |
Test ResultsSUITE_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 |
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. |
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? |
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! |
One fail on the old version of iOS, modern iOS versions seem to work fine. |
Changes in this pull request will be available for download in Keyman version 15.0.271 |
🍒 pick of #6787 and #6979 and fixes #7169 to stable-15.0 (16.0 master already contains the fix)
User Testing
Test environments
Tests
TEST_INAPP_ROTATE_P-TO-L:
TEST_INAPP_ROTATE_L-TO-P:
TEST_BROWSER_ROTATE_P-TO-L:
TEST_BROWSER_ROTATE_L-TO-P:
TEST_ANDROID_SHIFT_AFTER_GLOBE: Verifies fix to bug(android): At start of input, pressing globe key, going back, leads to stuck shift key #7169
(edit) Additional user tests from #6979
SUITE_ROTATION
Test environments
Tests
TEST_OSK_PORTRAIT_LOAD:
TEST_OSK_LANDSCAPE_LOAD:
TEST_BROWSER_ROTATE_P-TO-L:
TEST_BROWSER_ROTATE_L-TO-P: