-
Notifications
You must be signed in to change notification settings - Fork 10k
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.
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
AcroForm: Add support for ResetForm action #14083
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite this being marked as a "draft", I figured it couldn't hurt to add a few comments based on cursory look. The overall approach looks reasonable though :-)
@Snuffleupagus, when you'll have time, could you review this PR please ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me, with the comments addressed and passing tests; thank you!
src/display/annotation_layer.js
Outdated
if (!this._fieldObjects) { | ||
warn( | ||
`_bindResetFormAction - "resetForm" action not supported, ` + | ||
"ensure that the `fieldObjects` parameter is provided." | ||
); | ||
return; | ||
} | ||
|
||
link.className = "internalLink"; | ||
const otherClickAction = link.onclick; | ||
link.onclick = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the data, you may or may not have set either the href or className at this point. Furthermore, you also need to ensure that a dummy onclick
is added in the fallback-case.
All-in-all, can you please make the following changes which I believe should cover all of the cases that could possibly happen here?
if (!this._fieldObjects) { | |
warn( | |
`_bindResetFormAction - "resetForm" action not supported, ` + | |
"ensure that the `fieldObjects` parameter is provided." | |
); | |
return; | |
} | |
link.className = "internalLink"; | |
const otherClickAction = link.onclick; | |
link.onclick = () => { | |
const otherClickAction = link.onclick; | |
if (!otherClickAction) { | |
link.href = this.linkService.getAnchorUrl(""); | |
} | |
link.className = "internalLink"; | |
if (!this._fieldObjects) { | |
warn( | |
`_bindResetFormAction - "resetForm" action not supported, ` + | |
"ensure that the `fieldObjects` parameter is provided." | |
); | |
if (!otherClickAction) { | |
link.onclick = () => false; | |
} | |
return; | |
} | |
link.onclick = () => { |
src/display/annotation_layer.js
Outdated
} | ||
for (const fields of Object.values(this._fieldObjects)) { | ||
for (const field of fields) { | ||
if (include === fieldIds.has(field.id)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Let's swap the order in this comparison, since the "constant" value should usually be on the right. (Note that a comparison such as if (5 === someVar) { ... }
would actually be a linting failure.)
src/display/annotation_layer.js
Outdated
@@ -1217,6 +1338,13 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement { | |||
} | |||
} | |||
|
|||
selectElement.addEventListener("resetform", event => { | |||
const defaultValue = this.data.defaultFieldValue; | |||
Array.prototype.forEach.call(selectElement.options, option => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, I believe that you should be able to iterate through the options with a for...of
loop instead. Assuming that works, let's do that instead since it feels nicer than having to use Array.forEach in this way.
- it aims to fix mozilla#12721. - Thanks to PR mozilla#14023, we've now the fieldObjects in the annotation layer so we can easily map fields names on their id if needed. - Reset values in the storage, in the JS sandbox and in the visible html elements.
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/c8f732f3159a11d/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/1f29a21f3cbc57d/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/1f29a21f3cbc57d/output.txt Total script time: 2.81 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/c8f732f3159a11d/output.txt Total script time: 6.13 mins
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/fb8c254b0a5ffb2/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/4a30439f4249ed7/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/fb8c254b0a5ffb2/output.txt Total script time: 3.77 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/4a30439f4249ed7/output.txt Total script time: 6.27 mins
|
document.getElementsByName
lookups in the AnnotationLayer (issue 14003) #14023, we've now the fieldObjects in the annotation layer so we can easily map fields names on their id if needed.