Accessibility: Fixed Link annotation is not nested inside a Link structure element #1664
Conversation
| } | ||
|
|
||
| if (options.structParent && !options.Contents) { | ||
| options.Contents = new String(''); |
There was a problem hiding this comment.
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 linkOptions will either be {} or { structParent: ... } - does this not mean that options.Contents is never defined and options.Contents always set to an empty string.
If I pass the alt text from the doc.struct('Link', {alt: 'alt text here'} used in your example, then this solves the problem, like this:
options.Contents = new String(options.structParent.dictionary.data.Alt || ''); - But I confess that I've only just started looking into this library, so it's still new to me.
There was a problem hiding this comment.
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.
Hi, I've been using this:
https://pdfix.net/products/pdfix-desktop-lite/
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.
There was a problem hiding this comment.
kitchen-sink is a term meaning 'Everything', so kitchen-sink-accessible-tiny doesn't really make sense. I think accessible-links.js (matching the output pdf) is a better name.
|
There's some code format error |
I'm not sure I see them, there're many prettier warnings (most of them not in my files) but eslint passes without an issue 🤔 pushed latest branch update with main |
|
According to CI https://github.com/foliojs/pdfkit/actions/runs/20124764259/job/57778504512?pr=1664 , issue is from 2 files this PR touches Checking formatting... |
|
I've identified an issue where the links are not terminated properly and are extended to the texts after them, investigating. |
|
The fix is here, since _textOptions are inheriting previous properties, they were inheriting link properties as well. Fix removes the properties that shouldn't leak between structure elements. Relevant unit test added. |
|
After the fix even the structure tree issues are addressed in PAC, here's the output for accessible-links.pdf: |
| delete this._textOptions.link; | ||
| delete this._textOptions.goTo; | ||
| delete this._textOptions.destination; | ||
| delete this._textOptions.underline; | ||
| delete this._textOptions.strike; |
There was a problem hiding this comment.
Why is necessary delete those?
If possible we should avoid deleting properties
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ideally, these properties shouldn't end up in the _textOptions, but that would require larger refactor
There was a problem hiding this comment.
To be fair there are other places that follows this pattern.
4 participants
What kind of change does this PR introduce?
Fixes #1591
Checklist:
PAC Report:
