Acroform checkbox and radio controls #7664

benweet · 2016-09-22T15:21:47Z

Added fake checkboxes and radio buttons with a pinch of css.
Tested against http://www.cic.gc.ca/english/passport/forms/pdf/pptc153.pdf on Chrome and Firefox:

timvandermeij

This looks really good in general. I'm happy that you made this work with the new framework we're developing!

Aside from these review comments, I'd like to ask you to write unit tests for this. You can find these in test/unit/annotation_layer_spec.js. When this is done, I may be able to create a reference test for this patch.

timvandermeij · 2016-09-22T19:55:27Z

src/core/annotation.js

+  function ButtonWidgetAnnotation(params) {
+    WidgetAnnotation.call(this, params);
+
+    this.renderInteractiveForms = params.renderInteractiveForms;


If you rebase to the current master branch, you'll have this automatically because of https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js#L120, so you can remove this line.

timvandermeij · 2016-09-22T19:55:48Z

src/core/annotation.js

+      AnnotationFieldFlag.NOTOGGLETOOFF);
+    this.data.radio = this.hasFieldFlag(AnnotationFieldFlag.RADIO);
+    this.data.pushbutton = this.hasFieldFlag(AnnotationFieldFlag.PUSHBUTTON);
+    this.data.radiosInUnison = this.hasFieldFlag(


This does not appear to be used, so let's remove it for now to reduce the size of the patch.

timvandermeij · 2016-09-22T19:56:38Z

src/display/annotation_layer.js

@@ -76,6 +76,15 @@ AnnotationElementFactory.prototype =
        switch (fieldType) {
          case 'Tx':
            return new TextWidgetAnnotationElement(parameters);
+          case 'Btn':
+            if (!parameters.data.pushbutton) {


In the case of a pushbutton, we should add a warn call about this being unsupported.

Is this warning really necessary though, since it's going to be printed in the console regardless if forms are enabled or not?
Given that forms support is still some ways from being implemented, is it necessary to bother users about this?

Perhaps not really. I'm also fine with a comment such as // Push buttons are not implemented yet so that we at least know in the code that something is still missing here.

timvandermeij · 2016-09-22T19:57:08Z

src/display/annotation_layer.js

+ * @alias CheckboxWidgetAnnotationElement
+ */
+var CheckboxWidgetAnnotationElement =
+  (function CheckboxWidgetAnnotationElementClosure() {


Indent this with two more spaces and remove the newline on the next line.

timvandermeij · 2016-09-22T19:57:50Z

src/display/annotation_layer.js

+
+  Util.inherit(CheckboxWidgetAnnotationElement, WidgetAnnotationElement, {
+    /**
+     * Render the text widget annotation's HTML element in the empty container.


checkbox widget

timvandermeij · 2016-09-22T20:02:15Z

src/display/annotation_layer.js

+
+  Util.inherit(RadioButtonWidgetAnnotationElement, WidgetAnnotationElement, {
+    /**
+     * Render the text widget annotation's HTML element in the empty container.


radio button widget

timvandermeij · 2016-09-22T20:03:04Z

src/display/annotation_layer.js

+     */
+    _toggle: function RadioButtonWidgetAnnotationElement_toggle(element) {
+      element.className = element.className &&
+        !this.data.noToggleToOff ?'' : 'checked';


Add a space after ?. I find it a bit hard to understand what this line does exactly. Can we break it up a bit?

timvandermeij · 2016-09-22T20:05:39Z

src/display/annotation_layer.js

+      var otherKids = this.layer.querySelectorAll(
+        '[data-parent-name="' + this.data.fullName.split('`')[0] + '"]');
+      // Unselect other kids
+      for(var i=0; i<otherKids.length; i++) {


Change this to for (var i = 0, ii = otherKids.length; i < ii; i++) { to cache the length.

timvandermeij · 2016-09-22T20:06:43Z

src/display/annotation_layer.js

+      // Unselect other kids
+      for(var i=0; i<otherKids.length; i++) {
+        if (otherKids[i] !== element) {
+          otherKids[i].className = '';


My previous comment about the class name applies here too. It's a bit confusing why we do that.

timvandermeij · 2016-09-22T20:07:23Z

web/annotation_layer_builder.css

@@ -93,6 +93,60 @@
  width: 115%;
 }

+.annotationLayer .checkboxWidgetAnnotation div,
+.annotationLayer .radioButtonWidgetAnnotation div {
+  background-color: rgba(0, 54, 255, 0.13);


Is this styling also how it looks in Adobe Reader? If not, I'd prefer we keep it the same as Adobe Reader for now as that's what most users will expect. That's also what we did for text widgets.

That's the color used by you guys in .textWidgetAnnotation. It looks pretty much like Adobe Reader.

Snuffleupagus

Why is this patch using <div>s and a lot of complicated CSS instead of just standard HTML form elements (with appropriate styling)?
One obvious downside to the current approach is that the elements added in the patch isn't accessible as the other ones. E.g. note how it's not possible to tab to them, which using proper HTML form elements would fix.

Also, before we start building upon it, we really ought to go through and attempt to clean-up/refactor this code:

pdf.js/src/core/annotation.js

Lines 640 to 670 in 6c263c1

    
           // Building the full field name by collecting the field and 
        
           // its ancestors 'T' data and joining them using '.'. 
        
           var fieldName = []; 
        
           var namedItem = dict; 
        
           var ref = params.ref; 
        
           while (namedItem) { 
        
             var parent = namedItem.get('Parent'); 
        
             var parentRef = namedItem.getRaw('Parent'); 
        
             var name = namedItem.get('T'); 
        
             if (name) { 
        
               fieldName.unshift(stringToPDFString(name)); 
        
             } else if (parent && ref) { 
        
               // The field name is absent, that means more than one field 
        
               // with the same name may exist. Replacing the empty name 
        
               // with the '`' plus index in the parent's 'Kids' array. 
        
               // This is not in the PDF spec but necessary to id the 
        
               // the input controls. 
        
               var kids = parent.get('Kids'); 
        
               var j, jj; 
        
               for (j = 0, jj = kids.length; j < jj; j++) { 
        
                 var kidRef = kids[j]; 
        
                 if (kidRef.num === ref.num && kidRef.gen === ref.gen) { 
        
                   break; 
        
                 } 
        
               } 
        
               fieldName.unshift('`' + j); 
        
             } 
        
             namedItem = parent; 
        
             ref = parentRef; 
        
           } 
        
           data.fullName = fieldName.join('.');

Finally, this patch is currently missing both unit and reference tests!

benweet · 2016-09-22T21:14:36Z

@timvandermeij I'll take your remarks into account and add unit tests.

timvandermeij · 2016-09-23T19:52:00Z

I have been looking into this a bit more and I now realize why you chose divs. I forgot that it's not really possible to style input elements other than those with type="text". We do need input elements, however, both for accessibility as well as for easily reading the selected options, so I think your proposal for using hidden inputs is the only possible way to both make this look good and work like regular input elements, so I'm okay with that.

Snuffleupagus · 2016-09-23T20:12:30Z

[...] so I think your proposal for using hidden inputs is the only possible way to both make this look good and work like regular input elements, so I'm okay with that.

My suspicion is that using hidden elements will unfortunately do nothing to alleviate the keyboard accessibility problems! And it'd also be great if the solution isn't unnecessarily complex.
What I'd suggest is to use regular HTML form elements for now (e.g. checkbox, radiobutton, etc.), since they should be easy to implement and can be improved later on. (Given that there's still a number of major roadblocks to enabling forms by default, e.g. printing/saving the entered data and Appearance stream support.)
However, one possible way forward instead of using HTML form elements (e.g. checkbox, radiobutton, etc) could perhaps be to utilize <button> elements instead, since they are possible to style as you want and should still give proper accessibility.

timvandermeij · 2016-09-24T12:46:18Z

Right, it's a bit unfortunate, but the current state of the web platform won't really let us do what we want. Using <button> elements may help, along with a "checked" state in the form of a class as is done now.

@benweet Could you check if using <button> elements might help, or if you can improve accessibility (tabbing) of the hidden input fields? It could help determine what the best thing to do here would be. If those options do not work, I also suggest to just use regular checkboxes and radio buttons without styling for now and figure out styling in a follow-up PR.

benweet · 2016-09-24T14:59:28Z

Here is a new commit with hidden inputs. Keyboard accessibility works fine, though I didn't manage to have the highlighting on focus.
@timvandermeij I've taken your comments into account but I still have to add tests.

Snuffleupagus

[...] though I didn't manage to have the highlighting on focus.

I'm afraid that this is something that will need to be addressed somehow, if this current solution is to work, since it's not particularily useful to be able to tab to the element if the user cannot see that it has focus.

Snuffleupagus · 2016-09-24T16:14:42Z

web/annotation_layer_builder.css

@@ -43,7 +43,7 @@

 .annotationLayer .textWidgetAnnotation input,
 .annotationLayer .textWidgetAnnotation textarea {
-  background-color: rgba(0, 54, 255, 0.13);
+  background-color: rgb(222, 229, 255);


The background colour needs to be transparent, please undo this change.

My mistake, sorry about that.

Snuffleupagus · 2016-09-24T16:15:13Z

web/annotation_layer_builder.css

@@ -93,6 +93,64 @@
  width: 115%;
 }

+.annotationLayer .checkboxWidgetAnnotation label,
+.annotationLayer .radioButtonWidgetAnnotation label {
+  background-color: rgb(222, 229, 255);


Should be the same colour as above, i.e. background-color: rgba(0, 54, 255, 0.13);

When the pdf is pre-filled, the checked box is drawn in the canvas. I changed the color to be opaque so that the checked box is hidden behind the annotation.

I cannot reproduce this, please provide a PDF file that demonstrates this problem!
Without having seen the file, it sounds like it may be an issue related to Appearance streams support in conjunction with forms, which is a difficult issue that we'll have to somehow fix before we can enable forms by default.

However, simply hiding the background in this way isn't going to be a working solution, so please change this for now.

Here is a sample pdf: pptc153filled.pdf

As I suspected, that's an issue with Apperance streams, so please change this as I originally suggested since the proper solution lie elsewhere.

Can we just use an opaque color for the time being and keep this as a future enhancement?

Why not try to fix this properly instead, by adding getOperatorList overrides to the new class (in src/core/annotation.js)?
Similar to the existing TextWidgetAnnotation, but perhaps a bit simpler for now:

Util.inherit(ButtonWidgetAnnotation, WidgetAnnotation, { getOperatorList: function ButtonWidgetAnnotation_getOperatorList(evaluator, task, renderForms) { var operatorList = new OperatorList(); // Do not render form elements on the canvas when interactive forms are // enabled. The display layer is responsible for rendering them instead. if (renderForms) { return Promise.resolve(operatorList); } if (this.appearance) { return Annotation.prototype.getOperatorList.call(this, evaluator, task, renderForms); } return Promise.resolve(operatorList); } });

Works pretty good! I'll merge this tomorrow. Thanks

Snuffleupagus · 2016-09-24T16:18:31Z

src/core/annotation.js

+    this.data.pushbutton = this.hasFieldFlag(AnnotationFieldFlag.PUSHBUTTON);
+    var fieldValue = Util.getInheritableProperty(params.dict, 'V');
+    if (fieldValue) {
+      // Must be a Name object


The condition doesn't actually agree with the comment, so change it to:
if (isName(fieldValue)) { instead (and remove the comment since it's then superfluous).

Snuffleupagus · 2016-09-24T16:28:32Z

web/annotation_layer_builder.css

+  width: 60%;
+}
+
+.annotationLayer .radioButtonWidgetAnnotation input:checked + label:before {


There seems to be some overlap between these rules and the ones above, could that be simplified somewhat?

Snuffleupagus · 2016-09-24T21:19:30Z

src/core/annotation.js

+
+    this.data.noToggleToOff = this.hasFieldFlag(
+      AnnotationFieldFlag.NOTOGGLETOOFF);
+    this.data.radio = this.hasFieldFlag(AnnotationFieldFlag.RADIO);


The PDF specification specifically mentions that the RADIO flag cannot be set at the same time as the PUSHBUTTON flag, but that is missing from the code here.

Do we really want to check that the document complies with the PDF spec? What does it bring?

Well, PDF.js is a PDF viewer after all, so we most certainly want to try and follow the specification!
If experience has thought us anything, it's that PDF files found in the wild breaks the specification in all sorts of "interesting" ways, which often leads to bugs. So of course we always want to try to ensure that data present in a PDF file is valid!

Right. What do you suggest?

Note that I've already put a bit of logic in annotation_layer.js that does some kind of compliancy check:

if (!parameters.data.pushbutton) { if (parameters.data.radio) { return new RadioButtonWidgetAnnotationElement(parameters); } else { return new CheckboxWidgetAnnotationElement(parameters); } }

The main validation should happen in the core layer, so wouldn't e.g. this be enough:

this.data.radio = this.hasFieldFlag(AnnotationFieldFlag.RADIO) && !this.hasFieldFlag(AnnotationFieldFlag.PUSHBUTTON);

benweet · 2016-09-26T10:24:35Z

Added unit tests and CSS outline on focus.

timvandermeij · 2016-09-26T20:26:01Z

Let's remove the noToggleToOff code if we're not implementing it. Then, please squash the commits into one commit so we can do a new review round. If you're not familiar with squashing commits, here is how to do it easily: https://github.com/mozilla/pdf.js/wiki/Squashing-Commits

benweet · 2016-09-27T10:00:45Z

Done.
Cheers!

timvandermeij

This patch looks much better! Let's wait until the other PR is merged and then address these comments. Thanks!

timvandermeij · 2016-09-27T17:40:48Z

src/core/annotation.js

+    this.data.pushbutton = this.hasFieldFlag(AnnotationFieldFlag.PUSHBUTTON);
+    this.data.radio = !this.data.pushbutton &&
+                      this.hasFieldFlag(AnnotationFieldFlag.RADIO);
+    var fieldValue = Util.getInheritableProperty(params.dict, 'V');


You can use this.data.fieldValue here (and thus remove this line) after the other PR is merged, but that's just a note for now.

timvandermeij · 2016-09-27T17:42:49Z

src/core/annotation.js

+    this.data.radio = !this.data.pushbutton &&
+                      this.hasFieldFlag(AnnotationFieldFlag.RADIO);
+    var fieldValue = Util.getInheritableProperty(params.dict, 'V');
+    if (isName(fieldValue)) {


What happens if it's not a Name? It happens very often that PDF files do not adhere to the specification entirely, so if they do not provide a Name here, then this.data.fieldValue will be used as-is after the other PR is merged. Should we make it empty in such a case?

timvandermeij · 2016-09-27T17:44:20Z

src/display/annotation_layer.js

@@ -76,6 +76,15 @@ AnnotationElementFactory.prototype =
        switch (fieldType) {
          case 'Tx':
            return new TextWidgetAnnotationElement(parameters);
+          case 'Btn':
+            if (!parameters.data.pushbutton) {


Perhaps not really. I'm also fine with a comment such as // Push buttons are not implemented yet so that we at least know in the code that something is still missing here.

timvandermeij · 2016-09-27T17:45:45Z

src/display/annotation_layer.js

+          element.checked = true;
+        }
+        this.container.appendChild(element);
+        element = document.createElement('label');


Let's add an empty line before this line to make this a bit more readable as a new block of code.

timvandermeij · 2016-09-27T17:46:40Z

src/display/annotation_layer.js

+          element.checked = true;
+        }
+        this.container.appendChild(element);
+        element = document.createElement('label');


Same here, let's add a newline above this line for readability.

timvandermeij · 2016-09-27T17:47:21Z

test/unit/annotation_layer_spec.js

+    });
+
+    it('should have proper flags',
+        function() {


For all occurrences in this file: move this to the previous line. We only use this if it doesn't fit on one line anymore.

timvandermeij · 2016-09-27T17:48:38Z

test/unit/annotation_layer_spec.js

+      ]);
+
+      var checkboxWidgetAnnotation =
+          annotationFactory.create(xref, checkboxWidgetRef);


For all occurrences in this file: remove two spaces from the indentation here. We indent everything with two spaces.

timvandermeij · 2016-10-28T14:11:56Z

Everything that blocked this PR has landed, so it's now good to rebase and fix up, @benweet. Thanks.

benweet · 2016-11-04T10:15:36Z

Fixed. Cheers!

timvandermeij · 2016-12-15T23:15:32Z

Closing as replaced by #7898, which contains this commit and follow-up commits so it can be merged.

benweet mentioned this pull request Sep 22, 2016

Added acroform checkbox controls #7660

Closed

timvandermeij requested changes Sep 22, 2016

View reviewed changes

timvandermeij added annotations form-acroform labels Sep 22, 2016

Snuffleupagus requested changes Sep 22, 2016

View reviewed changes

benweet force-pushed the forms-checkbox-radio branch 3 times, most recently from 943b31c to 3453bbc Compare September 24, 2016 14:46

Snuffleupagus requested changes Sep 24, 2016

View reviewed changes

Snuffleupagus reviewed Sep 24, 2016

View reviewed changes

benweet force-pushed the forms-checkbox-radio branch from 14d5ec9 to 657229a Compare September 27, 2016 09:58

benweet mentioned this pull request Sep 27, 2016

Interactive forms: render choice widget annotations #7671

Merged

timvandermeij requested changes Sep 27, 2016

View reviewed changes

benweet force-pushed the forms-checkbox-radio branch from 657229a to f2450ac Compare November 4, 2016 10:13

Acroform checkbox and radio controls

9f10e09

benweet force-pushed the forms-checkbox-radio branch from f2450ac to 9f10e09 Compare November 4, 2016 12:01

timvandermeij mentioned this pull request Dec 15, 2016

Interactive forms: render button widget annotations (checkboxes and radio buttons) #7898

Merged

timvandermeij closed this Dec 15, 2016

	// Building the full field name by collecting the field and
	// its ancestors 'T' data and joining them using '.'.
	var fieldName = [];
	var namedItem = dict;
	var ref = params.ref;
	while (namedItem) {
	var parent = namedItem.get('Parent');
	var parentRef = namedItem.getRaw('Parent');
	var name = namedItem.get('T');
	if (name) {
	fieldName.unshift(stringToPDFString(name));
	} else if (parent && ref) {
	// The field name is absent, that means more than one field
	// with the same name may exist. Replacing the empty name
	// with the '`' plus index in the parent's 'Kids' array.
	// This is not in the PDF spec but necessary to id the
	// the input controls.
	var kids = parent.get('Kids');
	var j, jj;
	for (j = 0, jj = kids.length; j < jj; j++) {
	var kidRef = kids[j];
	if (kidRef.num === ref.num && kidRef.gen === ref.gen) {
	break;
	}
	}
	fieldName.unshift('`' + j);
	}
	namedItem = parent;
	ref = parentRef;
	}
	data.fullName = fieldName.join('.');

Acroform checkbox and radio controls #7664

Acroform checkbox and radio controls #7664

Conversation

benweet commented Sep 22, 2016

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

timvandermeij Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

benweet commented Sep 22, 2016

timvandermeij commented Sep 23, 2016

Snuffleupagus commented Sep 23, 2016 • edited Loading

timvandermeij commented Sep 24, 2016 • edited Loading

benweet commented Sep 24, 2016 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benweet commented Sep 26, 2016

timvandermeij commented Sep 26, 2016

benweet commented Sep 27, 2016

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

timvandermeij Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

timvandermeij commented Oct 28, 2016

benweet commented Nov 4, 2016

timvandermeij commented Dec 15, 2016 • edited Loading

timvandermeij Sep 22, 2016 •

edited

Loading

timvandermeij Sep 22, 2016 •

edited

Loading

timvandermeij Sep 22, 2016 •

edited

Loading

timvandermeij Sep 22, 2016 •

edited

Loading

Snuffleupagus commented Sep 23, 2016 •

edited

Loading

timvandermeij commented Sep 24, 2016 •

edited

Loading

benweet commented Sep 24, 2016 •

edited

Loading

Snuffleupagus Sep 24, 2016 •

edited

Loading

Snuffleupagus Sep 24, 2016 •

edited

Loading

Snuffleupagus Sep 24, 2016 •

edited

Loading

Snuffleupagus Sep 24, 2016 •

edited

Loading

timvandermeij Sep 27, 2016 •

edited

Loading

timvandermeij Sep 27, 2016 •

edited

Loading

timvandermeij Sep 27, 2016 •

edited

Loading

timvandermeij commented Dec 15, 2016 •

edited

Loading