Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AForm Validation for fields with non-digit characters #14307

Closed
wants to merge 6 commits into from
Closed

Fix AForm Validation for fields with non-digit characters #14307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5622906
Use parameter instead of closure variable
cincodenada Nov 24, 2021
989dd0f
Udpate artificial events in tests to reflect my testing
cincodenada Nov 24, 2021
b6e1309
Update mergeChanges() to treat appends as appends
cincodenada Nov 24, 2021
cc8da7a
Handle deletes as well
cincodenada Nov 24, 2021
6b73063
Accept a finished 8-digit phone number
cincodenada Nov 24, 2021
e9e853f
Adjust test expectations to match
cincodenada Nov 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/scripting_api/aform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,15 +486,15 @@ class AForm {
]);

function _checkValidity(_value, _cMask) {
for (let i = 0, ii = value.length; i < ii; i++) {
const mask = _cMask.charAt(i);
const char = _value.charAt(i);
const checker = checkers.get(mask);
for (let i = 0, ii = _value.length; i < ii; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const maskChar = _cMask.charAt(i);
const valueChar = _value.charAt(i);
const checker = checkers.get(maskChar);
if (checker) {
if (!checker(char)) {
if (!checker(valueChar)) {
return false;
}
} else if (mask !== char) {
} else if (maskChar !== valueChar) {
return false;
}
}
Expand Down Expand Up @@ -564,7 +564,7 @@ class AForm {
event.change.length +
event.selStart -
event.selEnd;
if (finalLen >= 8) {
if (finalLen > 8 || event.value[0] === "(") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@cincodenada cincodenada Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screen Shot 2021-11-24 at 3 34 02 PM

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, +1.

formatStr = "(999) 999-9999";
} else {
formatStr = "999-9999";
Expand Down
21 changes: 21 additions & 0 deletions src/scripting_api/event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,32 @@ class EventDispatcher {
this._document.obj._eventDispatcher = this;
}

/*
* Take an event object and reconstruct what we think the result will be once
* the change is applied, so we can validate against it and cancel the event
* if need be
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* TODO: Given the event info we currently have, we can't determine where in
* the value a single-character insertion/deletion happened. For now we just
* assume they happen at the end of the string, which is by far the most
* common case, but this leaves some edge cases, see issue #14307.
*/
mergeChange(event) {
let value = event.value;
if (typeof value !== "string") {
value = value.toString();
}
// If there was no selection, it's a single-character change
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@cincodenada cincodenada Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (event.selStart === -1 && event.selEnd === -1) {
if (event.change === "") {
// Empty change indicates a deletion
return value.slice(0, -1);
}
// Otherwise, assume it's an append
return value + event.change;
}

// Otherwise, splice in the change to replace the selection
const prefix =
event.selStart >= 0 ? value.substring(0, event.selStart) : "";
const postfix =
Expand Down
30 changes: 15 additions & 15 deletions test/unit/scripting_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,15 +361,15 @@ describe("Scripting", function () {
name: "Keystroke",
willCommit: false,
change: "o",
selStart: 4,
selEnd: 4,
selStart: -1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes in this file are very likely wrong.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

selEnd: -1,
});

expect(send_queue.has(refId)).toEqual(true);
expect(send_queue.get(refId)).toEqual({
id: refId,
value: "hell",
selRange: [4, 4],
selRange: [-1, -1],
});
});

Expand Down Expand Up @@ -398,8 +398,8 @@ describe("Scripting", function () {
name: "Keystroke",
willCommit: false,
change: "o",
selStart: 4,
selEnd: 4,
selStart: -1,
selEnd: -1,
});

expect(send_queue.has(refId)).toEqual(true);
Expand Down Expand Up @@ -1119,8 +1119,8 @@ describe("Scripting", function () {
change: "3",
name: "Keystroke",
willCommit: false,
selStart: 0,
selEnd: 0,
selStart: -1,
selEnd: -1,
});
expect(send_queue.has(refId)).toEqual(false);

Expand All @@ -1130,8 +1130,8 @@ describe("Scripting", function () {
change: "F",
name: "Keystroke",
willCommit: false,
selStart: 1,
selEnd: 1,
selStart: -1,
selEnd: -1,
});
expect(send_queue.has(refId)).toEqual(false);

Expand All @@ -1141,8 +1141,8 @@ describe("Scripting", function () {
change: "?",
name: "Keystroke",
willCommit: false,
selStart: 2,
selEnd: 2,
selStart: -1,
selEnd: -1,
});
expect(send_queue.has(refId)).toEqual(false);

Expand All @@ -1152,14 +1152,14 @@ describe("Scripting", function () {
change: "@",
name: "Keystroke",
willCommit: false,
selStart: 3,
selEnd: 3,
selStart: -1,
selEnd: -1,
});
expect(send_queue.has(refId)).toEqual(true);
expect(send_queue.get(refId)).toEqual({
id: refId,
value: "3F?",
selRange: [3, 3],
selRange: [-1, -1],
});

send_queue.delete(refId);
Expand All @@ -1169,7 +1169,7 @@ describe("Scripting", function () {
change: "0",
name: "Keystroke",
willCommit: true,
selStart: 3,
selStart: -1,
selEnd: 3,
});
expect(send_queue.has(refId)).toEqual(false);
Expand Down