fix(web): add "result sanitizing" wrapper for custom wordbreakers

[jahorton]

Fixes: #16587
Fixes: #16585

Build-bot: skip release:web,android,ios

🚧 Current known issues 🚧

• On mon_anonta, the second default suggestion, when applied, will often leave the banner unaffected. Re-tapping the suggestion will yield a blank banner and duplicate the suggestion - both of which are undesired behaviors.

User Testing

TEST_KHMER_REPRO: Using Keyman for Android, the khmer_angkor keyboard, and the sil.km.gcc model, verify that #16587's repro no longer triggers the error.

1. Select ការ
2. press spacebar
3. Verify that predictions do not show words starting with ការ aside from ការ itself.
4. Verify that applying a prediction does not replace ការ.

TEST_KHMER_PREDICTIONS: Using Keyman for Android, the khmer_angkor keyboard, and the sil.km.gcc model, verify that text predictions operate smoothly.

• Use it for about 3 minutes and report any unexpected issues you encounter.
• Add double-spaces on occasion and verify that this does not impact predictive text behavior.

TEST_ANONTA: Using Keyman for Android and the mon_anonta keyboard, verify that predictions operate smoothly.

• Try repeating the same two characters as the start of a word multiple times and report any issues encountered - both in generated predictions and when applying the suggestions after those first two characters.
• Add double-spaces on occasion and verify that this does not impact predictive text behavior.

[keymanapp-test-bot]

User Test Results

Test specification and instructions

• ⬜ TEST_KHMER_REPRO (OPEN)
• ⬜ TEST_KHMER_PREDICTIONS (OPEN)
• ⬜ TEST_ANONTA (OPEN)

Results Template

# Test Results

* **TEST_KHMER_REPRO (OPEN):** notes
* **TEST_KHMER_PREDICTIONS (OPEN):** notes
* **TEST_ANONTA (OPEN):** notes

Test Artifacts

• Android
  • Keyman for Android apk
  • FirstVoices Keyboards for Android apk
  • KeyboardHarness apk
  • KMSample1 apk
  • KMSample2 apk
• iOS
  • Keyman for iOS (simulator image)
  • FirstVoices Keyboards for iOS (simulator image)
  • TestFlight internal PR build version - 19.0.288 (0.16611.16423)
• Web
  • KeymanWeb Test Home

[mcdurdin]

I appreciate the work you have done to find a solution to this.

But I am now uncertain if this the best way forward. Workarounds have a tendency to linger and cause long-term pain when trying to move forward. There is a delicate balance between maintaining interface back-compat and fixing bugs in interface clients.

Is the documentation correct? I think it may be a little ambiguous on whether non-word tokens should be included in spans?

But it does seem like the two models which are experiencing issues are implementing the custom wordbreaker incorrectly, according to the documentation. So, if that is that case we should fix them rather than working around them.

The documentation states:

The function must return zero or more Span objects. The spans, representing an indivisible span of text, must be in ascending order of their start point, and they must be non-overlapping.

We could add a sentence to state that the spans must cover 100% of the text -- that is, there must be no gaps or overlaps. If that is what we need?

Hard call.

[mcdurdin]

It'd be good to be clear on what "wordbreaker whitespace handling and span indexing issues" actually are -- this is a bit vague.

[mcdurdin]

As far as I can tell, length is required in the documentation. So, in this case we should fix the model rather than adding a workaround.

There is a delicate balance between maintaining interface back-compat and fixing bugs in interface clients.

[jahorton]

I believe that fortunately, .length itself isn't critical. I could probably remove that specific section and things would proceed perfectly fine.

[jahorton]

I appreciate the work you have done to find a solution to this.

But I am now uncertain if this the best way forward. Workarounds have a tendency to linger and cause long-term pain when trying to move forward. There is a delicate balance between maintaining interface back-compat and fixing bugs in interface clients.

Is the documentation correct? I think it may be a little ambiguous on whether non-word tokens should be included in spans?

I suppose our guide documentation is ambiguous. As it was, I had to double-check things against our model-types TS definitions as declared here:

keyman/common/lexical-model-types/index.d.ts

Lines 286 to 323 in 4211b46

	/**
	* A simple word breaking function takes a phrase, and splits it into "words",
	* for whatever definition of "word" is usable for the language model.
	*
	* For example:
	*
	* getText(breakWordsEnglish("Hello, world!")) == ["Hello", "world"]
	* getText(breakWordsCree("ᑕᐻ ᒥᔪ ᑮᓯᑲᐤ ᐊᓄᐦᐨ᙮")) == ["ᑕᐻ", "ᒥᔪ ᑮᓯᑲᐤ""", "ᐊᓄᐦᐨ"]
	* getText(breakWordsJapanese("英語を話せますか？")) == ["英語", "を", "話せます", "か"]
	*
	* Not all language models take in a configurable word breaking function.
	*
	* @returns an array of spans from the phrase, in order as they appear in the
	* phrase, each span which representing a word.
	*/
	declare interface WordBreakingFunction {
	// invariant: span[i].end <= span[i + 1].start
	// invariant: for all span[i] and span[i + 1], there does not exist a span[k]
	// where span[i].end <= span[k].start AND span[k].end <= span[i + 1].start
	(phrase: string): Span[];
	}
	/**
	* A span of text in a phrase. This is usually meant to represent words from a
	* pharse.
	*/
	declare interface Span {
	// invariant: start < end (empty spans not allowed)
	readonly start: number;
	// invariant: end > end (empty spans not allowed)
	readonly end: number;
	// invariant: length === end - start
	readonly length: number;
	// invariant: text.length === length
	// invariant: each character is BMP UTF-16 code unit, or is a high surrogate
	// UTF-16 code unit followed by a low surrogate UTF-16 code unit.
	readonly text: string;
	}

(This is linked to directly by the page you linked in the comment above.)

Note that the examples do not produce spans for would-be whitespace tokens. Our Unicode-default wordbreaker also does not do this.

But it does seem like the two models which are experiencing issues are implementing the custom wordbreaker incorrectly, according to the documentation. So, if that is that case we should fix them rather than working around them.

The documentation states:

The function must return zero or more Span objects. The spans, representing an indivisible span of text, must be in ascending order of their start point, and they must be non-overlapping.

For ait.mnw.mon, it does not properly index spans because it naively uses the first context's first instance matching the specified substring - which is problematic should the same substring appear multiple times within the context's windows range. The substring doesn't even have to be a token prefix.

Its naive handling of whitespace is also problematic.

sil.km.gcc's wordbreaker is much smarter about the span-token indexing issue, but it still fails to handle whitespace appropriately.

We could add a sentence to state that the spans must cover 100% of the text -- that is, there must be no gaps or overlaps. If that is what we need?

Then our existing provided wordbreakers also don't meet that spec. Again, even our default Unicode wordbreaker currently does not report the whitespace tokens; we re-derive such spans in post.

That said, I'd rather it did provide the whitespace tokens.

Hard call.