Fix AForm Validation for fields with non-digit characters

[cincodenada]

This is a proposed fix for #14306 - I believe the core issue is that mergeChanges() tries to use the keyboard event to predict what the value of the field will be after the change is applied, and gets it wrong for changes that don't involve an explicit selection, at least in my environment.

The cause here is that mergeChanges() seems to assume that character insertions/appends will come with selStart and selEnd set, and thus it can just treat them like any other selection replacement. That isn't the case on in my environment, where appends (as well as single-character insertions and deletions) have selStart and selEnd set to -1. This results in the predicted value of the field being a single character: the character being appended.

This ends up not totally breaking things - for validations that will pass if each individual character typed is a valid first character in the field (since that's what it gets interpreted as), things are mostly fine - a ZIP code can be entered just fine, although length is not constrained until the field is exited.

But for fields with hyphens in them, for instance, pdf.js won't allow you to type the hyphen (because "-" by itself is not the beginning of a valid SSN), and then when the field is blurred, the final validation runs, and complains about the missing hyphens.

This PR updates mergeChanges() so that if selStart and selEnd are -1 it assumes the event is either an append and predicts the result as value + change, or a delete and predicts the result as value.slice(0, -1). This fixes the most common cases, but still leaves room for some very strange behavior if the user inserts or deletes characters in the middle.

Because we don't have selStart and selEnd, I don't see any way of distinguishing between and append an an insertion, or a deletion at the end of the value vs an internal deletion. In this PR I just assume that changes are happening at the end of the string, but that means we'll be quite wrong about internal deletions/insertions, and if someone goes back and inserts a character we'll predict that it was added at the end of the string.

Fortunately that prediction isn't persisted, and internal insertions/deletions should be rare, so I think it's worth this most-of-the-way fix that should cover the majority of cases. If there is some way to determine where single-character changes are being made, from the event or some other source, that would of course be optimal, but I didn't see one, and I don't know enough about how these events are generated to know if it's something we could add ourselves, or if it's something in some PDF standard somewhere.

[cincodenada]

I found the contribution guide I had somehow glossed over the first time and ran unit tests, and fixed up some expectations - I plan to circle back and add some more unit tests to cover the additional cases I ran into here as well.

[calixteman]

Where did you find this information ?
In the specs (see http://dev.datalogics.com/cookbook/document/AcrobatDC_js_api_reference.pdf#page=381&zoom=auto,-214,751) about selStart:
The starting position of the current text selection during a keystroke event.
I just tried in Acrobat to add text in textfield and print the changes.

• I entered: abcdef
• I selected bcd and I pressed g
  and I get the following output:

change=a, selStart=0, selEnd=0
change=b, selStart=1, selEnd=1
change=c, selStart=2, selEnd=2
change=d, selStart=3, selEnd=3
change=e, selStart=4, selEnd=4
change=f, selStart=5, selEnd=5
change=g, selStart=1, selEnd=4

My understanding is that the selStart is the position of the caret before the change.
When there is no selection, selEnd is equals to selStart else it's the position of the end of the selection.

[cincodenada]

I found it through some testing and logging with pdf.js, and seeing what the events being created actually looked like, and they had -1 for single-character edits.

Like I said (maybe here or on the bug, I don't remember), I wasn't sure whether the behavior of these events was in a specification or something somewhere or not - it looks like it is, which I guess means the bug is that something else in pdf.js isn't populating selStart/selEnd for these events? That would also make sense, and would solve the edge cases as well.

[calixteman]

All the changes in this file are very likely wrong.

[cincodenada]

These changes may be wrong in that they are against spec, but they are correct in that they are what pdf.js is emitting currently. It sounds like pdf.js's behavior in creating these events is the actual bug though.

[cincodenada]

So to be clear, I agree these will probably need to end up being reverted, hopefully replaced by some tests (or fixed test) of the generation of events for single-character edits, to validate that they are populating selStart/selEnd like this further up in the process.

[calixteman]

my bad... thank you to point that out.

[Snuffleupagus]

Nit: Here, and everywhere in the patch, please make sure to always end all sentences with a period.

Also, please keep https://github.com/mozilla/pdf.js/wiki/Squashing-Commits in mind when updating the patch.

[calixteman]

The main issue is here:
https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js#L1072
elementData.beforeInputSelectionRange is null when it shouldn't.

There is another problem here:
https://github.com/mozilla/pdf.js/blob/master/src/scripting_api/event.js#L29-L30
where we should have ?? instead of ||.

[calixteman]

I think the rhs of || is useless but definitely the lhs is a way better like this.

[cincodenada]

The RHS is necessary because we validate every time the field changes. For final validation you're right that it's unnecessary, but for the validation-during-typing, without the RHS if someone starts typing (888) the validation will stop them - for some reason the first character seems to be ignored for validation-during-typing, but the second parenthesis will cause an error:

This is because (888) only has 5 characters, so this code selects the second pattern (999-9999), and (888) does not match the beginning of the that pattern. Instead, in that case we want to select the first pattern to match against, which is what the RHS here is doing.

I can add some tests around this too.

[calixteman]

It makes sense, +1.

[calixteman]

And I just checked MDN:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/beforeinput_event
and beforeinput is now available, so we should likely use it now instead of this workaround.

[cincodenada]

The main issue is here:
https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js#L1072
elementData.beforeInputSelectionRange is null when it shouldn't.

Okay, so yeah sounds like this code will work fine if selStart/selEnd is always present, which it sounds like is the behavior of Acrobat. So then, the root bug is further up the stack, where we're not passing down selStart/selEnd for edits without an explicit selection. Are you on the trail of a fix for that, or should I try digging in further?

There is another problem here:
https://github.com/mozilla/pdf.js/blob/master/src/scripting_api/event.js#L29-L30
where we should have ?? instead of ||.

Oh, good catch - I looked at that code and didn't even notice but you're right, that code is dropping zeroes improperly. I wonder if that's connected to the reason the first character isn't validating during typing...

[cincodenada]

In summary: I'm happy to continue digging here and move this PR towards integrating beforeInput and getting a fix in higher up where it belongs, but I don't want to do duplicate work, so I can also just revert the workaround bits and this PR can just be the smaller fixes around phone numbers. Either way I'll squash things down once things are settled - do y'all prefer a small number of atomic commits, or one commit per PR?

[calixteman]

I'm fine with a PR to only modify aform.js and it'd be nice if you could add a test to check that format is fine.
It could something similar to:

pdf.js/test/unit/scripting_spec.js

Lines 791 to 828 in a2c380c

	it("should not validate a number on a keystroke event", async () => {
	const refId = getId();
	const data = {
	objects: {
	field: [
	{
	id: refId,
	value: "",
	actions: {
	Validate: [
	`AFNumber_Keystroke(null, 0, null, null, null, null);`,
	],
	},
	type: "text",
	name: "MyField",
	},
	],
	},
	appInfo: { language: "en-US", platform: "Linux x86_64" },
	calculationOrder: [],
	dispatchEventName: "_dispatchMe",
	};
	sandbox.createSandbox(data);
	await sandbox.dispatchEventInSandbox({
	id: refId,
	value: "123s456.789",
	name: "Keystroke",
	willCommit: true,
	});
	expect(send_queue.has("alert")).toEqual(true);
	expect(send_queue.get("alert")).toEqual({
	command: "alert",
	value:
	"The value entered does not match the format of the field [ MyField ]",
	});
	});
	});

where an alert is handled and to:

pdf.js/test/unit/scripting_spec.js

Lines 376 to 403 in a2c380c

	it("should trigger a Keystroke event and change it", async () => {
	const refId = getId();
	const data = {
	objects: {
	field: [
	{
	id: refId,
	value: "",
	actions: {
	Keystroke: [`event.change = "a";`],
	},
	type: "text",
	},
	],
	},
	appInfo: { language: "en-US", platform: "Linux x86_64" },
	calculationOrder: [],
	};
	sandbox.createSandbox(data);
	await sandbox.dispatchEventInSandbox({
	id: refId,
	value: "hell",
	name: "Keystroke",
	willCommit: false,
	change: "o",
	selStart: 4,
	selEnd: 4,
	});

where a text is entered (using selStart & selEnd).
Once that stuff is fixed then I think you could work on a second PR to add beforeinput stuff.

[calixteman]

@cincodenada, any progress ?
If you don't have time to work on that stuff, please tell it and I'll fix the issues myself.

[calixteman]

I push this PR: #14429
in order to address issues found by @cincodenada.
@cincodenada I hope you're well and thank you for your help.