fix(web): optimizes, enhances caret placement within text on mobile devices by jahorton · Pull Request #6551 · keymanapp/keyman

jahorton · 2022-04-20T18:12:30Z

Dramatically reduces the time taken to place the caret within text for TouchAliasElements, the emulated text inputs KMW produces on mobile devices to facilitate use of the Web-based keyboard. This is most notable for the first touch into such an area, which had been particularly bad due to some initialization issues for the caret-placement algorithm.

While I was at it, I also fixed up the caret placement logic; it should land where expected far, far more consistently and reliably than before.

User Testing

This PR adds a new testing page: web/testing/issue5312-touch-alias-optimization.

Said testing page (seen as "Tests performance of touch-alias elements with large amounts of text (#5312)" on the testing page index) features a text area preloaded with 6957 characters. (Thanks, Lorem Ipsum Generator.) Without the optimizations, my testing tab in Chrome would effectively hang immediately. (I had to force-quit Chrome a number of times for testing!) Now... interactions feel nigh-instant!

Tests

Use remote device emulation on Google Chrome on the testing page noted above.

GROUP_PHONE: Run tests on the page while emulating an iPhone SE via Chrome mobile-device emulation.
- Note that the page will likely auto-scroll on your first touch within the testing text. (The smaller resolution forces this.)
GROUP_TABLET: Run tests on the page while emulating an iPad Air (or similar tablet form-factor) using Chrome mobile-device emulation.
TEST_INITIAL_TOUCH:
- Touch text in the middle of the visible paragraph, preferably toward the center vertically and horizontally.
- The caret should land very near to the touched location, practically under the touch if possible.
- Repeat at least 3 times in different locations; ensure it behaves as expected.
- Finally, repeat once more slightly to the left or right of the previous repetition; ensure it behaves as expected.
- If any attempt to place a caret via touch for this test takes over 0.25 seconds, fail this test.
TEST_SCROLLED_TOUCH:
- Scroll the textbox down three paragraphs.
- Touch text in the middle of the now-top (fourth overall) paragraph, preferably toward the center vertically and horizontally.
- The caret should land very near to the touched location, practically under the touch if possible.
- Repeat at least 3 times in different locations; ensure it behaves as expected.
- Finally, repeat once more slightly to the left or right of the previous repetition; ensure it behaves as expected.
- If any attempt to place a caret via touch for this test takes over 0.25 seconds, fail this test.

As noted, there may be very slight lag on caret-placement attempts, but only very slight. It shouldn't ever come close to feeling like it's 'locked up' your tab in the browser.

Lone caveat: unless I'm mistaken, it will not allow you to place the caret at the start of any line but the first. If it does, it won't allow you to place it at the end of the previous line. Only one of the two will be possible; that's a TouchAliasElement limitation.

keymanapp-test-bot · 2022-04-20T18:12:34Z

User Test Results

Test specification and instructions

✅ GROUP_PHONE: Run tests on the page while emulating an iPhone SE via Chrome mobile-device emulation.
2 tests PASSED
- ✅ TEST_INITIAL_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.
- ✅ TEST_SCROLLED_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.
✅ GROUP_TABLET: Run tests on the page while emulating an iPad Air (or similar tablet form-factor) using Chrome mobile-device emulation.
2 tests PASSED
- ✅ TEST_INITIAL_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.
- ✅ TEST_SCROLLED_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.

Test Artifacts

Android
Developer
- Keyman Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 15.0.237 (0.6551.6165)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows
- Keyman for Windows
- FirstVoices Keyboards for Windows

jahorton · 2022-04-20T20:32:24Z

web/source/dom/domEventHandlers.ts

-    setFocusWithTouch(tEvent: {clientX: number, clientY: number, target?: EventTarget}) {
-      var touchX=tEvent.clientX,touchY=tEvent.clientY;
+    setFocusWithTouch(tEvent: {pageX: number, pageY: number, target?: EventTarget}) {
+      var touchX=tEvent.pageX,touchY=tEvent.pageY;


The getAbsoluteY function (used heavily by caret-placement logic) operates from a pageY perspective.

This change is needed to properly handle caret positioning when the page is scrolled; my iPhone SE tests (see test spec) fail otherwise, as the page will auto-scroll, causing divergence between clientY and pageY.

This looks like it's just renaming variables to better match reality, but it doesn't change any behavior, or do I miss something here?

Note the second line - there's an actual functional change here.

This link may help explain the distinction: https://stackoverflow.com/a/21452887

Ahh, I fell into the TypeScript trap again 😄 The event actually contains both client* and page* fields but we were using the wrong set. Thanks!

jahorton · 2022-04-21T02:00:04Z

web/source/dom/domEventHandlers.ts

-            y=dom.Utils.getAbsoluteY(caret)-dy;  //top of caret
-            if(y > touchY && cp > cpMin && cp != cpMax) {cpMax=cp; cp=Math.round((cp+cpMin)/2);}
-            else if(y < touchY-yRow && cp < cpMax && cp != cpMin) {cpMin=cp; cp=Math.round((cp+cpMax)/2);}
-            else break;
-            target.setTextCaret(cp);
-          }
-
-          while(dom.Utils.getAbsoluteY(caret)-dy > touchY && cp > cpMin) {
-            target.setTextCaret(--cp);
-          }
-
-          while(dom.Utils.getAbsoluteY(caret)-dy < touchY-yRow && cp < cpMax) {
-            target.setTextCaret(++cp);
-          }


For comparison with the other block, re: getAbsoluteY motivating the clientY -> pageY change.

This block still exists; it's just been refactored into the new performCaretSearch function. Along with some light refactoring to reformat and clarify the code, as it was somewhat dense in its original form.

bharanidharanj · 2022-04-21T13:35:14Z

GROUP_PHONE: Run tests on the page while emulating an iPhone SE via Chrome mobile-device emulation.

TEST_INITIAL_TOUCH (FAILED): Tested this as per the instructions in Chrome mobile emulator and it seems to be a little bit sluggishness on the cursor while repeating the steps.

TEST_SCROLLED_TOUCH (FAILED): Tested this as per the instructions in Chrome mobile emulator and it seems to be a little bit sluggishness on the cursor while repeating the steps.

GROUP_TABLET: Run tests on the page while emulating an iPad Air (or similar tablet form-factor) using Chrome mobile-device emulation.

TEST_INITIAL_TOUCH (FAILED): Tested this as per the instructions in Chrome mobile emulator and it seems to be a little bit sluggishness on the cursor while repeating the steps.

TEST_SCROLLED_TOUCH (FAILED): Tested this as per the instructions in Chrome mobile emulator and it seems to be a little bit sluggishness on the cursor while repeating the steps.

@bharanidharanj

and it seems to be a little bit sluggishness on the cursor while repeating the steps

Define "a little bit sluggishness". Are we talking about a quarter second, a half second, more than one second...? It'd help to know the order of magnitude.
I did note a "little bit", but on my computer (at least) it was under 0.25 seconds, hence that detail from the test instructions:

If any attempt to place a caret via touch for this test takes over 0.25 seconds...

A little bit sluggish, but only about as long as it takes to blink... which I think is supposed to be about 0.1 seconds, actually. If it was the same for you, then all the tests passed, not failed.
Either way, "a little bit" is, at least, far, far better than the amount of time it originally took.

@jahorton Okay, Thanks for the clarification. I will retest it (with stop clock) and will post my comment.

@jahorton "little bit" sluggishness means sometime, the cursor stick around the text even though I clicked some other text in the same paragraph. But it happens randomly. Not able to reproduce it.

jahorton · 2022-04-21T15:08:21Z

@bharanidharanj

and it seems to be a little bit sluggishness on the cursor while repeating the steps

Define "a little bit sluggishness". Are we talking about a quarter second, a half second, more than one second...? It'd help to know the order of magnitude.

I did note a "little bit", but on my computer (at least) it was under 0.25 seconds, hence that detail from the test instructions:

If any attempt to place a caret via touch for this test takes over 0.25 seconds...

A little bit sluggish, but only about as long as it takes to blink... which I think is supposed to be about 0.1 seconds, actually. If it was the same for you, then all the tests passed, not failed.

Either way, "a little bit" is, at least, far, far better than the amount of time it originally took.

ermshiperete · 2022-04-21T13:33:25Z

web/source/dom/domEventHandlers.ts

-    setFocusWithTouch(tEvent: {clientX: number, clientY: number, target?: EventTarget}) {
-      var touchX=tEvent.clientX,touchY=tEvent.clientY;
+    setFocusWithTouch(tEvent: {pageX: number, pageY: number, target?: EventTarget}) {
+      var touchX=tEvent.pageX,touchY=tEvent.pageY;


This looks like it's just renaming variables to better match reality, but it doesn't change any behavior, or do I miss something here?

bharanidharanj · 2022-04-22T09:40:51Z

@bharanidharanj

and it seems to be a little bit sluggishness on the cursor while repeating the steps

Define "a little bit sluggishness". Are we talking about a quarter second, a half second, more than one second...? It'd help to know the order of magnitude.

I did note a "little bit", but on my computer (at least) it was under 0.25 seconds, hence that detail from the test instructions:

If any attempt to place a caret via touch for this test takes over 0.25 seconds...

A little bit sluggish, but only about as long as it takes to blink... which I think is supposed to be about 0.1 seconds, actually. If it was the same for you, then all the tests passed, not failed.

Either way, "a little bit" is, at least, far, far better than the amount of time it originally took.

@jahorton Okay, Thanks for the clarification. I will retest it (with stop clock) and will post my comment.

bharanidharanj · 2022-04-22T10:33:15Z

GROUP_PHONE: Run tests on the page while emulating an iPhone SE via Chrome mobile-device emulation.

TEST_INITIAL_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.
TEST_SCROLLED_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.

GROUP_TABLET: Run tests on the page while emulating an iPad Air (or similar tablet form-factor) using Chrome mobile-device emulation.

TEST_INITIAL_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.
TEST_SCROLLED_TOUCH (PASSED): Retested this issue as per @jahorton suggestion and I noticed that the cursor appear on the text (under 0.25 seconds) in Chrome emulator.

keyman-server · 2022-04-22T18:01:17Z

Changes in this pull request will be available for download in Keyman version 15.0.239-beta

mcdurdin

Looking good. A few comments on the comments, mostly.

mcdurdin · 2022-04-24T22:27:39Z

web/source/dom/domEventHandlers.ts

+      // Except... we... haven't modified cpMin & cpMax since their roles in the
+      // y-position search, which matters if the backing element is a TextArea.
+      // Why not?  That sounds dangerous!


I have no idea what this comment means?!

That comment was written during my analysis of the old code; there was faulty logic when repurposing the <input>-oriented horizontal positioning code, which assumes cpMin and cpMax to be boundary positions on the target line. The <textarea> branch never tightened the bounds.

I can probably clean that up now that the bug's removed, but it was a critical note when diagnosing the caret-misplacement aspect noted in the description:

While I was at it, I also fixed up the caret placement logic; it should land where expected far, far more consistently and reliably than before.

Yes, please clean up -- comments are intended for future readers who don't know the history or context. So write them accordingly 😁

mcdurdin · 2022-04-24T22:30:47Z

web/source/dom/domEventHandlers.ts

      }
    }

+    private performCaretSearch(target: TouchAliasElement, scroller: HTMLElement, touchX: number, touchY: number) {


This seems to have changed somewhat from the previous code. I have done a comparison and think I can see the differences okay.

Also, please add a code comment describing the function.

mcdurdin · 2022-04-24T22:36:52Z

web/source/dom/domEventHandlers.ts

+      var snapOrder;
+      if(target.dir == 'rtl') {  // I would use arrow functions, but IE doesn't like 'em.
+        snapOrder = function(a, b) {
+          return a < b;
+        };
+      } else {
+        snapOrder = function(a, b) {
+          return a > b;
+        };
+      }


Suggested change

var snapOrder;

if(target.dir == 'rtl') { // I would use arrow functions, but IE doesn't like 'em.

snapOrder = function(a, b) {

return a < b;

};

} else {

snapOrder = function(a, b) {

return a > b;

};

}

const snapOrder = target.dir == 'rtl' ? (a,b) => a < b : (a,b) => a > b;

Would it be fine if I placed this change within #6557 instead?

mcdurdin · 2022-04-24T22:37:27Z

web/source/dom/domEventHandlers.ts

+          if(y > maxY) {
+            maxCpMax = searchPt-1; // We already know it's not on this row, so the maximum possible should be decreased.
+          } else {
+            minCpMax = searchPt; // Still in the same row, so the boundary can only be at or after this point.
+          }


Suggested change

if(y > maxY) {

maxCpMax = searchPt-1; // We already know it's not on this row, so the maximum possible should be decreased.

} else {

minCpMax = searchPt; // Still in the same row, so the boundary can only be at or after this point.

}

if(y > maxY) {

// We already know it's not on this row, so the maximum possible should be decreased.

maxCpMax = searchPt-1;

} else {

// Still in the same row, so the boundary can only be at or after this point.

minCpMax = searchPt;

}

mcdurdin · 2022-04-24T22:37:58Z

web/source/dom/domEventHandlers.ts

+          if(y < minY) {
+            minCpMin = searchPt+1; // We already know it's not on this row, so the minimum possible should be increased.
+          } else {
+            maxCpMin = searchPt; // Still in the same row, so the boundary can only be at or before this point.
+          }


Suggested change

if(y < minY) {

minCpMin = searchPt+1; // We already know it's not on this row, so the minimum possible should be increased.

} else {

maxCpMin = searchPt; // Still in the same row, so the boundary can only be at or before this point.

}

if(y < minY) {

// We already know it's not on this row, so the minimum possible should be increased.

minCpMin = searchPt+1;

} else {

// Still in the same row, so the boundary can only be at or before this point.

maxCpMin = searchPt;

}

mcdurdin · 2022-04-24T22:39:50Z

web/source/dom/domEventHandlers.ts

+          if(y > maxY && cp > cpMin /*&& cp != cpMax*/) {
+            // If caret's prior placement is below (after) the touch's y-pos...
+            cpMax=cp;  // new max position
+            cp=Math.round((cp+cpMin)/2); // guess the halfway mark
+          } else if(y < minY && cp < cpMax /*&& cp != cpMin*/) {


Suggested change

if(y > maxY && cp > cpMin /*&& cp != cpMax*/) {

// If caret's prior placement is below (after) the touch's y-pos...

cpMax=cp; // new max position

cp=Math.round((cp+cpMin)/2); // guess the halfway mark

} else if(y < minY && cp < cpMax /*&& cp != cpMin*/) {

if(y > maxY && cp > cpMin) {

// If caret's prior placement is below (after) the touch's y-pos...

cpMax=cp; // new max position

cp=Math.round((cp+cpMin)/2); // guess the halfway mark

} else if(y < minY && cp < cpMax) {

mcdurdin · 2022-04-24T22:41:29Z

web/source/dom/domEventHandlers.ts

+          // Break the binary search if our final search window is extremely small.
+          if(cpMax - cpMin <= 1) {
+            break;
+          }


Suggested change

// Break the binary search if our final search window is extremely small.

if(cpMax - cpMin <= 1) {

break;

}

if(cpMax - cpMin <= 1) {

// Break the binary search if our final search window is extremely small.

break;

}

mcdurdin · 2022-04-24T22:43:26Z

web/source/dom/touchAliasElement.ts

    __caretTimerId: number;
    __activeCaret: boolean = false;

+    __midCaretSearching: boolean = false;


Perhaps __executingCaretSearch is better? ('mid' is a bit confusing as it has mathematical meaning as well).

mcdurdin · 2022-04-24T22:45:53Z

web/source/dom/domEventHandlers.ts

+        let _this = this;
+        target.executeCaretSearchFunctor(function() {
+          _this.performCaretSearch(target, scroller, touchX, touchY);
+        });


Suggested change

let _this = this;

target.executeCaretSearchFunctor(function() {

_this.performCaretSearch(target, scroller, touchX, touchY);

});

target.executeCaretSearch(() => this.performCaretSearch(target, scroller, touchX, touchY));

Using arrow function means we have lexical this.

mcdurdin · 2022-04-24T22:47:13Z

web/source/dom/touchAliasElement.ts

+     * Exists to encapsulate this behavior to ensure it's always, unfailingly re-enabled.
+     * @param functor A function that will search for the best caret position.
+     */
+    executeCaretSearchFunctor(functor: () => void) {


Just executeCaretSearch is probably a good name? Functor is not really required.

I understand you need this indirection because the state variable is in touchAliasElement but the actual search is done in domEventHandlers, yes?

I understand you need this indirection because the state variable is in touchAliasElement but the actual search is done in domEventHandlers, yes?

Correct.

jahorton added 3 commits April 20, 2022 12:28

feat(web): touch-alias optimization test page

931cf73

fix(web): major performance boost, fixes search bound logic bug

cd666d0

change(web): further clarifies performCaretSearch

1c394c2

jahorton added this to the B15S7 milestone Apr 20, 2022

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Apr 20, 2022

github-actions bot added web/ web/engine/ fix labels Apr 20, 2022

jahorton added 6 commits April 20, 2022 14:52

fix(web): mobile device caret positioning

94edc5c

docs(web): better comments & var naming

48fb137

chore(web): minor polish

a355f2e

docs(web): documents some optimizing blocks

d3900fc

docs(web): credited the text generator

73d4b09

fix(web): caret position when page is scrolled

56b4c5a

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Apr 20, 2022

jahorton linked an issue Apr 20, 2022 that may be closed by this pull request

bug(web): performance problem clicking on textarea with large content [touch] #5312

Closed

jahorton marked this pull request as ready for review April 20, 2022 20:30

jahorton requested review from ermshiperete and mcdurdin as code owners April 20, 2022 20:30

jahorton commented Apr 20, 2022

View reviewed changes

jahorton commented Apr 21, 2022

View reviewed changes

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Apr 21, 2022

ermshiperete approved these changes Apr 21, 2022

View reviewed changes

jahorton merged commit 35d011d into beta Apr 22, 2022

jahorton deleted the fix/web/5312-touch-alias-optimization branch April 22, 2022 13:22

mcdurdin reviewed Apr 24, 2022

View reviewed changes

jahorton mentioned this pull request Apr 25, 2022

change(web): touch alias optimization followup #6566

Merged

jahorton pushed a commit that referenced this pull request Jun 14, 2022

change(web): implements arrow-func desired in #6551 comment

5065672

Uh oh!

Conversation

jahorton commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Testing

Tests

Uh oh!

keymanapp-test-bot bot commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

jahorton Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharanidharanj commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GROUP_PHONE: Run tests on the page while emulating an iPhone SE via Chrome mobile-device emulation.

GROUP_TABLET: Run tests on the page while emulating an iPad Air (or similar tablet form-factor) using Chrome mobile-device emulation.

Uh oh!

jahorton commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharanidharanj commented Apr 22, 2022

Uh oh!

bharanidharanj commented Apr 22, 2022

GROUP_PHONE: Run tests on the page while emulating an iPhone SE via Chrome mobile-device emulation.

GROUP_TABLET: Run tests on the page while emulating an iPad Air (or similar tablet form-factor) using Chrome mobile-device emulation.

Uh oh!

keyman-server commented Apr 22, 2022

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahorton Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

jahorton commented Apr 20, 2022 •

edited

Loading

keymanapp-test-bot bot commented Apr 20, 2022 •

edited

Loading

jahorton Apr 20, 2022 •

edited

Loading

bharanidharanj commented Apr 21, 2022 •

edited

Loading

jahorton commented Apr 21, 2022 •

edited

Loading

jahorton Apr 25, 2022 •

edited

Loading