fix(common/web): mock deadkey handling after rules manipulating context 📴

[jahorton]

Happened upon this one while working on #7343.

The bug being addressed: a keyboard rule triggered manipulation of prior context that resulted in adding or removing a character before the caret, deadkey positions weren't being updated. Subsequent rules would fail to match any deadkeys still in the context after the caret.

The scenario is quite niche, so it's unlikely to have had any major effect - especially since we're talking about post-caret deadkeys. If any effect resulted from this, it'd have affected our predictive text quality.

@keymanapp-test-bot skip

This bug was discovered by our unit tests once I corrected a reference to point to Mock testing instead of TouchAlias testing, and the fix fully corrects it without any change to the unit test. Unit tests are sufficient here.

[keymanapp-test-bot]

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

• Android
  • Keyman for Android apk
  • FirstVoices Keyboards for Android apk
  • KMSample1 apk
  • KMSample2 apk
• Developer
  • Keyman Developer
  • Compiler Regression Tests
  • kmcomp.zip
• iOS
  • Keyman for iOS (simulator image)
  • FirstVoices Keyboards for iOS (simulator image)
  • TestFlight internal PR build version - 16.0.69 (0.7345.6999)
• Keyboards
  • Test Keyboards
• Web
  • KeymanWeb Test Home
• Windows
  • Keyman for Windows
  • FirstVoices Keyboards for Windows

[jahorton]

Compare:

keyman/web/source/dom/targets/touchAlias.ts

Lines 53 to 67 in 1a7ad09

	deleteCharsBeforeCaret(dn: number): void {
	if(dn > 0) {
	let curText = this.getTextBeforeCaret();
	if(this.getDeadkeyCaret() < dn) {
	dn = this.getDeadkeyCaret();
	}
	this.adjustDeadkeys(-dn);
	this.root.setTextBeforeCaret(curText.kmwSubstring(0, this.root.getTextCaret() - dn));
	}
	}
	insertTextBeforeCaret(s: string): void {
	this.adjustDeadkeys(s._kmwLength());
	this.root.setTextBeforeCaret(this.root.getTextBeforeCaret() + s);
	}

Touch-alias elements are relatively similar to Mocks - both are programmatically managed and modeled by KMW. Note how the former has code updating deadkeys... while the latter doesn't. This was almost certainly a simple oversight that only just now got caught.

[mcdurdin]

Why s._kmwLength() as opposed to s.kmwLength()?

[jahorton]

Most of the calls in that file use _kmwLength, so it follows the same pattern.

[jahorton]

Hmm... there are a few notable issues with element focus management cropping up in the unit tests. I'll need to investigate exactly what's causing the issues.

The gist I'm seeing so far: whenever KMW decides it needs to re-focus on the should-remain-activeElement, sometimes that very act actually unsets the activeElement - and that's breaking the affected unit tests.

[mcdurdin]

Subsequent rules would fail to match any deadkeys still in the context after the caret.

How can a rule match a deadkey in the context after the caret? Rules always match pre-caret. Furthermore, a deadkey that was after the caret is effectively gone, because caret movement (using mouse, cursor keys, etc) triggers an invalidation of context which drops all deadkey state. I'm worried that there are some incorrect assumptions about how deadkeys work?

[mcdurdin]

I assume the unit test fixes are actually cleanup for other issues? It might be good to split them out into separate PRs.

The code change to domEventHandlers.ts, however, seems quite unrelated to the deadkey handling. Could we track that separately please?

[mcdurdin]

I've been thinking about this a bit more. This is a change to keystroke processing. Generally, I think it is super important that we do user testing (and if possible automated tests also) on any change to the keystroke processing code. So I think we need to:

1. provide an example of how it was going wrong (e.g. with a specific keyboard by comparison with Keyman 14 for Windows, and/or by comparison with KeymanWeb 10*)
2. do a user test on the change.

• Keyman 14 for Windows because it used the old Windows-core which was effectively our reference implementation. Keyman 15 for Windows is very close to that but still ironing out a few edge cases.
• KeymanWeb 10 because that was effectively our ref impl for web, and an old "known-good" version. Could alternatively use 2.x series. Not that it was 100% the same as Windows, nor necessarily 100% correct, but at least we need to demonstrate where this was diverging with Mocks.

[jahorton]

I've been thinking about this a bit more. This is a change to keystroke processing.

Uh, how exactly? This is a bug fix, not a change. The implementation of the Mock type (within outputTarget.ts) was incomplete in this regard, mismatching its close parallel with touch-alias element handling.

[jahorton]

... And I merged in #7346 before I realized it was based on this PR, not master. Oh well.

[mcdurdin]

This is a bug fix, not a change

A bug fix is a change. A change in the right direction, certainly, but still a change.

[jahorton]

Subsequent rules would fail to match any deadkeys still in the context after the caret.

How can a rule match a deadkey in the context after the caret? Rules always match pre-caret. Furthermore, a deadkey that was after the caret is effectively gone, because caret movement (using mouse, cursor keys, etc) triggers an invalidation of context which drops all deadkey state. I'm worried that there are some incorrect assumptions about how deadkeys work?

This particular function has existed since before KMW 10 and is the reason the unit test exists in the first place:

KMW 14+:

keyman/common/web/keyboard-processor/src/text/deadkeys.ts

Lines 137 to 154 in 497a861

	/**
	* Function adjustPositions (formerly _DeadkeyAdjustPos)
	* Scope Private
	* @param {number} Lstart start position in context
	* @param {number} Ldelta characters to adjust by
	* Description Adjust saved positions of deadkeys in context
	*/
	adjustPositions(Lstart: number, Ldelta: number): void {
	if(Ldelta == 0) {
	return;
	}
	for(let dk of this.dks) {
	if(dk.p > Lstart) {
	dk.p += Ldelta;
	}
	}
	}

KMW 10:

keyman/web/source/kmwcallback.ts

Lines 1250 to 1263 in f16872a

	/**
	* Function _DeadkeyAdjustPos
	* Scope Private
	* @param {number} Lstart start position in context
	* @param {number} Ldelta characters to adjust by
	* Description Adjust saved positions of deadkeys in context
	*/
	_DeadkeyAdjustPos(Lstart: number, Ldelta: number): void {
	for(let dk of this._DeadKeys) {
	if(dk.p > Lstart) {
	dk.p += Ldelta;
	}
	}
	}

Admittedly, I've always been a bit fuzzy on exactly what sort of case this was meant to address. I found the most straightforward way to double-check that its functionality was maintained uninterrupted was via a unit test that happens to operate in violation of your intuition above. As long as the tested case is clearly handled by this code block, I figure its behavior will be maintained as intended, regardless of whether or not the exact specifics of its test-usage perfectly match an actual use-case for keyboards.

Therefore:

1. provide an example of how it was going wrong (e.g. with a specific keyboard by comparison with Keyman 14 for Windows, and/or by comparison with KeymanWeb 10*)

If you can provide me with a keyboard that motivated the existence of that code above... then I can fulfill your request... if there's also an existing lexical model to match such a keyboard.

Unfortunately, there are no notes regarding what old issue it was intended to address, let alone which keyboards were affected. It's hard to trace back into history I wasn't present for (re: the original motivation for this function), and there are a lot of keyboards in the repo.

[jahorton]

1. provide an example of how it was going wrong (e.g. with a specific keyboard by comparison with Keyman 14 for Windows, and/or by comparison with KeymanWeb 10*)

A minor followup on this point: due to the specific nature of how bugs related to this could surface, this request is literally impossible with regard to KMW 10. KMW 10 did not support predictive text. This bug is on Mocks, an input-receiving type that didn't even exist until KMW 12, and is currently triggered in production only by predictive-text-prep operations.

[jahorton]

I assume the unit test fixes are actually cleanup for other issues? It might be good to split them out into separate PRs.

Well, about that.

This PR itself is cleanup for that other issue: see #7343. The "unit test fix" is that the test was pointing at a type we're about to deprecate in that PR, rather than the type it was always supposed to be testing.

Said unit-test fails once corrected without this PR's changes. My investigation into why it failed... is why this PR exists.

[mcdurdin]

Admittedly, I've always been a bit fuzzy on exactly what sort of case this was meant to address.

It's a long time ago, but I think it relates to IE's text selection support. I have a feeling it may be completely obsolete now. IE's selection management did not give an offset relative to start of text, so we had everything calculated as relative offsets to the 'selection' (caret if selection is empty). Hence, any changes to context required a recalculation of our cached deadkey offsets. However, once we have deadkey offsets in absolute positions, then deletion of content just requires deletion of deadkeys that fall to the right of the caret -- and that should already be working, of course.

We should do some careful testing, I guess, to see if this feeling of mine is correct, but if so, time for a little happy dance: simplifying complex logic is a Good Thing.

[jahorton]

Admittedly, I've always been a bit fuzzy on exactly what sort of case this was meant to address.

It's a long time ago, but I think it relates to IE's text selection support. I have a feeling it may be completely obsolete now. IE's selection management did not give an offset relative to start of text, so we had everything calculated as relative offsets to the 'selection' (caret if selection is empty). Hence, any changes to context required a recalculation of our cached deadkey offsets. However, once we have deadkey offsets in absolute positions, then deletion of content just requires deletion of deadkeys that fall to the right of the caret -- and that should already be working, of course.

We should do some careful testing, I guess, to see if this feeling of mine is correct, but if so, time for a little happy dance: simplifying complex logic is a Good Thing.

Sounds like something that should be a separate PR, though - that involves far more substantial changes than what this PR brings.

I'm also a bit curious if we should maintain the relative offset bit anyway for things like #5258.

[mcdurdin]

Well, about that.

This PR itself is cleanup for that other issue: see #7343. The "unit test fix" is that the test was pointing at a type we're about to deprecate in that PR, rather than the type it was always supposed to be testing.

Said unit-test fails once corrected without this PR's changes. My investigation into why it failed... is why this PR exists.

🤯

So confused... can you explain again? I don't understand why you are making changes against master that relate to a completely separate PR's changes? Why aren't those changes in #7343 or at least in a PR targeting #7343?

[mcdurdin]

I'm also a bit curious if we should maintain the relative offset bit anyway for things like #5258.

I'm okay with removing it entirely and then revisiting one day in full for that issue, rather than attempting to build on the currently shaky foundation for relative offsets.

[jahorton]

So confused... can you explain again? I don't understand why you are making changes against master that relate to a completely separate PR's changes? Why aren't those changes in #7343 [...]

It already has quite the changeset. This is orthogonal enough to split off as a separate thing, which should simplify the overall reviewing process.

Technically, what this addresses actually been a hidden bug for quite a while, as the unit test wasn't pointing at the right test target and allowed the related unit tests to pass when they should have failed.

or at least in a PR targeting #7343?

Because it's actually orthogonal enough to #7343 to implement first without any ill effects whatsoever. And not only that - even if we decided to change our minds tomorrow and drop all work on #7343, I believe this is its own bug that should still be fixed. Granted, there's the strong chance for a further-simplifying followup as noted in some of your comments above.

[mcdurdin]

OK, LGTM I think. I think the original problem I had here when reviewing was confusion to the other fixes that had crept in with the actual fix, which is in two parts: 1) the unit test was never actually testing Mocks, and 2) adjustDeadkeys wasn't called.

I'm going to call out for hopeful removal of adjustDeadkeys (and any correlated IE-specific adjustment code, if it exists) as a separate issue.

[keyman-server]

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