feat(web): VisualKeyboard layout-reflow optimization 🪠 #11177

jahorton · 2024-04-05T06:00:20Z

Now for some of the "meat" of the OSK optimization - this PR heavily reduces VisualKeyboard layout reflow, batching similar operations (read vs write) as much as possible at all times. Outside of a few knock-on effects for key and gesture previews that needed resolving, all changes are restricted to the scope of VisualKeyboard.refreshLayout(), the methods it calls, and the values it requests.

This PR has been spun off of an older state of #11140.

@keymanapp-test-bot skip

All testing reasonable to do here is also reasonable to do on #11140.

keymanapp-test-bot · 2024-04-05T06:00:23Z

User Test Results

Test specification and instructions

User tests are not required

…o feat/web/vkbd-layout-optimization

keyman-server · 2024-04-12T18:04:36Z

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

mcdurdin

I am not comfortable with this change. We may have to live with it for 17.0 because we are running out of time, but the code flow is extremely obtuse due to the extensive use of closures, and I think it will be very hard to maintain.

I think we should be able to simplify some of these patterns dramatically with a bit of careful design, what do you think?

mcdurdin · 2024-04-13T03:18:39Z

web/src/engine/osk/src/keyboard-layout/oskBaseKey.ts

-    this.square.style.marginLeft = vkbd.layoutWidth.scaledBy(key.proportionalPad).styleString;
-    this.btn.style.width = vkbd.usesFixedWidthScaling ? this.square.style.width : '100%';
+  public refreshLayout(layoutParams: KeyLayoutParams) {
+    const keyTextClosure = super.refreshLayout(layoutParams);  // key labels in particular.


Why 'keyTextClosure'? It's really unclear what that does! Perhaps we should call it superFinalization to clarify that it's more work that super.refreshlayout has to do once this function is finished?

Why 'keyTextClosure'?

Because it's a closure to finalize/apply layout changes directly tied to key text as seen in OSKKey.

Perhaps we should call it superFinalization [...]

If I swapped to superFinalization or something else unrelated to the code-contents of the closure... well, I suppose I can always just add a comment about what the closure's actual effects are.

mcdurdin · 2024-04-13T03:21:43Z

web/src/engine/osk/src/keyboard-layout/oskKey.ts

+        return () => {
+          this.label.style.fontSize = fontSize.styleString;
+        };


This is kinda ugly and it's unclear why this is postponed and how it gets called

This is the other side of that keyTextClosure you were asking about - its actual definition.

Admittedly, yeah, the keyTextClosure aspect is probably the least-clear thing in this PR. It got tricky to forward well due to the inheritance structures involved.

mcdurdin · 2024-04-13T03:23:16Z

web/src/engine/osk/src/keyboard-layout/oskKey.ts

+  public refreshLayout(layoutParams: KeyLayoutParams) {
+    // Avoid doing any font-size related calculations if there's no text to display.
+    if(this.spec.sp == ButtonClasses.spacer || this.spec.sp == ButtonClasses.blank) {
+      return () => {};


It'd be better to return null and then in the caller, use closure?.() to make the call: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining#optional_chaining_with_function_calls

mcdurdin · 2024-04-13T03:27:26Z

web/src/engine/osk/src/keyboard-layout/oskLayer.ts

+    rowClosures.forEach((closure) => closure());
+    rowKeyClosures.forEach((closure) => closure());


I'm really not keen on all this passing around of closures. It's really hard to follow what is happening when. It shouldn't be necessary with a good structure to the code.

Huh.

To me, I felt that this made it extremely clear that we've deferred and essentially batched all operations that could affect DOM layout (and thus, could trigger layout reflow) to this one place. (Granted, maybe I should have added documentation of this detail to the lower-level closure-building calls...) All of the closures rely on precomputed values, some of which do refer to existing layout (when prerequisite values are uncached). By precomputing all values first, without any edits... and then triggering those edits later... that's how we prevent layout reflow here.

Now that all precomputations are complete, we're safe to apply all of them sequentially without any reference to layout or style information. Use of closures automatically caches the precomputations with manually-defined code, which is a major reason I used the pattern. (Rather than manually tracking the values, we can just leverage existing language features to do that heavy lifting.)

Granted, maybe some extra comments could make this clearer - whether here or on the lower-level refresh methods in regard to why the returned closures exist. What would make this clearer for you? Alternatively... what alternate "good structure" would you suggest?

mcdurdin · 2024-04-13T03:28:03Z

web/src/engine/osk/src/keyboard-layout/oskLayer.ts

-    const layerHeight = layoutParams.keyboardHeight;
-    this.showLanguage(layoutParams.spacebarText);
-
+  public refreshLayout(layoutParams: LayerLayoutParams) {


So this implementation of refreshLayout doesn't return a closure but other ones do. It would be good if this was clear without having to search for return statements.

sentry-io · 2024-04-15T12:10:38Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ TypeError: null is not an object (evaluating 'e.val') new ParsedLengthStyle(kmw/engine/17.0.308/src/w... View Issue

_{Did you find this useful? React with a 👍 or 👎}

jahorton

I think we should be able to simplify some of these patterns dramatically with a bit of careful design, what do you think?

I can only really see it working if we promote those closures to class methods... and manually construct the value-tracking objects that we get for free by using closures. I did look at that pattern initially, but it started getting a bit too gnarly for my tastes for the cases where key-style values weren't yet cached.

Then again, maybe having a "try-cache" method (sorry re: the horrible pun) before the precomputation pass could work? (As in, "fetch key font-style info if not yet cached and cacheable".)

Waaaaaaait. That (the "try-cache") might actually work better. We'd need to test, but I think it'd be possible to run that specific call earlier in the VisualKeyboard.refreshLayout method - before it runs its getComputedStyle calls. If so, I believe we'd be able to eliminate a major layout-reflow source I previously thought we couldn't.

(Reasoning: the font-size computation either picks up a fixed font-size - in which case, changing the base font-scaling doesn't matter - or it picks up a relative one... and still successfully computes the local, key-specific scaling factor just fine.)

jahorton · 2024-04-17T01:31:20Z

web/src/engine/osk/src/keyboard-layout/oskBaseKey.ts

-    this.square.style.marginLeft = vkbd.layoutWidth.scaledBy(key.proportionalPad).styleString;
-    this.btn.style.width = vkbd.usesFixedWidthScaling ? this.square.style.width : '100%';
+  public refreshLayout(layoutParams: KeyLayoutParams) {
+    const keyTextClosure = super.refreshLayout(layoutParams);  // key labels in particular.


Why 'keyTextClosure'?

Because it's a closure to finalize/apply layout changes directly tied to key text as seen in OSKKey.

Perhaps we should call it superFinalization [...]

If I swapped to superFinalization or something else unrelated to the code-contents of the closure... well, I suppose I can always just add a comment about what the closure's actual effects are.

jahorton · 2024-04-17T01:33:14Z

web/src/engine/osk/src/keyboard-layout/oskKey.ts

+        return () => {
+          this.label.style.fontSize = fontSize.styleString;
+        };


This is the other side of that keyTextClosure you were asking about - its actual definition.

Admittedly, yeah, the keyTextClosure aspect is probably the least-clear thing in this PR. It got tricky to forward well due to the inheritance structures involved.

jahorton · 2024-04-17T01:41:58Z

web/src/engine/osk/src/keyboard-layout/oskLayer.ts

+    rowClosures.forEach((closure) => closure());
+    rowKeyClosures.forEach((closure) => closure());


Huh.

To me, I felt that this made it extremely clear that we've deferred and essentially batched all operations that could affect DOM layout (and thus, could trigger layout reflow) to this one place. (Granted, maybe I should have added documentation of this detail to the lower-level closure-building calls...) All of the closures rely on precomputed values, some of which do refer to existing layout (when prerequisite values are uncached). By precomputing all values first, without any edits... and then triggering those edits later... that's how we prevent layout reflow here.

Now that all precomputations are complete, we're safe to apply all of them sequentially without any reference to layout or style information. Use of closures automatically caches the precomputations with manually-defined code, which is a major reason I used the pattern. (Rather than manually tracking the values, we can just leverage existing language features to do that heavy lifting.)

Granted, maybe some extra comments could make this clearer - whether here or on the lower-level refresh methods in regard to why the returned closures exist. What would make this clearer for you? Alternatively... what alternate "good structure" would you suggest?

mcdurdin · 2024-04-17T05:55:13Z

Granted, maybe some extra comments could make this clearer - whether here or on the lower-level refresh methods in regard to why the returned closures exist. What would make this clearer for you? Alternatively... what alternate "good structure" would you suggest?

Some extra comments would definitely help. The problem with the term closure is that I have to go and find the definition of it to figure out what it is actually doing.

keyman/web/src/engine/osk/src/keyboard-layout/oskLayer.ts

Lines 195 to 196 in 573ffd8

    
           rowClosures.forEach((closure) => closure()); 
        
           rowKeyClosures.forEach((closure) => closure());

jahorton added 4 commits April 5, 2024 12:39

feat(web): VisualKeyboard layout-reflow optimization

a6093c2

fix(web): fixed width vs fixed height references

f2dba20

change(web): layer-specific refresh tweak

8a21fbc

docs(web): comment on recent code

f21afb1

jahorton requested a review from ermshiperete as a code owner April 5, 2024 06:00

keymanapp-test-bot bot added this to the B17S5 milestone Apr 5, 2024

github-actions bot added web/ feat labels Apr 5, 2024

jahorton mentioned this pull request Apr 5, 2024

change(web): OSK optimization, improved responsiveness 🪠 #11140

Merged

jahorton added 2 commits April 8, 2024 09:12

fix(web): key-height calc should be based on btn, not square

bee64b6

fix(web): refixes bad return case

328900c

ermshiperete approved these changes Apr 9, 2024

View reviewed changes

jahorton added 2 commits April 10, 2024 14:44

fix(web): phone key-tip font scaling

5a55ee1

chore(web): Merge branch 'refactor/web/layer-group-encapsulation' int…

573ffd8

…o feat/web/vkbd-layout-optimization

ermshiperete approved these changes Apr 10, 2024

View reviewed changes

Base automatically changed from refactor/web/layer-group-encapsulation to beta April 12, 2024 00:49

jahorton merged commit 68f6a24 into beta Apr 12, 2024
14 of 15 checks passed

jahorton deleted the feat/web/vkbd-layout-optimization branch April 12, 2024 00:49

mcdurdin reviewed Apr 13, 2024

View reviewed changes

jahorton commented Apr 17, 2024

View reviewed changes

jahorton mentioned this pull request Apr 17, 2024

bug(web): empty-string style parse lacks proper default #11234

Closed

This was referenced Apr 17, 2024

chore(web): simple layout reflow polish 🪠 #11237

Merged

change(web): drops need for closures to optimize layout-reflow 🪠 #11238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(web): VisualKeyboard layout-reflow optimization 🪠 #11177

feat(web): VisualKeyboard layout-reflow optimization 🪠 #11177

jahorton commented Apr 5, 2024

keymanapp-test-bot bot commented Apr 5, 2024

keyman-server commented Apr 12, 2024

mcdurdin left a comment

mcdurdin Apr 13, 2024

jahorton Apr 17, 2024

mcdurdin Apr 13, 2024

jahorton Apr 17, 2024

mcdurdin Apr 13, 2024

mcdurdin Apr 13, 2024

jahorton Apr 17, 2024

mcdurdin Apr 13, 2024

sentry-io bot commented Apr 15, 2024

jahorton left a comment

jahorton Apr 17, 2024

jahorton Apr 17, 2024

jahorton Apr 17, 2024

mcdurdin commented Apr 17, 2024

		rowClosures.forEach((closure) => closure());
		rowKeyClosures.forEach((closure) => closure());

feat(web): VisualKeyboard layout-reflow optimization 🪠 #11177

feat(web): VisualKeyboard layout-reflow optimization 🪠 #11177

Conversation

jahorton commented Apr 5, 2024

keymanapp-test-bot bot commented Apr 5, 2024

User Test Results

keyman-server commented Apr 12, 2024

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Apr 15, 2024

Suspect Issues

jahorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin commented Apr 17, 2024