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.

Sign up for GitHub

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

Jump to bottom

Internal links are broken (Cairo bug) #678

Closed
Tontyna opened this issue Aug 26, 2018 · 15 comments
Closed

Internal links are broken (Cairo bug) #678

Tontyna opened this issue Aug 26, 2018 · 15 comments
Milestone
43

Comments

@Tontyna
Copy link
Contributor

Tontyna commented Aug 26, 2018 •
edited
Loading

In commit "Don't use pdfrw anymore" 0834615 @liZe commented

some internal links are broken

May I ask which internal links still do work?

Because it looks to me like all internal links are broken. Nothing special about my ids, but only external hrefsare clickable.

Edit: Cairo v 1.15.12
@Tontyna
Copy link
Contributor Author

Tontyna commented Aug 27, 2018 •
edited
Loading

AH! Got it:

Internal links dont work if the target is on a page coming after the <a>'s page.
If the target is on the same or an earlier page they are clickable.

That means: write_pdf should draw the pages and collect the hyperlinks in 'Step 6 - Drawing' and add the hyperlinks in 'Step 7 - Adding PDF metadata'.

Maybe adding the links with CAIRO_TAG_DEST instead of CAIRO_TAG_LINK could do the trick? Of course then we need not only add_hyperlinks() but also a add_destinations().

BTW: Wondering why not issuing context.tag_end(cairo.TAG_LINK) doesnt upset Cairo.

@Tontyna
Copy link
Contributor Author

Tontyna commented Aug 27, 2018

... but it seems there isnt a function to reactivate an already emitted surface ...

@liZe
Copy link
Member

liZe commented Aug 27, 2018

It's a bug in Cairo, I've submitted a patch: https://lists.cairographics.org/archives/cairo/2018-August/028716.html

BTW: Wondering why not issuing context.tag_end(cairo.TAG_LINK) doesnt upset Cairo.

We should call tag_end, it's useless for now in our case but that's just a Cairo's implementation detail.

@Tontyna Tontyna changed the title No more clickable internal links Internal links are broken (Cairo bug) Aug 27, 2018
@Tontyna
Copy link
Contributor Author

Tontyna commented Aug 28, 2018

Oh dear! Waiting for Cairo... (since Bryce Harrington took notice, the prospects are not too bad)

Until then I'll patch my personal WeasyPrint to stay with pdfrw. Or maybe I'll patch your patch into my personal Cairo -- quoting myself: it won't harm me when I start using my MSYS2 shell and brain and learn something new...

@liZe
Copy link
Member

liZe commented Aug 28, 2018

Oh dear! Waiting for Cairo... (since Bryce Harrington took notice, the prospects are not too bad)

Yes, after the review I have to fix my fix now 😄.

Or maybe I'll patch your patch into my personal Cairo -- quoting myself: it won't harm me when I start using my MSYS2 shell and brain and learn something new...

Good luck!

@Tontyna
Copy link
Contributor Author

Tontyna commented Aug 28, 2018 •
edited
Loading

Out of interest: Why do you communicate this issue via mailing list and not via GitLab?

@liZe
Copy link
Member

liZe commented Aug 28, 2018

Out of interest: Why do you communicate this issue via mailing list and not via GitLab?

Cairo's GitLab has only been created yesterday 😉.

There was a Bugzilla instance, but I wasn't sure that it was a bug at the beginning, so … the ML was a good option for me to get extra information before creating an issue.

@Tontyna
Copy link
Contributor Author

Tontyna commented Sep 1, 2018

Will it speed up the fixing when I create an account on GitLab and 👍 the issue?

Until now we could tell people in need of new features like flex, targets etc. to download master branch, but as long as that bug persists I cant recommend that anymore.

@Tontyna Tontyna mentioned this issue Sep 4, 2018
Merged
@liZe
Copy link
Member

liZe commented Sep 5, 2018

Will it speed up the fixing when I create an account on GitLab and the issue?

Only Cairo devs know that…

Until now we could tell people in need of new features like flex, targets etc. to download master branch, but as long as that bug persists I cant recommend that anymore.

Yes, that's really annoying. I've merged the PR soon to avoid conflicts, but the master branch is really experimental now…

Tontyna added a commit to Tontyna/WeasyPrint that referenced this issue Sep 5, 2018
@Tontyna
Broken hyperlinks in Cairo - reintroduce pdfrw
699412c 
Allows falling back on pdfrw to outlast until Cairo has fixed the
hyperlink bug, see Kozea#678

pdfrw is off by default.
To turn it on define environmen variable WEASYPRINT_USE_PDFRW
Fallback is disabled when pytesting
@Tontyna
Copy link
Contributor Author

Tontyna commented Sep 5, 2018

Will keep that branch in sync with master until the bug is fixed.

@liZe
Copy link
Member

liZe commented Sep 21, 2018

Will it speed up the fixing when I create an account on GitLab and the issue?

1.15.14 has been released as a beta for 1.16.0. I've added a comment on GitLab, but I'm a bit afraid I don't get a review before the stable release.

I don't understand how to talk with Cairo devs. I'm getting really frustrated 😢.

@Tontyna
Copy link
Contributor Author

Tontyna commented Sep 21, 2018

I'm watching activities on GitLab since and don't understand the rules for communication, too. Maybe the real communication happens offline?
But: Adrian Johnson obviously has started to deal with hyperlink problems in PDF export so maybe maybe maybe...

@Tontyna
Copy link
Contributor Author

Tontyna commented Sep 22, 2018

Indeed, Adrian commented

... you are welcome to change it. [...] as long as the tests pass and Bryce is happy, I don't mind what you do with it.

How to make Bryce happy? More patches?

I suggest to implement what Adrian intended when he added pdf links:
Apply CAIRO_TAG_DEST to the intenal links and their destinations (and hope it works as expected).
If your patch eventually enters Cairo we can always switch back to CAIRO_TAG_LINK.

A dysfunctional master branch is ugly.

@liZe - shall I / will you rewrite document.write_pdf()? Or shall we wait another fortnight for happiness to arise?

@liZe
Copy link
Member

liZe commented Sep 24, 2018

How to make Bryce happy? More patches?

Probably with a test, I've begun to work on it this week-end.

I suggest to implement what Adrian intended when he added pdf links:
Apply CAIRO_TAG_DEST to the intenal links and their destinations (and hope it works as expected).
If your patch eventually enters Cairo we can always switch back to CAIRO_TAG_LINK.

Of course.

@liZe - shall I / will you rewrite document.write_pdf()? Or shall we wait another fortnight for happiness to arise?

I'll do it 💣 RIGHT NOW 💣.

@liZe liZe closed this as completed in dfab8a8 Sep 24, 2018
@liZe liZe added this to the 43 milestone Sep 24, 2018
@Tontyna
Copy link
Contributor Author

Tontyna commented Sep 24, 2018

👍 👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
43
Development

No branches or pull requests
2 participants
@liZe @Tontyna