JS - Don't use document.getElementsByName when updating some annotations #14008
Conversation
calixteman
commented
Sep 12, 2021
calixteman
commented
- it aims to fix Hazard: annotation_layer.js uses document.getElementsByName without verification #14003.
- in a custom viewer, the use of document.getElementsByName could lead to get some elements which don't correspond to an annotation, so this patch removes it and annotations with the same fieldName are collected when they're created.
- it aims to fix mozilla#14003. - in a custom viewer, the use of document.getElementsByName could lead to get some elements which don't correspond to an annotation, so this patch removes it and annotations with the same fieldName are collected when they're created.
| @@ -2049,7 +2049,8 @@ class AnnotationLayer { | |||
| */ | |||
| static render(parameters) { | |||
| const sortedAnnotations = [], | |||
| popupAnnotations = []; | |||
| popupAnnotations = [], | |||
| sameName = new Map(); | |||
There was a problem hiding this comment.
Could annotations with the same name ever be found on different pages, because if so I'm not sure that this will work (since it's limited to just the current page)?
There was a problem hiding this comment.
And I'd say that it was already an issue (I mean before this patch).
There was a problem hiding this comment.
My (possibly very stupid) idea when I initially saw the issue was that you could maybe track the created input elements in e.g. a WeakSet defined "globally" in this file, and then just check the elements that we lookup with document.getElementsByName against that.
Based on the following quote from #14003 (comment), Rob probably has better ideas though:
I suggest to replace the document.getElementsByTagName calls with a function that checks that the elements are created by the annotation_layer.js code before returning them. There are several ways to do so, e.g. by adding a custom attribute to each input element.
There was a problem hiding this comment.
It won't avoid the problem with fields with same name across different pages: document.getElementsByName will get only the visible elements.
There was a problem hiding this comment.
And I'd say that it was already an issue (I mean before this patch).
I see what you're saying, but it seems to me (I may be wrong of course) that it'd be slightly worse since before you got the Annotations on all active pages whereas with this patch it's only the current page.
Anyway, it seems that we might need to expand the AnnotationStorage to also have a name -> id mapping such that the initial value gets set correctly when rendering new pages? If so, that probably shouldn't be too hard to implement (famous last words).
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
When I filed the issue I was thinking of something simple like using a WeakSet, tagging the element with a custom attribute, or even just prepending a prefix to the name to have something unique that can't overlap with existing ones. This draft patch looks more complex than that - why?
There was a problem hiding this comment.
@calixteman Sorry if I'm massively overstepping here, but I've got a (possibly stupid) idea.
What if we utilized the data returned by the getFieldObjects-method in the API, since then we'd be able to both access the fields on all active pages and also update the AnnotationStorage for not-yet-rendered pages?
There was a problem hiding this comment.
@Snuffleupagus, no, you aren't and I think that's a very good idea.
This way we don't have to change AnnotationStorage implementation because we'll have all the ids for fields with the same name. Would you mind to write the patch since you've almost thought about everything here ? If yes, please close this one.
@Rob--W, my idea was to remove the call to getElementsByName and just update the fields with the same name. But as pointed out by Jonas, it was a bad idea and in the meantime it was not so bad because it helped us to find a bug.
|
@calixteman – while eliminating
@Snuffleupagus pre-defense text
I know the use cases of Mozilla (with the viewer) but PDF.js as a library is very capable and generic (and able to power a lot more than just the whole document viewer it is bundled with) and |
|
Re #14008 (comment), please let's not move the "goalpost" here since this PR is specifically about fixing issue #14003 and not something else (e.g. shadow DOM)? |
|
Closing in favour of PR #14023. |
