From 5fed7112a28db7394df86997f923366168fed6b6 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Sat, 22 Aug 2020 13:57:12 +0200 Subject: [PATCH 1/5] Use the export value instead of the display value for choice widget option selection The export value is used when the document is saved, so it should also be used when the document is opened to determine which choice widget option is selected. The display value is, as the name implies, only to be used for viewer display purposes and not for other logic. This makes sure that in the document from #12233 the "Favourite colour" choice widget is correctly initialized with "Red" instead of "Black" because the field value is equal to the export value (always the case), but not the display value (not always the case). Moreover, saving now also correctly uses the export value and not the display value. --- src/display/annotation_layer.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index f1385f86dfef1..c1c6b4207c736 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -682,16 +682,16 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement { const optionElement = document.createElement("option"); optionElement.textContent = option.displayValue; optionElement.value = option.exportValue; - if (this.data.fieldValue.includes(option.displayValue)) { + if (this.data.fieldValue.includes(option.exportValue)) { optionElement.setAttribute("selected", true); - storage.setValue(id, option.displayValue); + storage.setValue(id, option.exportValue); } selectElement.appendChild(optionElement); } selectElement.addEventListener("input", function (event) { const options = event.target.options; - const value = options[options.selectedIndex].text; + const value = options[options.selectedIndex].value; storage.setValue(id, value); }); From 36e149800e17ae72463307f3e56fd4723c2373e5 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Sat, 22 Aug 2020 14:29:00 +0200 Subject: [PATCH 2/5] Unconditionally set the field value for choice widgets in the annotation storage This commit makes the following improvements: - The code is similar to the other interactive form widgets now, with a clear note for the only difference. - Calling `getOrCreateValue` unconditionally ensures that choice widgets always have a value in the annotation storage. Previously we only inserted a value in the annotation storage when an option matched or when a selection was changed. However, this causes breakage when saving/printing because comboboxes, which we don't fully support yet but are rendered, might not have a value in storage at all. Their field value might not match any option since it allows the user to enter a custom value. - Calling `getOrCreateValue` unconditionally ensures that forms with choice widgets no longer always trigger a warning when the user navigates away from the page. This fixes https://github.com/mozilla/pdf.js/pull/12241#discussion_r474279654 --- src/display/annotation_layer.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index c1c6b4207c736..795617615becf 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -665,6 +665,18 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement { const storage = this.annotationStorage; const id = this.data.id; + // For printing/saving we currently only support choice widgets with one + // option selection. Therefore, listboxes (#12189) and comboboxes (#12224) + // are not properly printed/saved yet, so we only store the first item in + // the field value array instead of the entire array. Once support for those + // two field types is implemented, we should use the same pattern as the + // other interactive widgets where the return value of `getOrCreateValue` is + // used and the full array of field values is stored. + storage.getOrCreateValue( + id, + this.data.fieldValue.length > 0 ? this.data.fieldValue[0] : null + ); + const selectElement = document.createElement("select"); selectElement.disabled = this.data.readOnly; selectElement.name = this.data.fieldName; @@ -684,7 +696,6 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement { optionElement.value = option.exportValue; if (this.data.fieldValue.includes(option.exportValue)) { optionElement.setAttribute("selected", true); - storage.setValue(id, option.exportValue); } selectElement.appendChild(optionElement); } From 1b82ad8fff45f8964f5b3decf6f525d38dd4a76a Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Sat, 22 Aug 2020 15:02:29 +0200 Subject: [PATCH 3/5] Decode widget form values consistently The helper method `_decodeFormValue` is used to ensure that it happens in one place. Note that form values are field values, display values and export values. --- src/core/annotation.js | 43 ++++++++++++++++++++++++++++-------- test/unit/annotation_spec.js | 19 ++++++++-------- 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/core/annotation.js b/src/core/annotation.js index 5133633c09f9f..bca494b587f65 100644 --- a/src/core/annotation.js +++ b/src/core/annotation.js @@ -803,11 +803,14 @@ class WidgetAnnotation extends Annotation { data.annotationType = AnnotationType.WIDGET; data.fieldName = this._constructFieldName(dict); - data.fieldValue = getInheritableProperty({ + + const fieldValue = getInheritableProperty({ dict, key: "V", getArray: true, }); + data.fieldValue = this._decodeFormValue(fieldValue); + data.alternativeText = stringToPDFString(dict.get("TU") || ""); data.defaultAppearance = getInheritableProperty({ dict, key: "DA" }) || @@ -882,6 +885,28 @@ class WidgetAnnotation extends Annotation { return fieldName.join("."); } + /** + * Decode the given form value. + * + * @private + * @memberof WidgetAnnotation + * @param {Array|Name|string} formValue - The (possibly encoded) + * form value. + * @returns {Array|string|null} + */ + _decodeFormValue(formValue) { + if (Array.isArray(formValue)) { + return formValue + .filter(item => isString(item)) + .map(item => stringToPDFString(item)); + } else if (isName(formValue)) { + return stringToPDFString(formValue.name); + } else if (isString(formValue)) { + return stringToPDFString(formValue); + } + return null; + } + /** * Check if a provided field flag is set. * @@ -1194,7 +1219,9 @@ class TextWidgetAnnotation extends WidgetAnnotation { const dict = params.dict; // The field value is always a string. - this.data.fieldValue = stringToPDFString(this.data.fieldValue || ""); + if (!isString(this.data.fieldValue)) { + this.data.fieldValue = ""; + } // Determine the alignment of text in the field. let alignment = getInheritableProperty({ dict, key: "Q" }); @@ -1499,10 +1526,6 @@ class ButtonWidgetAnnotation extends WidgetAnnotation { } _processCheckBox(params) { - if (isName(this.data.fieldValue)) { - this.data.fieldValue = this.data.fieldValue.name; - } - const customAppearance = params.dict.get("AP"); if (!isDict(customAppearance)) { return; @@ -1541,7 +1564,7 @@ class ButtonWidgetAnnotation extends WidgetAnnotation { const fieldParentValue = fieldParent.get("V"); if (isName(fieldParentValue)) { this.parent = params.dict.getRaw("Parent"); - this.data.fieldValue = fieldParentValue.name; + this.data.fieldValue = this._decodeFormValue(fieldParentValue); } } @@ -1602,8 +1625,10 @@ class ChoiceWidgetAnnotation extends WidgetAnnotation { const isOptionArray = Array.isArray(option); this.data.options[i] = { - exportValue: isOptionArray ? xref.fetchIfRef(option[0]) : option, - displayValue: stringToPDFString( + exportValue: this._decodeFormValue( + isOptionArray ? xref.fetchIfRef(option[0]) : option + ), + displayValue: this._decodeFormValue( isOptionArray ? xref.fetchIfRef(option[1]) : option ), }; diff --git a/test/unit/annotation_spec.js b/test/unit/annotation_spec.js index 9964cb67618fd..46ed399741e75 100644 --- a/test/unit/annotation_spec.js +++ b/test/unit/annotation_spec.js @@ -2450,16 +2450,12 @@ describe("annotation", function () { }, done.fail); }); - it("should sanitize display values in option arrays (issue 8947)", function (done) { - // The option value is a UTF-16BE string. The display value should be - // sanitized, but the export value should remain the same since that - // may be used as a unique identifier when exporting form values. - const options = ["\xFE\xFF\x00F\x00o\x00o"]; - const expected = [ - { exportValue: "\xFE\xFF\x00F\x00o\x00o", displayValue: "Foo" }, - ]; + it("should decode form values", function (done) { + const encodedString = "\xFE\xFF\x00F\x00o\x00o"; + const decodedString = "Foo"; - choiceWidgetDict.set("Opt", options); + choiceWidgetDict.set("Opt", [encodedString]); + choiceWidgetDict.set("V", encodedString); const choiceWidgetRef = Ref.get(984, 0); const xref = new XRefMock([ @@ -2473,7 +2469,10 @@ describe("annotation", function () { idFactoryMock ).then(({ data }) => { expect(data.annotationType).toEqual(AnnotationType.WIDGET); - expect(data.options).toEqual(expected); + expect(data.fieldValue).toEqual([decodedString]); + expect(data.options).toEqual([ + { exportValue: decodedString, displayValue: decodedString }, + ]); done(); }, done.fail); }); From a8efc0296beb4c390c55a40daa1aa1c1e8d133a7 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Sat, 22 Aug 2020 15:45:33 +0200 Subject: [PATCH 4/5] Obtain the export values for choice widgets from the normal appearance The down appearance (`D`) is optional and not available in the document from #12233, so the checkboxes are never saved/printed as checked because the checked appearance is based on the export value that is missing because the `D` entry is not available. Instead, we should use the normal appearance (`N`) since that one is required and therefore always available. Finally, the /Off appearance is optional according to section 12.7.4.2.3 of the specification, so that needs to be taken into account to match the specification and to fix reference test failures for the `annotation-button-widget-print` test. That is a file that doesn't specify an /Off appearance in the normal appearance dictionary. --- src/core/annotation.js | 18 +++++------ test/unit/annotation_spec.js | 60 +++++++++++++++++++++--------------- 2 files changed, 44 insertions(+), 34 deletions(-) diff --git a/src/core/annotation.js b/src/core/annotation.js index bca494b587f65..5fe3d86547620 100644 --- a/src/core/annotation.js +++ b/src/core/annotation.js @@ -1531,25 +1531,23 @@ class ButtonWidgetAnnotation extends WidgetAnnotation { return; } - const exportValueOptionsDict = customAppearance.get("D"); - if (!isDict(exportValueOptionsDict)) { + const normalAppearance = customAppearance.get("N"); + if (!isDict(normalAppearance)) { return; } - const exportValues = exportValueOptionsDict.getKeys(); - const hasCorrectOptionCount = exportValues.length === 2; - if (!hasCorrectOptionCount) { + const exportValues = normalAppearance.getKeys(); + if (!exportValues.includes("Off")) { + // The /Off appearance is optional. + exportValues.push("Off"); + } + if (exportValues.length !== 2) { return; } this.data.exportValue = exportValues[0] === "Off" ? exportValues[1] : exportValues[0]; - const normalAppearance = customAppearance.get("N"); - if (!isDict(normalAppearance)) { - return; - } - this.checkedAppearance = normalAppearance.get(this.data.exportValue); this.uncheckedAppearance = normalAppearance.get("Off") || null; } diff --git a/test/unit/annotation_spec.js b/test/unit/annotation_spec.js index 46ed399741e75..f948a926ddcf4 100644 --- a/test/unit/annotation_spec.js +++ b/test/unit/annotation_spec.js @@ -1882,11 +1882,11 @@ describe("annotation", function () { buttonWidgetDict.set("V", Name.get("1")); const appearanceStatesDict = new Dict(); - const exportValueOptionsDict = new Dict(); + const normalAppearanceDict = new Dict(); - exportValueOptionsDict.set("Off", 0); - exportValueOptionsDict.set("Checked", 1); - appearanceStatesDict.set("D", exportValueOptionsDict); + normalAppearanceDict.set("Off", 0); + normalAppearanceDict.set("Checked", 1); + appearanceStatesDict.set("N", normalAppearanceDict); buttonWidgetDict.set("AP", appearanceStatesDict); const buttonWidgetRef = Ref.get(124, 0); @@ -1931,9 +1931,38 @@ describe("annotation", function () { }, done.fail); }); + it("should handle checkboxes without /Off appearance", function (done) { + buttonWidgetDict.set("V", Name.get("1")); + + const appearanceStatesDict = new Dict(); + const normalAppearanceDict = new Dict(); + + normalAppearanceDict.set("Checked", 1); + appearanceStatesDict.set("N", normalAppearanceDict); + buttonWidgetDict.set("AP", appearanceStatesDict); + + const buttonWidgetRef = Ref.get(124, 0); + const xref = new XRefMock([ + { ref: buttonWidgetRef, data: buttonWidgetDict }, + ]); + + AnnotationFactory.create( + xref, + buttonWidgetRef, + pdfManagerMock, + idFactoryMock + ).then(({ data }) => { + expect(data.annotationType).toEqual(AnnotationType.WIDGET); + expect(data.checkBox).toEqual(true); + expect(data.fieldValue).toEqual("1"); + expect(data.radioButton).toEqual(false); + expect(data.exportValue).toEqual("Checked"); + done(); + }, done.fail); + }); + it("should render checkboxes for printing", function (done) { const appearanceStatesDict = new Dict(); - const exportValueOptionsDict = new Dict(); const normalAppearanceDict = new Dict(); const checkedAppearanceDict = new Dict(); const uncheckedAppearanceDict = new Dict(); @@ -1949,9 +1978,6 @@ describe("annotation", function () { checkedAppearanceDict.set("Matrix", [1, 0, 0, 1, 0, 0]); normalAppearanceDict.set("Checked", checkedStream); normalAppearanceDict.set("Off", uncheckedStream); - exportValueOptionsDict.set("Off", 0); - exportValueOptionsDict.set("Checked", 1); - appearanceStatesDict.set("D", exportValueOptionsDict); appearanceStatesDict.set("N", normalAppearanceDict); buttonWidgetDict.set("AP", appearanceStatesDict); @@ -2019,14 +2045,10 @@ describe("annotation", function () { it("should save checkboxes", function (done) { const appearanceStatesDict = new Dict(); - const exportValueOptionsDict = new Dict(); const normalAppearanceDict = new Dict(); normalAppearanceDict.set("Checked", Ref.get(314, 0)); normalAppearanceDict.set("Off", Ref.get(271, 0)); - exportValueOptionsDict.set("Off", 0); - exportValueOptionsDict.set("Checked", 1); - appearanceStatesDict.set("D", exportValueOptionsDict); appearanceStatesDict.set("N", normalAppearanceDict); buttonWidgetDict.set("AP", appearanceStatesDict); @@ -2059,8 +2081,7 @@ describe("annotation", function () { expect(oldData.data).toEqual( "123 0 obj\n" + "<< /Type /Annot /Subtype /Widget /FT /Btn " + - "/AP << /D << /Off 0 /Checked 1>> " + - "/N << /Checked 314 0 R /Off 271 0 R>>>> " + + "/AP << /N << /Checked 314 0 R /Off 271 0 R>>>> " + "/V /Checked /AS /Checked /M (date)>>\nendobj\n" ); return annotation; @@ -2142,7 +2163,6 @@ describe("annotation", function () { it("should render radio buttons for printing", function (done) { const appearanceStatesDict = new Dict(); - const exportValueOptionsDict = new Dict(); const normalAppearanceDict = new Dict(); const checkedAppearanceDict = new Dict(); const uncheckedAppearanceDict = new Dict(); @@ -2158,9 +2178,6 @@ describe("annotation", function () { checkedAppearanceDict.set("Matrix", [1, 0, 0, 1, 0, 0]); normalAppearanceDict.set("Checked", checkedStream); normalAppearanceDict.set("Off", uncheckedStream); - exportValueOptionsDict.set("Off", 0); - exportValueOptionsDict.set("Checked", 1); - appearanceStatesDict.set("D", exportValueOptionsDict); appearanceStatesDict.set("N", normalAppearanceDict); buttonWidgetDict.set("Ff", AnnotationFieldFlag.RADIO); @@ -2229,14 +2246,10 @@ describe("annotation", function () { it("should save radio buttons", function (done) { const appearanceStatesDict = new Dict(); - const exportValueOptionsDict = new Dict(); const normalAppearanceDict = new Dict(); normalAppearanceDict.set("Checked", Ref.get(314, 0)); normalAppearanceDict.set("Off", Ref.get(271, 0)); - exportValueOptionsDict.set("Off", 0); - exportValueOptionsDict.set("Checked", 1); - appearanceStatesDict.set("D", exportValueOptionsDict); appearanceStatesDict.set("N", normalAppearanceDict); buttonWidgetDict.set("Ff", AnnotationFieldFlag.RADIO); @@ -2282,8 +2295,7 @@ describe("annotation", function () { expect(radioData.data).toEqual( "123 0 obj\n" + "<< /Type /Annot /Subtype /Widget /FT /Btn /Ff 32768 " + - "/AP << /D << /Off 0 /Checked 1>> " + - "/N << /Checked 314 0 R /Off 271 0 R>>>> " + + "/AP << /N << /Checked 314 0 R /Off 271 0 R>>>> " + "/Parent 456 0 R /AS /Checked /M (date)>>\nendobj\n" ); expect(parentData.ref).toEqual(Ref.get(456, 0)); From 73bc8e1d9f18b581f131c08273edaeda90fc6896 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Sat, 22 Aug 2020 16:24:03 +0200 Subject: [PATCH 5/5] Include forms/print reference tests for the document from #12233 In addition to the unit tests these reference tests make sure that this document, that triggered some edge cases in our code, can be rendered and printed successfully now. --- test/pdfs/issue12233.pdf.link | 1 + test/test_manifest.json | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 test/pdfs/issue12233.pdf.link diff --git a/test/pdfs/issue12233.pdf.link b/test/pdfs/issue12233.pdf.link new file mode 100644 index 0000000000000..a050305243cba --- /dev/null +++ b/test/pdfs/issue12233.pdf.link @@ -0,0 +1 @@ +https://github.com/mozilla/pdf.js/files/5112498/OoPdfFormExample.pdf diff --git a/test/test_manifest.json b/test/test_manifest.json index 9a9370bd84be7..27337835903a1 100644 --- a/test/test_manifest.json +++ b/test/test_manifest.json @@ -4492,6 +4492,25 @@ "63R": "" } }, + { "id": "issue12233-forms", + "file": "pdfs/issue12233.pdf", + "md5": "6099fc695fe018ce444752929d86f9c8", + "link": true, + "rounds": 1, + "type": "eq", + "forms": true + }, + { "id": "issue12233-print", + "file": "pdfs/issue12233.pdf", + "md5": "6099fc695fe018ce444752929d86f9c8", + "link": true, + "rounds": 1, + "type": "eq", + "print": true, + "annotationStorage": { + "20R": true + } + }, { "id": "issue11931", "file": "pdfs/issue11931.pdf", "md5": "9ea233037992e1f10280420a49e72845",