chore(web): TransformUtils.insertWhitespace seems convoluted

[mcdurdin]

This seems a little convoluted.

• I'm really not sure exactly what we are testing here. It seems like you are looking for insert.match(/[\u0009...]$/)? Is it intended to match having a single whitespace character on the end of the text? If so, then the function name is not very clear. If not, then AFAICT the function is not correct.

• i modifier should not be necessary on the regex -- there's no case to be insensitive about 🤣.

• Can we use a character class for the regex match rather than a set of characters? (e.g. insert.match(/[\p{Z}\r\n]$/u). Note Chrome 50+ though (so we could have the shorter regex as a comment if we can't use it for back-compat reasons).

• Finally, can we have a javadoc comment rather than the in-function comment?

Originally posted by @mcdurdin in #7205 (comment)

[mcdurdin]

JH wrote:

Keep in mind that I did not edit this function at all. I simply extracted it from its prior home. From master:

keyman/common/web/lm-worker/src/model-compositor.ts

Lines 19 to 33 in 80d7ce7

	protected isWhitespace(transform: Transform): boolean {
	// Matches prefixed text + any instance of a character with Unicode general property Z* or the following: CR, LF, and Tab.
	let whitespaceRemover = /.*[\u0009\u000A\u000D\u0020\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u200b\u2028\u2029\u202f\u205f\u3000]/i;
	// Filter out null-inserts; their high probability can cause issues.
	if(transform.insert == '') { // Can actually register as 'whitespace'.
	return false;
	}
	let insert = transform.insert;
	insert = insert.replace(whitespaceRemover, '');
	return insert == '';
	}

This all dates back to #1851, and @eddieantonio was the one responsible for that regex: #1851 (comment)

Can't speak to why he didn't use a character class here, but I figure that there was some reason or other. For what it's worth, following that character class link and drilling down to the actual classes: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions/Character_Classes

I don't see a compatibility chart there. Now, there are other ways to search for this...

https://caniuse.com/mdn-javascript_builtins_regexp_property_escapes

Note the minimum Chrome version listed there - if desktop Chrome didn't support these property escapes before version 64, I doubt that mobile had it implemented at that time. This, to me, implies a strong chance that using them would break predictive text outright on non-updated Android 5.0 devices, at the least.

---

Where do you see a $ in the regex string here? Is it implied with the flags on the pattern? It's certainly not present in plain-text.

[jahorton]

Well... we ended up going ahead and addressing it ahead of time anyway. Took a few iterations to settle on the solution, though.

[mcdurdin]

Well... we ended up going ahead and addressing it ahead of time anyway.

Can you link the PR that addressed this?

[jahorton]

Addressed by #7241.