Provide href attribute support in a elements
#313
Conversation
|
Thanks! At the very least we should build upon the fix for the getBoundingBoxByChildren function - however implementing it properly with the transformation should not be too difficult, either, so I guess this should be done, too, because even though translations are common, scales are probably at least as common and this seems to fail. Is this function never used in other contexts? I wonder how it could be that broken.... That said, it would be very helpful, if you could come up with a few test cases, ideally some that show the various problematic combinations with transforms etc. so that we have test cases, even if they fail. |
|
If I am right - I cannot use I add commit which directly applies x/y axis scales from the matrix. As primary example one can use SVG from developer.mozilla.org. Also here is SVG with some transform attributes:
|
src/nodes/group.ts
Outdated
|
|
||
| export class GroupA extends Group { | ||
| protected async renderCore(context: Context): Promise<void> { | ||
| await super.renderCore(context) | ||
|
|
||
| const href = getAttribute(this.element, context.styleSheets, 'href') | ||
| if (href) { | ||
| const box = this.getBoundingBox(context) | ||
| const scale = context.pdf.internal.scaleFactor | ||
| const ph = context.pdf.internal.pageSize.getHeight() | ||
|
|
||
| context.pdf.link(scale*(box[0] * context.transform.sx + context.transform.tx), | ||
| ph - scale*(box[1] * context.transform.sy + context.transform.ty), scale*box[2], scale*box[3], { url: href }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I would suggest renaming the class to "Anchor" and moving it to its own file.
src/utils/bbox.ts
Outdated
| if ((nodeBox[0] === 0) && (nodeBox[1] === 0) && (nodeBox[2] === 0) && (nodeBox[3] === 0)) | ||
| return; | ||
| const transform = child.computeNodeTransform(context) | ||
| nodeBox[0] = nodeBox[0] * transform.sx + transform.tx | ||
| nodeBox[1] = nodeBox[1] * transform.sy + transform.ty | ||
| if (boundingBox.length === 0) | ||
| boundingBox = nodeBox; | ||
| else |
There was a problem hiding this comment.
The width and height of the bounding box also need to be transformed. You can use the multVecMatrix function from geometry.ts
Do not use dummy (all 0) bounding box when search bonding box for all children. Otherwise minimal values always will be 0. Also take into account transformation attributes of child node - it may differ from child to child. Apply scale and translation, but not rotation yet
While `a` element typically used together with text, one need to have estimation of bounding box for text. To avoid reimplementation of complete text rendering logic just remember text position and sizes as bounding box. Implemented only for plain text, any usage of tspan will not work properly
It derives from Group class and uses same rendering. Plus if `href` attribute specified - adds link with bounding box. Requires usage of scale and page height values - otherwise created link area does not match drawn text
This test also can be used to demonstrate missing support of dominant-baseline attribute of the text
|
I rebase PR, introducing I did not try to use I add |
There was a problem hiding this comment.
Thanks for adding test cases. The dominant-baseline attribute is being implemented in #286.
I would love some test cases with links on elements other than text and maybe some rotated elements after rotation is considered in the bounding box.
But one need to implement bounding box for such elements. |
|
I tried to implement bounding box using Not sure that method works properly - see produced pdf in the Here three different transformations are applied - rotation, translate X, translate Y. |
|
You're right. The bounding boxes of the rotated elements are not correct. The |
|
Can we split this? One PR is creation of URL link for Second is proper calculation of bounding box in case of rotated And maybe third is testing if bounding box for all other elements like |
|
Yes, sure. |
Then I removing last two commits and will create another PR - once this is finalized |
There was a problem hiding this comment.
Please update the reference files for the failed tests. They fail because of the changed bounding box implementation. You can download the files from the last CI run: https://github.com/yWorks/svg2pdf.js/actions/runs/11701142768?pr=313
Changes because of new implementation of bounding box
|
Yes, I did it |
This PR provide possible solution for #312
To be able use
jsPDF.link()method, one need to have valid bounding box for all child nodes ofaelement.Seems to be, existing implementation of
getBoundingBoxByChildrenfunction is not correct.First commit excludes dummy box (all 0) from consideration.
Also transformation attributes from child nodes are taken into account - for now only translations.
While
textnode is most probable candidate to be used insideaelement - one have to provide reasonable bounding box implementation for it. In the PR second method is just workaround to avoid re-implementation of rendering again to just get bounding box. Provided solution works only for plaintextwithout tspan elements inside.Last commit implements
GroupAclass which extends normalGrouprendering and creates link in the PDF document.Here some internals of jsPDF context are used - maybe better solution can be provided.