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

Add link tags for pdfs #2405

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mcfedr
Copy link
Contributor

@mcfedr mcfedr commented Jul 17, 2024

Interested to see if people think this is a reasonable implementation.

I went with as simple as possible based off https://www.cairographics.org/manual/cairo-Tags-and-Links.html#link

I know its not standard api, but there isn't one for this in canvas, and as was mentioned in the linked issue the project should be open to such an addition.

Fixes #2044

  • Have you updated CHANGELOG.md?

src/Canvas.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sort of like addHitRegion() (RIP) or the proposed HitRegionSet (whatwg/html#3407).

If we add a non-standard API (generally avoided), we should make sure it covers all foreseeable use cases to avoid having to add another non-standard API in the future. In this case, we might want to (prepare to) expose the full Cairo API for tags (i.e. support specifying an array of rects as the hit region).

+0 from me.

src/CanvasRenderingContext2d.cc Outdated Show resolved Hide resolved
@mcfedr
Copy link
Contributor Author

mcfedr commented Jul 22, 2024

This is sort of like addHitRegion() (RIP) or the proposed HitRegionSet (whatwg/html#3407).

These proposals seem to mostly be dead or nearly dead, I think the best thing would just to avoid being close to them in any naming

If we add a non-standard API (generally avoided), we should make sure it covers all foreseeable use cases to avoid having to add another non-standard API in the future

I agree, I think would be good to be able to support the different tag types that cairo can support.

cairo_tag_begin

I see a couple of options

  • api like in cairo, that takes two strings, tag_name and attributes - then this simple use case is a bit harder, as the user has to provide the tag name 'Link' and the uri=... attributes

  • provide some sort of conversion of the attributes from an object into the format expected by cairo, this would be more work now

  • maybe we assume that second param can be an object in the future, but for now take a string, and assume that its a uri

  • if we want the use to provide the tag name, i.e. "Link" do we want to expose the const CAIRO_TAG_LINK as a const on Context2d - but that would be more non-standard API surface.

I've be very happy to adjust this implementation to something that can work along these lines

@mcfedr
Copy link
Contributor Author

mcfedr commented Aug 13, 2024

@zbjornson I've made some changes to make it simple for this API to be extended in the future.

I've kept only support for Link with uri, and leave it as a task for someone who needs it to add support for more types, but with the object based config for tag, this should be simple to do.

I've also added escaping and check for ascii.

@mcfedr mcfedr requested a review from zbjornson August 13, 2024 09:15
@mcfedr mcfedr force-pushed the link-tags branch 3 times, most recently from e9ceca8 to 1abe048 Compare October 16, 2024 12:56
@mcfedr
Copy link
Contributor Author

mcfedr commented Oct 28, 2024

@chearon would you review this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Insert Hyperlink in PDF
2 participants