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): updates mobile screen dimension detection code ⬅️ #6979

Closed jahorton closed 1 year ago

jahorton commented 1 year ago

Based on #6787, as its user test issues motivated development of this PR.

So, it turns out there there are some very strange issues with how window.innerWidth updates on mobile devices, and these issues were behind the failed user tests. It appears that the best approach for detecting screen width (what the mobile-form OSK uses) is actually document.documentElement.clientWidth. (Minor reference: https://stackoverflow.com/a/54812656)

Relying upon that, along with some other shifts to how the mobile-form OSK computes its preferred height, appear to stabilize the base PR's issues. That said, these are some notable shifts to how the calculations are done, and there may be noteworthy history behind why things were the way that they were, so I expect some scrunity in the review & in the user-test suite.

Alternatively, adding a 50ms timeout halfway during the rotation adjustments seems to be enough to remedy the issue. There's some very odd behavior that I noticed when stepping through this function via debugger:

https://github.com/keymanapp/keyman/blob/16643d7479b2dc751b4e8ccaa3f5627bc2570bda/web/source/kmwrotation.ts#L44-L67

Before keyman.alignInputs() is called, window.innerWidth is consistently "wrong" - it's notably larger than the value we'd expect it to hold. After it's called, when stepping through, window.innerWidth changes. Unfortunately, that appears to be asynchronous, and a very short timeout isn't enough to alleviate the issue. And there's nothing in that function that should directly affect the browser's management of window.innerWidth.

So, that "halfway during" note seen above would involve setting a 50ms timeout to run lines 47-60. Possible to do, but that feels more "hacky" than the approach I decided to run with. That may just be my perception, though.

By the way, setting the rotation "resolve" timer to a longer period (i.e. further delaying the whole function shown above) does not remedy the issue. Somehow, it's about that line - keyman.alignInputs - so far as I can tell.

User Testing

SUITE_ROTATION

Test environments

Tests

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

User Test Results

Test specification and instructions

✅ SUITE_ROTATION:

24 tests in 6 groups PASSED * ✅ GROUP_IOS_PHONE: - any modern iOS phone should suffice
4 tests PASSED - ✅ **TEST_OSK_PORTRAIT_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629))**: Tested this as per the instructions in iOS 15.5 / iPhone 13 Pro Max Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629)) - ✅ **TEST_OSK_LANDSCAPE_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629))**: Tested this as per the instructions in iOS 15.5 / iPhone 13 Pro Max Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629)) - ✅ **TEST_BROWSER_ROTATE_P-TO-L ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629))**: Tested this as per the instructions in iOS 15.5 / iPhone 13 Pro Max Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629)) - ✅ **TEST_BROWSER_ROTATE_L-TO-P ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629))**: Tested this as per the instructions in iOS 15.5 / iPhone 13 Pro Max Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201288629))
* ✅ GROUP_IOS_TABLET: - any modern iOS tablet should suffice
4 tests PASSED - ✅ **TEST_OSK_PORTRAIT_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653))**: Tested this as per instructions in iOS 15.5 / iPad Pro (9.7 inch) Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653)) - ✅ **TEST_OSK_LANDSCAPE_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653))**: Tested this as per instructions in iOS 15.5 / iPad Pro (9.7 inch) Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653)) - ✅ **TEST_BROWSER_ROTATE_P-TO-L ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653))**: Tested this as per instructions in iOS 15.5 / iPad Pro (9.7 inch) Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653)) - ✅ **TEST_BROWSER_ROTATE_L-TO-P ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653))**: Tested this as per instructions in iOS 15.5 / iPad Pro (9.7 inch) Simulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202168653))
* ✅ GROUP_OLD_IOS: - try to find an iOS 12 or iOS 13 device to test with
4 tests PASSED - ✅ **TEST_OSK_PORTRAIT_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876))**: Tested this as per the instructions in iOS 13.7 / iPhone 11 Pro Max and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876)) - ✅ **TEST_OSK_LANDSCAPE_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876))**: TTested this as per the instructions in iOS 13.7 / iPhone 11 Pro Max and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876)) - ✅ **TEST_BROWSER_ROTATE_P-TO-L ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876))**: Tested this as per the instructions in iOS 13.7 / iPhone 11 Pro Max and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876)) - ✅ **TEST_BROWSER_ROTATE_L-TO-P ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876))**: Tested this as per the instructions in iOS 13.7 / iPhone 11 Pro Max and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202266876))
* ✅ GROUP_ANDROID_PHONE: - any modern Android phone should suffice
4 tests PASSED - ✅ **TEST_OSK_PORTRAIT_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476))**: Tested this as per the instructions in my Android Mobile device / Version 11.0 and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476)) - ✅ **TEST_OSK_LANDSCAPE_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476))**: Tested this as per the instructions in my Android Mobile device / Version 11.0 and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476)) - ✅ **TEST_BROWSER_ROTATE_P-TO-L ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476))**: Tested this as per the instructions in my Android Mobile device / Version 11.0 and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476)) - ✅ **TEST_BROWSER_ROTATE_L-TO-P ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476))**: Tested this as per the instructions in my Android Mobile device / Version 11.0 and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1202312476))
* ✅ GROUP_ANDROID_TABLET: - any modern Android tablet should suffice
4 tests PASSED - ✅ **TEST_OSK_PORTRAIT_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201119735))**: Tested this in API 30 / 10.1 WXGA (Tablet) emualtor (Android 11.0) and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201119735)) - ✅ **TEST_OSK_LANDSCAPE_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201119735))**: Tested this in API 30 / 10.1 WXGA (Tablet) emualtor (Android 11.0) and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201119735)) - ✅ **TEST_BROWSER_ROTATE_P-TO-L ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1203756770))**: As per Makara's comments it seems that this test case is passed in the actual device. - ✅ **TEST_BROWSER_ROTATE_L-TO-P ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1203756770))**: As per Makara's comments it seems that this test case is passed in the actual device.
* ✅ GROUP_ANDROID_7: - run tests against early Android if possible (Android 7.0)
4 tests PASSED - ✅ **TEST_OSK_PORTRAIT_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730))**: Tested this as per the instructions in API 24 / Android 7.0 emulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730)) - ✅ **TEST_OSK_LANDSCAPE_LOAD ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730))**: Tested this as per the instructions in API 24 / Android 7.0 emulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730)) - ✅ **TEST_BROWSER_ROTATE_P-TO-L ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730))**: Tested this as per the instructions in API 24 / Android 7.0 emulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730)) - ✅ **TEST_BROWSER_ROTATE_L-TO-P ([PASSED](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730))**: Tested this as per the instructions in API 24 / Android 7.0 emulator and it is working as expected. ([notes](https://github.com/keymanapp/keyman/pull/6979#issuecomment-1201221730))

Test Artifacts

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_PHONE: - any modern Android phone should suffice

mcdurdin commented 1 year ago

Somehow, it's about that line - keyman.alignInputs - so far as I can tell.

So it sounds like window.innerWidth is internally invalidated, but refresh of the value is delayed. My suspicion is that the code in keyman.alignInputs is forcing a DOM layout or reflow, which internally also ends up validating window.innerWidth.

If this is the case, then when alignInputs goes away with #3030, we'll need to re-fix this. We should be able to do that by forcing a reflow using one of the triggers in the aforementioned link.

(Technically, this could be classed as a bug in Chrome I guess -- reading window.innerWidth should force a layout reflow post-rotation? Building a minimal repro might be painful though.)

BTW, that link above is a hugely worthwhile resource to wrap your brain around -- in terms of performance of the OSK in web in particular. Some of the notes may be slightly out of date but mostly very solid as far as I can tell.

mcdurdin commented 1 year ago

change(web): updates mobile screen-dim detection code ⬅️

I didn't understand this PR title for a long time... what does this have to with screen dimming?

I find it helpful when approaching a new PR to have a short summary at the start of the initial comment stating what the purpose of the PR is. Just one or two sentences is good, introducing the purpose and approach. Something along the lines of "change the screen dimension calculations to use x.y.z instead of a.b.c, because a.b.c appears to be unreliable."

jahorton commented 1 year ago

Somehow, it's about that line - keyman.alignInputs - so far as I can tell.

So it sounds like window.innerWidth is internally invalidated, but refresh of the value is delayed. My suspicion is that the code in keyman.alignInputs is forcing a DOM layout or reflow, which internally also ends up validating window.innerWidth.

It most likely is; in my memory, alignInputs checks for the current layout properties of the base elements for any touch-aliased elements KMW has produced and "re-aligns" those aliases to properly cover their bases in case the elements shifted. That'd definitely trigger some DOM layout calculations.

If this is the case, then when alignInputs goes away with #3030, we'll need to re-fix this. We should be able to do that by forcing a reflow using one of the triggers in the aforementioned link.

(Technically, this could be classed as a bug in Chrome I guess -- reading window.innerWidth should force a layout reflow post-rotation? Building a minimal repro might be painful though.)

And Safari. The same user test failed for both Android (Chrome) and iOS (Safari). It's also mentioned as one of the reflow triggers in that document. So, technically, aren't we already triggering layout reflow with that method?

mcdurdin commented 1 year ago

So, technically, aren't we already triggering layout reflow with that method?

Yes. The async part is the problem I guess :grin:

mcdurdin commented 1 year ago

What about this: https://stackoverflow.com/a/49383279/1836776?

mcdurdin commented 1 year ago

orientationchange is apparently deprecated: https://developer.mozilla.org/en-US/docs/Web/API/Window/orientationchange_event

See also:

jahorton commented 1 year ago

orientationchange is apparently deprecated: https://developer.mozilla.org/en-US/docs/Web/API/Window/orientationchange_event

See also:

* https://developer.mozilla.org/en-US/docs/Web/API/ScreenOrientation

* https://bugs.webkit.org/show_bug.cgi?id=170595

* https://github.com/gajus/orientationchangeend

And the suggested replacement... isn't even implemented on Safari yet. I'll take the one that's fully cross-platform.

mcdurdin commented 1 year ago

And the suggested replacement... isn't even implemented on Safari yet. I'll take the one that's fully cross-platform.

First draft of that spec was published 2012-05-22. Only 10 years ago. Given this is working around what is effectively a bug, we may have better results with multiple solutions to the issue. I'm not opposed to a big comment saying "do this for Safari which doesn't have the Orientation API" -- and using feature detection to get us an answer.

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_IOS_PHONE: - any modern iOS phone should suffice

GROUP_IOS_TABLET: - any modern iOS tablet 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

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_5: - run tests against Android API 21 if possible (Android 5.0)

jahorton commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_5: - run tests against Android API 21 if possible (Android 5.0)

Huh. that's odd. Haven't we run tests against the TeamCity-hosted Web test page artifacts before on emulated Android 5.0? I wonder why it's giving us errors this time; it's clearly a TC thing (that, or an emulated-Android thing) not due to this PR's changes.

jahorton commented 1 year ago

What about this: https://stackoverflow.com/a/49383279/1836776?

We already do that.

https://github.com/keymanapp/keyman/blob/16a63d84ea2c23aebc327b0b486cf34356245cc5/web/source/kmwrotation.ts#L91-L125

This block also happens to be where we'd set the potential screenOrientation.onchange event handler. I guess it'd come first in the Android-block's if-else chain? I mean, I can add it if we care that strongly about it at the moment.

A bit of an aside, but...

change(web): updates mobile screen-dim detection code ⬅️

I didn't understand this PR title for a long time... what does this have to with screen dimming?

So, that angle never even occurred to me. My mind went "screen dimension" - eh, just shorten it, it'll be obvious. It took me a moment or two to figure out where the confusion came from!

MakaraSok commented 1 year ago

Test Results

SUITE_ROTATION:

Tested on Samsung Galaxy Tab A7 running Android 12, One UI Core 4.1.

GROUP_ANDROID_TABLET: - any modern Android tablet should suffice

MakaraSok commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_5: - run tests against Android API 21 if possible (Android 5.0)

TEST_OSK_PORTRAIT_LOAD (BLOCKED): I also experience this same thing.

image
mcdurdin commented 1 year ago

Re blocked tests @bharanidharanj, can we try against Android 6 or 7? (I am investigating options for supporting Android 5, but it looks like it is too old to work with new websites)

jahorton commented 1 year ago

Re blocked tests @bharanidharanj, can we try against Android 6 or 7? (I am investigating options for supporting Android 5, but it looks like it is too old to work with new websites)

That does sound like the best approach we can take for the moment with our current testing setup.

bharanidharanj commented 1 year ago

Re blocked tests @bharanidharanj, can we try against Android 6 or 7? (I am investigating options for supporting Android 5, but it looks like it is too old to work with new websites)

@mcdurdin @jahorton That's sounds good. I will try it in Android 6 emulator and will post my comment. Thanks.

bharanidharanj commented 1 year ago

@mcdurdin @jahorton I am getting the same issue in Android 6.0 emulator. But, it is working fine in Android 7.0 emulator.

So, Shall we use Android 7.0 emulator for future testing?

mcdurdin commented 1 year ago

@mcdurdin @jahorton I am getting the same issue in Android 6.0 emulator. But, it is working fine in Android 7.0 emulator.

So, Shall we use Android 7.0 emulator for future testing?

Here's what I think we should do:

  1. For KeymanWeb testing, we test on Android 7.0 as our minimum.
  2. For Keyman for Android testing, we continue to test on Android 5.0 as our minimum.

@keymanapp-test-bot retest SUITE_ROTATION GROUP_ANDROID_5 TEST_OSK_PORTRAIT_LOAD TEST_OSK_LANDSCAPE_LOAD TEST_BROWSER_ROTATE_P-TO-L TEST_BROWSER_ROTATE_L-TO-P

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_TABLET: - any modern Android tablet should suffice

OSK_Portrait:

OSK_Landscape:

before rotation:

after rotation:

before rotation:

after rotation:

bharanidharanj commented 1 year ago

SUITE_ROTATION:

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

before rotation:

after rotation:

before rotation:

after rotation:

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_IOS_PHONE: - any modern iOS phone should suffice

before rotation:

after rotation:

before rotation:

after rotation:

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_IOS_TABLET: - any modern iOS tablet should suffice

before rotation:

after rotation:

before rotation:

after rotation:

bharanidharanj commented 1 year ago

SUITE_ROTATION:

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

before rotation:

after rotation:

before rotation:

after rotation:

bharanidharanj commented 1 year ago

SUITE_ROTATION:

GROUP_ANDROID_PHONE: - any modern Android phone should suffice

before rotation:

after rotation:

before rotation:

after rotation:

MakaraSok commented 1 year ago

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

I've observed that the OSK only stays visible when tested on an actual device. It quickly disappears when tested with an emulator, API 30.

bharanidharanj commented 1 year ago

Test Results

SUITE_ROTATION:

GROUP_ANDROID_TABLET: - any modern Android tablet should suffice

keyman-server commented 1 year ago

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