Skip to content
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.

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

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

Merged

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 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
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.

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.

src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Show resolved Hide resolved
@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
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
@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
  - 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 27, 2021
  - 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
  - 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
  - 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
bh213 pushed a commit to bh213/pdf.js that referenced this pull request Jun 3, 2022
  - 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.
rousek pushed a commit to signosoft/pdf.js that referenced this pull request Aug 10, 2022
rousek pushed a commit to signosoft/pdf.js that referenced this pull request Aug 10, 2022
  - 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hazard: annotation_layer.js uses document.getElementsByName without verification
4 participants