Skip to content

Re-factor document.getElementsByName lookups in the AnnotationLayer (issue 14003) #14023

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 23, 2021
Merged

Re-factor document.getElementsByName lookups in the AnnotationLayer (issue 14003) #14023

merged 2 commits into from
Sep 23, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

This replaces direct document.getElementsByName lookups with a helper method which:

  • Lets the AnnotationLayer use the data returned by the PDFDocumentProxy.getFieldObjects API-method, such that we can directly lookup only the necessary DOM elements.
  • Fallback to using document.getElementsByName as before, such that e.g. the standalone viewer components still work.

Finally, to fix the problems reported in issue 14003, regardless of the code-path we now also enforce that the DOM elements found were actually created by the AnnotationLayer code.
With these changes we'll thus be able to update form elements on all visible pages just as before, but we'll additionally update the AnnotationStorage for not-yet-rendered elements thus fixing a pre-existing bug.

Fixes #14003

@Snuffleupagus Snuffleupagus mentioned this pull request Sep 14, 2021
Closed
@Snuffleupagus Snuffleupagus force-pushed the AnnotationLayer-getElementsByName branch from 525a3f0 to f54431a Compare September 14, 2021 11:42
@Snuffleupagus Snuffleupagus changed the title Re-factor document.getElementsByName lookups in AnnotationLayer (issue 14003) Re-factor document.getElementsByName lookups in the AnnotationLayer (issue 14003) Sep 14, 2021
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@Snuffleupagus Snuffleupagus force-pushed the AnnotationLayer-getElementsByName branch from f54431a to 3b4dea4 Compare September 14, 2021 11:59
calixteman
calixteman reviewed Sep 14, 2021
View reviewed changes
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an integration test please ?
Anyway it looks good overall.

@Snuffleupagus Snuffleupagus force-pushed the AnnotationLayer-getElementsByName branch 2 times, most recently from bc0b248 to b9f9ae6 Compare September 14, 2021 19:41
This was referenced Sep 15, 2021
Closed
Merged
@Snuffleupagus Snuffleupagus force-pushed the AnnotationLayer-getElementsByName branch 2 times, most recently from 5976a51 to 6cf4142 Compare September 15, 2021 19:42
Rob--W
Rob--W reviewed Sep 16, 2021
View reviewed changes
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started looking but haven't had the chance to take a very close look. A question follows below + small nit about use of self.

@Snuffleupagus Snuffleupagus force-pushed the AnnotationLayer-getElementsByName branch from 6cf4142 to 1ddc633 Compare September 17, 2021 07:29
calixteman added a commit to calixteman/pdf.js that referenced this pull request Sep 17, 2021
@calixteman
Integration test for PR mozilla#14023
2d551ea
@calixteman
Copy link
Contributor

@Snuffleupagus here is an integration test: calixteman@2d551ea

@Snuffleupagus
Copy link
Collaborator Author

@Snuffleupagus here is an integration test: calixteman@2d551ea

Thank you, thank looks fantastic!

Snuffleupagus pushed a commit to Snuffleupagus/pdf.js that referenced this pull request Sep 17, 2021
@calixteman @Snuffleupagus
Integration test for PR mozilla#14023
b1daf3b
@mozilla mozilla deleted a comment from pdfjsbot Sep 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Sep 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Sep 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Sep 17, 2021
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/7948e35c756ad38/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/52d7931e3f91535/output.txt

@Snuffleupagus Snuffleupagus force-pushed the AnnotationLayer-getElementsByName branch from 6040ae3 to 386acf5 Compare September 23, 2021 11:05
@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/cb9814ec02fa851/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/cb9814ec02fa851/output.txt

Total script time: 2.78 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

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

Total script time: 5.59 mins

  • Unit Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/45a43b1af76969a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/45a43b1af76969a/output.txt

Total script time: 3.62 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

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

Total script time: 6.93 mins

  • Integration Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

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/f44028359798898/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/54a2b22e6f59757/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/f44028359798898/output.txt

Total script time: 21.69 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 8
  different first/second rendering: 2

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/54a2b22e6f59757/output.txt

Total script time: 32.92 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  errors: 628
  different ref/snapshot: 11

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

@Snuffleupagus Snuffleupagus merged commit c914e9f into mozilla:master Sep 23, 2021
@Snuffleupagus Snuffleupagus deleted the AnnotationLayer-getElementsByName branch September 23, 2021 12:06
calixteman added a commit to calixteman/pdf.js that referenced this pull request Sep 26, 2021
@calixteman
AcroForm: Add support for ResetForm action
bc90ea4 
  - 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.
@calixteman calixteman mentioned this pull request Sep 26, 2021
Merged
calixteman added a commit to calixteman/pdf.js that referenced this pull request Sep 27, 2021
@calixteman
AcroForm: Add support for ResetForm action
3bcef10 
  - 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.
calixteman added a commit to calixteman/pdf.js that referenced this pull request Sep 28, 2021
@calixteman
AcroForm: Add support for ResetForm action
3bd1924 
  - 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.
calixteman added a commit to calixteman/pdf.js that referenced this pull request Sep 30, 2021
@calixteman
AcroForm: Add support for ResetForm action
aecbd7c 
  - 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.
bh213 pushed a commit to bh213/pdf.js that referenced this pull request Jun 3, 2022
@calixteman @bh213
Integration test for PR mozilla#14023
b9ca2a2
bh213 pushed a commit to bh213/pdf.js that referenced this pull request Jun 3, 2022
@calixteman @bh213
AcroForm: Add support for ResetForm action
3eb48a7 
  - 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.
@calixteman calixteman mentioned this pull request Jun 24, 2022
Closed
rousek pushed a commit to signosoft/pdf.js that referenced this pull request Aug 10, 2022
@calixteman @rousek
Integration test for PR mozilla#14023
d66e112
rousek pushed a commit to signosoft/pdf.js that referenced this pull request Aug 10, 2022
@calixteman @rousek
AcroForm: Add support for ResetForm action
999225a 
  - 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.
@calixteman calixteman mentioned this pull request Aug 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rob--W Rob--W Rob--W approved these changes

@calixteman calixteman calixteman approved these changes

Assignees
No one assigned
Labels
annotations core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hazard: annotation_layer.js uses document.getElementsByName without verification
4 participants
@Snuffleupagus @calixteman @pdfjsbot @Rob--W