[api-minor] JS -- Add listener for sandbox events only if there are some actions #12546

calixteman · 2020-10-28T18:54:07Z

When no actions then set it to null instead of empty object
Even if a field has no actions, it needs to listen to events from the sandbox in order to be updated if an action changes something in it.

brendandahl

Looks good overall, but I'd like to hear if it's possible to address the comment.

web/base_viewer.js

Snuffleupagus

Can't we actually delay this completely until Annotation parsing actually occurs, since in most documents there's either no annotations present or at least no JavaScript actions present!?

That'd probably reduce the overall size of this, and also simplify the implementation (especially on the viewer side). One quick idea that I had, was to simply include this property in the annotation data itself such that you'll only need to pass around the enableScripting-value to the various viewer-components.

To illustrate my idea, perhaps something along these lines (purposely incomplete, but only to illustrate my idea):

diff --git a/src/core/annotation.js b/src/core/annotation.js
index 252e1529b..f28806726 100644
--- a/src/core/annotation.js
+++ b/src/core/annotation.js
@@ -62,13 +62,17 @@ class AnnotationFactory {
    *   instance.
    */
   static create(xref, ref, pdfManager, idFactory) {
-    return pdfManager.ensureCatalog("acroForm").then(acroForm => {
+    return Promise.all([
+      pdfManager.ensureCatalog("acroForm"),
+      pdfManager.ensureDoc("hasJSActions"),
+    ]).then(([acroForm, hasJSActions]) => {
       return pdfManager.ensure(this, "_create", [
         xref,
         ref,
         pdfManager,
         idFactory,
         acroForm,
+        hasJSActions,
       ]);
     });
   }
@@ -76,7 +80,14 @@ class AnnotationFactory {
   /**
    * @private
    */
-  static _create(xref, ref, pdfManager, idFactory, acroForm) {
+  static _create(
+    xref,
+    ref,
+    pdfManager,
+    idFactory,
+    acroForm,
+    hasJSActions = false
+  ) {
     const dict = xref.fetchIfRef(ref);
     if (!isDict(dict)) {
       return undefined;
@@ -97,6 +108,7 @@ class AnnotationFactory {
       id,
       pdfManager,
       acroForm: acroForm instanceof Dict ? acroForm : Dict.empty,
+      hasJSActions,
     };
 
     switch (subtype) {
@@ -281,6 +293,7 @@ class Annotation {
       modificationDate: this.modificationDate,
       rect: this.rectangle,
       subtype: params.subtype,
+      documentHasJSActions: params.hasJSActions,
     };
 
     this._fallbackFontDict = null;
diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js
index effa122b5..4e07676fe 100644
--- a/src/display/annotation_layer.js
+++ b/src/display/annotation_layer.js
@@ -478,7 +478,8 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
         event.target.setSelectionRange(0, 0);
       });
 
-      if (this.data.actions) {
+      // Assuming that the `enableScripting`-value is passed in:
+      if (this.enableScripting && this.data.documentHasJSActions) {
         element.addEventListener("updateFromSandbox", function (event) {
           const data = event.detail;
           if ("value" in data) {

calixteman · 2020-10-30T09:25:29Z

I think this is a very good idea: I like it, thank you.

Snuffleupagus

Looks much better this way; I've added a few comments, after which this should be good to go (assuming all tests pass of course :-)

src/core/annotation.js

src/core/document.js

src/display/api.js

web/base_viewer.js

web/pdf_page_view.js

src/display/annotation_layer.js

timvandermeij

Looks good but I'd really like to see the property comment addressed since it doesn't feel like the right place to put it.

src/core/annotation.js

src/core/document.js

web/pdf_page_view.js

* When no actions then set it to null instead of empty object * Even if a field has no actions, it needs to listen to events from the sandbox in order to be updated if an action changes something in it.

brendandahl · 2020-11-09T21:30:18Z

/botio test

pdfjsbot · 2020-11-09T21:30:19Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/f4cd0e18f636d93/output.txt

pdfjsbot · 2020-11-09T21:30:20Z

From: Bot.io (Windows)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://3.101.106.178:8877/e38ecbda1eeccf1/output.txt

pdfjsbot · 2020-11-09T21:55:14Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/f4cd0e18f636d93/output.txt

Total script time: 24.90 mins

Font tests: Passed
Unit tests: FAILED
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/f4cd0e18f636d93/reftest-analyzer.html#web=eq.log

pdfjsbot · 2020-11-09T21:58:37Z

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/e38ecbda1eeccf1/output.txt

Total script time: 28.28 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/e38ecbda1eeccf1/reftest-analyzer.html#web=eq.log

timvandermeij · 2020-11-09T23:04:13Z

/botio-linux preview

pdfjsbot · 2020-11-09T23:04:14Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/4ae43562e91da07/output.txt

pdfjsbot · 2020-11-09T23:08:24Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/4ae43562e91da07/output.txt

Total script time: 4.14 mins

Published

Viewer: http://54.67.70.0:8877/4ae43562e91da07/web/viewer.html
Viewer (ES5): http://54.67.70.0:8877/4ae43562e91da07/es5/web/viewer.html

timvandermeij · 2020-11-09T23:27:04Z

Looks good to me too now. Thank you!

timvandermeij added the core label Oct 28, 2020

calixteman mentioned this pull request Oct 29, 2020

Miscellaneous (small) improvements in src/core/annotation.js #12552

Merged

brendandahl changed the title ~~JS -- Add listener for sandbox events only if there are some actions~~ [api-minor] JS -- Add listener for sandbox events only if there are some actions Oct 29, 2020

brendandahl suggested changes Oct 29, 2020

View reviewed changes

web/base_viewer.js Outdated Show resolved Hide resolved

Snuffleupagus requested changes Oct 30, 2020

View reviewed changes

calixteman force-pushed the hasjs branch 3 times, most recently from 10d1ce7 to 10d1965 Compare November 3, 2020 12:14

brendandahl approved these changes Nov 5, 2020

View reviewed changes

Snuffleupagus reviewed Nov 5, 2020

View reviewed changes

calixteman force-pushed the hasjs branch from 10d1965 to 8fc1534 Compare November 6, 2020 19:23

timvandermeij reviewed Nov 6, 2020

View reviewed changes

src/core/annotation.js Outdated Show resolved Hide resolved

src/core/document.js Outdated Show resolved Hide resolved

calixteman force-pushed the hasjs branch 2 times, most recently from 4aeff66 to de4b0d5 Compare November 9, 2020 10:29

Snuffleupagus requested changes Nov 9, 2020

View reviewed changes

web/pdf_page_view.js Outdated Show resolved Hide resolved

JS -- Add listener for sandbox events only if there are some actions

a527989

* When no actions then set it to null instead of empty object * Even if a field has no actions, it needs to listen to events from the sandbox in order to be updated if an action changes something in it.

calixteman force-pushed the hasjs branch from de4b0d5 to a527989 Compare November 9, 2020 17:38

timvandermeij approved these changes Nov 9, 2020

View reviewed changes

timvandermeij merged commit 1c17e07 into mozilla:master Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-minor] JS -- Add listener for sandbox events only if there are some actions #12546

[api-minor] JS -- Add listener for sandbox events only if there are some actions #12546

calixteman commented Oct 28, 2020

brendandahl left a comment

Snuffleupagus left a comment

calixteman commented Oct 30, 2020

Snuffleupagus left a comment

timvandermeij left a comment

brendandahl commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

timvandermeij commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

timvandermeij commented Nov 9, 2020

[api-minor] JS -- Add listener for sandbox events only if there are some actions #12546

[api-minor] JS -- Add listener for sandbox events only if there are some actions #12546

Conversation

calixteman commented Oct 28, 2020

brendandahl left a comment

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Oct 30, 2020

Snuffleupagus left a comment

Choose a reason for hiding this comment

timvandermeij left a comment

Choose a reason for hiding this comment

brendandahl commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Nov 9, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Nov 9, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Nov 9, 2020

From: Bot.io (Windows)

Failed

timvandermeij commented Nov 9, 2020

pdfjsbot commented Nov 9, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Nov 9, 2020

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Nov 9, 2020