fix(web): position popups correctly in landscape mode on Android and during Chrome emulation #5075

jahorton · 2021-05-13T02:20:44Z

To ensure this doesn't break anything for Android...

User Testing

I'm requesting two sets of tests. That said, I'd only expect an issue from the second (non-embedded) set if any issue occurs.

In-app test

Just fire up the Android app (from this PR's build) and ensure subkeys work as expected in landscape mode, both in-app and in system-keyboard mode. Behavior should be unaffected and match your default expectations.

Non-embedded test

This one's going to be a bit more involved, but reflects the actually-changed parts of the code.

Ensure that KeymanWeb's test pages are accessible through localhost on your machine.
- In case you don't yet have a localhost setup: instructions
In your local copy of the repo, navigate to web/source and run ./build.sh.
Once complete, in Chrome navigate to the unminified testing page using your localhost setup.
- For my setup, that's http://localhost/testing/unminified.html.
Activate device emulation and ensure that the subkeys display where you expect them to be.
- For both Android and iOS device types, both phone and tablet form factors.
Then, use Android Studio to launch an emulated Android device.
Launch the Chrome app and navigate to the same testing URL, but in place of localhost, type 10.0.2.2.
- For my setup, that's 10.0.2.2/localhost/testing/unminified.html.
Ensure that the popup keys position properly on the emulated Android device.

A similar test may be run for steps 5 through 7 for those developing on Mac in order to test iOS device setups. (But without replacing localhost with 10.0.2.2.)

Once the PR lands on master, this will become possible to test directly with keymanweb.com.

mcdurdin · 2021-05-13T03:13:59Z

web/source/osk/visualKeyboard.ts

@@ -1789,8 +1789,12 @@ namespace com.keyman.osk {

      // And correct its position with respect to that element
      ss=subKeys.style;
-      var x=dom.Utils.getAbsoluteX(e)+0.5*(e.offsetWidth-subKeys.offsetWidth), y,
-        xMax=(util.landscapeView()?screen.height:screen.width)-subKeys.offsetWidth;
+      var x=dom.Utils.getAbsoluteX(e)+0.5*(e.offsetWidth-subKeys.offsetWidth);


var y is no longer defined?

VS Code indicates that the variable was unused, so yes.

mcdurdin · 2021-05-13T03:17:14Z

web/source/osk/visualKeyboard.ts

+      var x=dom.Utils.getAbsoluteX(e)+0.5*(e.offsetWidth-subKeys.offsetWidth);
+
+      // https://stackoverflow.com/questions/28083715/how-to-detect-if-a-mobile-device-is-emulated-by-google-chrome
+      // (Note the last reply to the accepted answer.)


Suggested change

// (Note the last reply to the accepted answer.)

// Note in particular https://stackoverflow.com/questions/28083715/#comment108747578_28084346:

// The definitive way to determine this is to check for `navigator.maxTouchPoints > 1` Because Chrome

// emulated devices will always have only 1 touchpoint (ie. your mouse cursor), while actual (modern)

// mobile devices will always have more than 1, and devices without touch support will always have

// 0. – Ivan [CC-BY-SA 4.0]

And yet I wonder if this is true on a Windows computer with a touch screen.

I will try and test this on my wife's touch-screen laptop shortly.

If false in that case, it'll still fail during Chrome emulation of an iOS device. Fortunately, all standard user use-cases will be fine, though it could affect keyboard developers like Makara when using the Developer test host.

I have verified that navigator.maxTouchPoints is 10 on my wife's computer when mobile emulation is not running, but it is 1 when mobile emulation is running. So that's good news.

mcdurdin · 2021-05-13T03:19:57Z

Suggest changing title to clarify what this is fixing, e.g. "fix(web): position popups correctly in landscape mode on Android and Chrome emulation"?

mcdurdin · 2021-05-13T03:33:18Z

Position is better but width of popup seems incorrect on right side of screen. The p popup should be a single row, for example.

mcdurdin · 2021-05-13T03:44:02Z

I'm trying to get a full picture of the possible scenarios we are trying to support, and the differences between them:

iPhone/iPad - portrait
iPhone/iPad - landscape
Android phone/tablet - portrait
Android phone/tablet - landscape
(Windows/mac) Chrome emulation of iPhone/iPad - portrait
(Windows/mac) Chrome emulation of iPhone/iPad - landscape
(Windows/mac) Chrome emulation of Android phone/tablet - portrait
(Windows/mac) Chrome emulation of Android phone/tablet - landscape

Is that a comprehensive list? Is there a table we can draw up of the differences between the values we are relying on? (If possible, we should file a bug against Chrome if its emulation of iPhone/iPad mismatches.)

Also:

Firefox emulation of above (future)

jahorton · 2021-05-13T03:47:04Z

OK, I can repro that effect when targeting a Pixel 2. (It didn't show up when targeting a Nexus.)

MakaraSok · 2021-05-13T04:13:04Z

In-app test

Just fire up the Android app (from this PR's build) and ensure subkeys work as expected in landscape mode, both in-app and in system-keyboard mode. Behavior should be unaffected and match your default expectations.

Tested on Android Emulator (Pixel 2 API 29, Android 10). The popped key on the top right corner behaves as expected within Keyman app on landscape mode. The same normal/expected behavior is observed when using it as a system keyboard on landscape mode.

Still figure out how to test "Non-embedded test". I will get back to this first thing this afternoon.

mcdurdin · 2021-05-13T04:55:48Z

web/source/osk/visualKeyboard.ts

-      var x=dom.Utils.getAbsoluteX(e)+0.5*(e.offsetWidth-subKeys.offsetWidth), y,
-        xMax=(util.landscapeView()?screen.height:screen.width)-subKeys.offsetWidth;
+      var x = dom.Utils.getAbsoluteX(e)+0.5*(e.offsetWidth-subKeys.offsetWidth);
+      var xMax = keyman.osk.getWidth() - subKeys.offsetWidth;


Hmm, liking this more than the previous iteration! 😂

jahorton · 2021-05-13T04:56:29Z

I usually prefer to not have VisualKeyboard refer to its owning OSKManager when possible, but it looks like the latter's width-calc function is more robust here; screen.width doesn't give the available width on Android devices. As the base OSK was built correctly, we can tell that osk.getWidth() is getting the right values.

MakaraSok · 2021-05-13T08:54:48Z

Non-embedded test

Tested using https://ecb5a861dbca.ngrok.io/keymanweb/testing/unminified.html and iPad/Pixel 2/iPhone emulated on Chrome 90.0.4430.212 in landscape mode.

The popped up keys are shown directly on the based key on the top right corner. It is working as expected.

MakaraSok · 2021-05-13T09:03:49Z

Screenshots:

mcdurdin

LGTM. Tested on Android physical device now with good results as well as on Chrome with various devices all in landscape and portrait, and no problems encountered.

mcdurdin · 2021-05-13T23:28:30Z

I usually prefer to not have VisualKeyboard refer to its owning OSKManager when possible

Does this potentially indicate a refactor of that code is in the future?

mcdurdin · 2021-05-14T04:50:27Z

I think we should cherry-pick to stable-14.0 :)

jahorton · 2021-05-17T01:03:08Z

I usually prefer to not have VisualKeyboard refer to its owning OSKManager when possible

Does this potentially indicate a refactor of that code is in the future?

I mean, yeah. OSK-Core work's going to affect more than a few things. There are other parts of the code that have been using the same approach; this just adds an extra to the list.

keyman-server · 2021-05-17T18:00:49Z

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

fix(web): popup key positioning logic

a1d989c

jahorton added web/ fix labels May 13, 2021

jahorton added this to the A15S4 milestone May 13, 2021

github-actions bot added the web/engine/ label May 13, 2021

mcdurdin reviewed May 13, 2021

View reviewed changes

jahorton changed the title ~~fix(web): popup key positioning logic~~ fix(web): position popups correctly in landscape mode on Android and during Chrome emulation May 13, 2021

fix(web): reuses OSK width calc

3102435

mcdurdin reviewed May 13, 2021

View reviewed changes

mcdurdin approved these changes May 13, 2021

View reviewed changes

mcdurdin modified the milestones: A15S4, A15S5 May 14, 2021

jahorton mentioned this pull request May 17, 2021

fix(web): position popups correctly in landscape mode on Android and during Chrome emulation 🍒 #5090

Merged

jahorton merged commit 2c4ce9c into master May 17, 2021

jahorton deleted the fix/web/popup-key-positioning branch May 17, 2021 01:16

jahorton mentioned this pull request Jun 29, 2021

refactor(web): longpresses, groundwork for additional gestures 🍕 #5387

Merged

mcdurdin added the has-user-test label Feb 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(web): position popups correctly in landscape mode on Android and during Chrome emulation #5075

fix(web): position popups correctly in landscape mode on Android and during Chrome emulation #5075

jahorton commented May 13, 2021 •

edited

mcdurdin May 13, 2021

jahorton May 13, 2021

mcdurdin May 13, 2021

jahorton May 13, 2021

mcdurdin May 13, 2021

mcdurdin commented May 13, 2021

mcdurdin commented May 13, 2021

mcdurdin commented May 13, 2021

jahorton commented May 13, 2021

MakaraSok commented May 13, 2021

In-app test

mcdurdin May 13, 2021

jahorton commented May 13, 2021

MakaraSok commented May 13, 2021

MakaraSok commented May 13, 2021

mcdurdin left a comment

mcdurdin commented May 13, 2021

mcdurdin commented May 14, 2021

jahorton commented May 17, 2021

keyman-server commented May 17, 2021

-      // (Note the last reply to the accepted answer.)
+      // Note in particular https://stackoverflow.com/questions/28083715/#comment108747578_28084346:
+      // The definitive way to determine this is to check for `navigator.maxTouchPoints > 1` Because Chrome
+      // emulated devices will always have only 1 touchpoint (ie. your mouse cursor), while actual (modern)
+      // mobile devices will always have more than 1, and devices without touch support will always have
+      // 0. – Ivan [CC-BY-SA 4.0]

fix(web): position popups correctly in landscape mode on Android and during Chrome emulation #5075

fix(web): position popups correctly in landscape mode on Android and during Chrome emulation #5075

Conversation

jahorton commented May 13, 2021 • edited

User Testing

In-app test

Non-embedded test

mcdurdin May 13, 2021

Choose a reason for hiding this comment

jahorton May 13, 2021

Choose a reason for hiding this comment

mcdurdin May 13, 2021

Choose a reason for hiding this comment

jahorton May 13, 2021

Choose a reason for hiding this comment

mcdurdin May 13, 2021

Choose a reason for hiding this comment

mcdurdin commented May 13, 2021

mcdurdin commented May 13, 2021

mcdurdin commented May 13, 2021

jahorton commented May 13, 2021

MakaraSok commented May 13, 2021

In-app test

mcdurdin May 13, 2021

Choose a reason for hiding this comment

jahorton commented May 13, 2021

MakaraSok commented May 13, 2021

Non-embedded test

MakaraSok commented May 13, 2021

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin commented May 13, 2021

mcdurdin commented May 14, 2021

jahorton commented May 17, 2021

keyman-server commented May 17, 2021

jahorton commented May 13, 2021 •

edited