-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Accessibility: Fixed Link annotation is not nested inside a Link structure element #1664
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
Changes from all commits
cd10b3c
fd207a4
03abf95
a899108
5b177d0
bd8e7e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ playground/ | |
| build/ | ||
| js/ | ||
| .vscode | ||
| .idea | ||
| coverage | ||
| package-lock.json | ||
| /examples/browserify/bundle.js | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| var PDFDocument = require('../'); | ||
| var fs = require('fs'); | ||
|
|
||
| // Create a new PDFDocument | ||
| var doc = new PDFDocument({ | ||
| autoFirstPage: true, | ||
| bufferPages: true, | ||
| pdfVersion: '1.5', | ||
| // @ts-ignore PDF/UA needs to be enforced for PAC accessibility checker | ||
| subset: 'PDF/UA', | ||
| tagged: true, | ||
| displayTitle: true, | ||
| lang: 'en-US', | ||
| fontSize: 12, | ||
| }); | ||
|
|
||
| doc.pipe(fs.createWriteStream('accessible-links.pdf')); | ||
|
|
||
| // Set some meta data | ||
| doc.info['Title'] = 'Test Document'; | ||
| doc.info['Author'] = 'Devon Govett'; | ||
|
|
||
| // Initialise document logical structure | ||
| var struct = doc.struct('Document'); | ||
| doc.addStructure(struct); | ||
|
|
||
| // Register a font name for use later | ||
| doc.registerFont('Palatino', 'fonts/PalatinoBold.ttf'); | ||
|
|
||
| // Set the font and draw some text | ||
| struct.add( | ||
| doc.struct('P', () => { | ||
| doc | ||
| .font('Palatino') | ||
| .fontSize(25) | ||
| .text('Some text with an embedded font! ', 100, 100); | ||
| }), | ||
| ); | ||
|
|
||
| // Add another page | ||
| doc.addPage(); | ||
|
|
||
| // Add some text with annotations | ||
| var linkSection = doc.struct('Sect'); | ||
| struct.add(linkSection); | ||
|
|
||
| var paragraph = doc.struct('P'); | ||
| linkSection.add(paragraph); | ||
|
|
||
| paragraph.add( | ||
| doc.struct('Span', () => { | ||
| doc | ||
| .font('Palatino') | ||
| .fillColor('black') | ||
| .text('This is some text before ', 100, 100, { | ||
| continued: true, | ||
| }); | ||
| }), | ||
| ); | ||
|
|
||
| paragraph.add( | ||
| doc.struct( | ||
| 'Link', | ||
| { | ||
| alt: 'Here is a link! ', | ||
| }, | ||
| () => { | ||
| doc.fillColor('blue').text('Here is a link!', { | ||
| link: 'http://google.com/', | ||
| underline: true, | ||
| continued: true, | ||
| }); | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| paragraph.add( | ||
| doc.struct('Span', () => { | ||
| doc.fillColor('black').text(' and this is text after the link.'); | ||
| }), | ||
| ); | ||
|
|
||
| paragraph.end(); | ||
| linkSection.end(); | ||
|
|
||
| // End and flush the document | ||
| doc.end(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import PDFAnnotationReference from '../structure_annotation'; | ||
|
|
||
| export default { | ||
| annotate(x, y, w, h, options) { | ||
| options.Type = 'Annot'; | ||
|
|
@@ -19,6 +21,9 @@ export default { | |
| options.Dest = new String(options.Dest); | ||
| } | ||
|
|
||
| const structParent = options.structParent; | ||
| delete options.structParent; | ||
adyry marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Capitalize keys | ||
| for (let key in options) { | ||
| const val = options[key]; | ||
|
|
@@ -27,6 +32,12 @@ export default { | |
|
|
||
| const ref = this.ref(options); | ||
| this.page.annotations.push(ref); | ||
|
|
||
| if (structParent && typeof structParent.add === 'function') { | ||
| const annotRef = new PDFAnnotationReference(ref); | ||
| structParent.add(annotRef); | ||
| } | ||
|
|
||
| ref.end(); | ||
| return this; | ||
| }, | ||
|
|
@@ -77,6 +88,10 @@ export default { | |
| options.A.end(); | ||
| } | ||
|
|
||
| if (options.structParent && !options.Contents) { | ||
| options.Contents = new String(''); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just started looking into the validation errors around links, and then I saw your PR, great work! When I monkey patch your changes into my code, I am still getting this error: In what instances will the options.Contents be defined here? Because as far as I can see, the If I pass the alt text from the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @tr1stan what are you using to check the issue? I've tested on Acrobat PRO (tho I ran out of free trial just today) and on PAC There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, I've been using this: This is in a document I'm rendering, not on your test document though, but I can't see anything that would test differently between the two. |
||
| } | ||
|
|
||
| return this.annotate(x, y, w, h, options); | ||
| }, | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,6 +99,13 @@ export default { | |
| endMarkedContent() { | ||
| this.page.markings.pop(); | ||
| this.addContent('EMC'); | ||
| if (this._textOptions) { | ||
| delete this._textOptions.link; | ||
| delete this._textOptions.goTo; | ||
| delete this._textOptions.destination; | ||
| delete this._textOptions.underline; | ||
| delete this._textOptions.strike; | ||
|
Comment on lines
+103
to
+107
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is necessary delete those? If possible we should avoid deleting properties
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioned that in the comment - _textOptions are inheriting previous properties, they were inheriting link properties as well 🤔 Fix removes the properties that shouldn't leak between structure elements
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, these properties shouldn't end up in the _textOptions, but that would require larger refactor
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be fair there are other places that follows this pattern. |
||
| } | ||
| return this; | ||
| }, | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| class PDFAnnotationReference { | ||
| constructor(annotationRef) { | ||
| this.annotationRef = annotationRef; | ||
| } | ||
| } | ||
|
|
||
| export default PDFAnnotationReference; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kitchen-sinkis a term meaning 'Everything', sokitchen-sink-accessible-tinydoesn't really make sense. I thinkaccessible-links.js(matching the output pdf) is a better name.