[Acroform] Use the full path to find the node in the XFA datasets where to store the value #16082

calixteman · 2023-02-22T21:11:49Z

I noticed several 'Path not found' errors because of a field called #subform[2]. From the XFA specs, the hash is used for a class of elements in the template tree. When we're looking for a node in the datasets tree, it doesn't make sense to search for a class. Hence the path element starting with a hash are just skipped.

Snuffleupagus

Can you please add a test-case, to avoid this breaking in the future?

src/core/annotation.js

calixteman · 2023-02-22T22:27:14Z

Can you please add a test-case, to avoid this breaking in the future?

It's a bit hard to have a test.
We generally don't use the value from the xfa datasets except when it's missing from the acroform.
I thought writing a unit test and check that a string is in the saved data but the pdf is encrypted.
I don't have any tool to generate a file with both xfa and acroform (even none to just generate a xfa).

So I see two solutions to have a test use qpdf to have a clear pdf and just check a xml string is included in the file or try to write a pdf by hand. I'm not super excited by the 2nd solution and I'm not sure if we can attach a pdf to the gh issue where encryption has been removed.
Maybe I'm just overlooking something easy... it's bit late here.

Do you have any ideas ?

Snuffleupagus · 2023-02-23T08:36:44Z

How about a unit-test then, just so that the new code isn't completely untested?

Note that we've got a number of unit-tests specifically for the _constructFieldName-method, and I'd hope that it's not too complicated to add one that covers the new code; see

pdf.js/test/unit/annotation_spec.js

Lines 1308 to 1385 in e676c93

    
           it("should handle unknown field names", async function () { 
        
             const widgetRef = Ref.get(20, 0); 
        
             const xref = new XRefMock([{ ref: widgetRef, data: widgetDict }]); 
        
             const { data } = await AnnotationFactory.create( 
        
               xref, 
        
               widgetRef, 
        
               pdfManagerMock, 
        
               idFactoryMock 
        
             ); 
        
             expect(data.annotationType).toEqual(AnnotationType.WIDGET); 
        
             expect(data.fieldName).toEqual(""); 
        
           }); 
        
           it("should construct the field name when there are no ancestors", async function () { 
        
             widgetDict.set("T", "foo"); 
        
             const widgetRef = Ref.get(21, 0); 
        
             const xref = new XRefMock([{ ref: widgetRef, data: widgetDict }]); 
        
             const { data } = await AnnotationFactory.create( 
        
               xref, 
        
               widgetRef, 
        
               pdfManagerMock, 
        
               idFactoryMock 
        
             ); 
        
             expect(data.annotationType).toEqual(AnnotationType.WIDGET); 
        
             expect(data.fieldName).toEqual("foo"); 
        
           }); 
        
           it("should construct the field name when there are ancestors", async function () { 
        
             const firstParent = new Dict(); 
        
             firstParent.set("T", "foo"); 
        
             const secondParent = new Dict(); 
        
             secondParent.set("Parent", firstParent); 
        
             secondParent.set("T", "bar"); 
        
             widgetDict.set("Parent", secondParent); 
        
             widgetDict.set("T", "baz"); 
        
             const widgetRef = Ref.get(22, 0); 
        
             const xref = new XRefMock([{ ref: widgetRef, data: widgetDict }]); 
        
             const { data } = await AnnotationFactory.create( 
        
               xref, 
        
               widgetRef, 
        
               pdfManagerMock, 
        
               idFactoryMock 
        
             ); 
        
             expect(data.annotationType).toEqual(AnnotationType.WIDGET); 
        
             expect(data.fieldName).toEqual("foo.bar.baz"); 
        
           }); 
        
           it( 
        
             "should construct the field name if a parent is not a dictionary " + 
        
               "(issue 8143)", 
        
             async function () { 
        
               const parentDict = new Dict(); 
        
               parentDict.set("Parent", null); 
        
               parentDict.set("T", "foo"); 
        
               widgetDict.set("Parent", parentDict); 
        
               widgetDict.set("T", "bar"); 
        
               const widgetRef = Ref.get(22, 0); 
        
               const xref = new XRefMock([{ ref: widgetRef, data: widgetDict }]); 
        
               const { data } = await AnnotationFactory.create( 
        
                 xref, 
        
                 widgetRef, 
        
                 pdfManagerMock, 
        
                 idFactoryMock 
        
               ); 
        
               expect(data.annotationType).toEqual(AnnotationType.WIDGET); 
        
               expect(data.fieldName).toEqual("foo.bar"); 
        
             } 
        
           );

calixteman · 2023-02-23T09:23:53Z

I managed to write a test but I had to a new function in the api.
At least the test covers exactly what it should.

test/unit/api_spec.js

src/display/api.js

calixteman · 2023-02-23T10:35:28Z

When I wrote the code to save data in the xfa datasets it was with f1040 form in mind.
So I tested with it and my patch broke saving for it: the paths to the fields are wrong but the final name is enough to identify them.
Hence, I fixed the issue and I added a test for f1040.

Snuffleupagus

r=me, thanks for adding tests!

Snuffleupagus · 2023-02-23T11:00:24Z

/botio test

pdfjsbot · 2023-02-23T11:00:26Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/b108c009fd28d6a/output.txt

pdfjsbot · 2023-02-23T11:00:26Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/2ce9481eb89590f/output.txt

pdfjsbot · 2023-02-23T11:05:05Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/2ce9481eb89590f/output.txt

Total script time: 4.63 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: Passed
Regression tests: FAILED

Image differences available at: http://54.241.84.105:8877/2ce9481eb89590f/reftest-analyzer.html#web=eq.log

pdfjsbot · 2023-02-23T11:09:36Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/b108c009fd28d6a/output.txt

Total script time: 9.15 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: FAILED
Regression tests: FAILED

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

…re to store the value I noticed several 'Path not found' errors because of a field called #subform[2]. From the XFA specs, the hash is used for a class of elements in the template tree. When we're looking for a node in the datasets tree, it doesn't make sense to search for a class. Hence the path element starting with a hash are just skipped.

calixteman · 2023-02-23T11:13:14Z

Strange...
I fixed the md5.

calixteman · 2023-02-23T11:13:20Z

/botio test

pdfjsbot · 2023-02-23T11:13:23Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/8f9a6e2d949a841/output.txt

pdfjsbot · 2023-02-23T11:13:23Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/5be3c21866bc8a2/output.txt

pdfjsbot · 2023-02-23T11:39:26Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/5be3c21866bc8a2/output.txt

Total script time: 26.03 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 9
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/5be3c21866bc8a2/reftest-analyzer.html#web=eq.log

pdfjsbot · 2023-02-23T11:45:32Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/8f9a6e2d949a841/output.txt

Total script time: 32.13 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 28

Image differences available at: http://54.193.163.58:8877/8f9a6e2d949a841/reftest-analyzer.html#web=eq.log

calixteman requested a review from Snuffleupagus February 22, 2023 21:11

calixteman linked an issue Feb 22, 2023 that may be closed by this pull request

[Acroform] Field not correctly saved in the xfa datasets #16081

Closed

Snuffleupagus reviewed Feb 22, 2023

View reviewed changes

src/core/annotation.js Show resolved Hide resolved

calixteman force-pushed the fix_class_in_path branch from 9b878aa to 19d0f19 Compare February 23, 2023 09:20

Snuffleupagus reviewed Feb 23, 2023

View reviewed changes

test/unit/api_spec.js Show resolved Hide resolved

src/display/api.js Outdated Show resolved Hide resolved

src/display/api.js Outdated Show resolved Hide resolved

calixteman force-pushed the fix_class_in_path branch from 19d0f19 to 2e6a306 Compare February 23, 2023 10:33

Snuffleupagus approved these changes Feb 23, 2023

View reviewed changes

calixteman force-pushed the fix_class_in_path branch from 2e6a306 to 3a21423 Compare February 23, 2023 11:12

calixteman merged commit 9c58d4f into mozilla:master Feb 23, 2023

calixteman deleted the fix_class_in_path branch February 23, 2023 12:39

calixteman mentioned this pull request Sep 7, 2023

Annotation fieldName not loaded correctly, part of the name containing "#subform" is ignored #16914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Acroform] Use the full path to find the node in the XFA datasets where to store the value #16082

[Acroform] Use the full path to find the node in the XFA datasets where to store the value #16082

calixteman commented Feb 22, 2023

Snuffleupagus left a comment

calixteman commented Feb 22, 2023

Snuffleupagus commented Feb 23, 2023

calixteman commented Feb 23, 2023

calixteman commented Feb 23, 2023

Snuffleupagus left a comment

Snuffleupagus commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

calixteman commented Feb 23, 2023

calixteman commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

[Acroform] Use the full path to find the node in the XFA datasets where to store the value #16082

[Acroform] Use the full path to find the node in the XFA datasets where to store the value #16082

Conversation

calixteman commented Feb 22, 2023

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Feb 22, 2023

Snuffleupagus commented Feb 23, 2023

calixteman commented Feb 23, 2023

calixteman commented Feb 23, 2023

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 23, 2023

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 23, 2023

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Feb 23, 2023

From: Bot.io (Windows)

Failed

calixteman commented Feb 23, 2023

calixteman commented Feb 23, 2023

pdfjsbot commented Feb 23, 2023

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 23, 2023

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 23, 2023

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Feb 23, 2023

From: Bot.io (Windows)

Failed