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

fix: copy paste for registered font #2408

Closed
wants to merge 0 commits into from
Closed

Conversation

chihyux
Copy link
Contributor

@chihyux chihyux commented Oct 8, 2023

Hello! Thank you for creating this package. My company has chosen this package, and I encountered an issue when using a custom font where copying text results in garbled characters. I have also noticed that there are many similar issues in the repository, where the characters copied from the generated PDF are different from what is displayed on the screen.

I have found that the package uses an older version of pdfkit, and based on the code in the latest version of that package, the following function is no longer used:

 this.unicode[gid] = this.font._cmapProcessor.codePointsForGlyph(
            glyph.id
          );

The reason for this change is that the function was producing duplicate Unicode values, leading to consecutive or incorrect characters in the cmap and resulting in garbled text when copying.

Additionally, when using this package in my country, which uses Chinese characters, copying text from the generated document in Chrome and Edge browsers can also result in garbled characters when there are a large number of Unicode characters. I suspect that this is because the original implementation toUnicodeCmap used bfrange for generating Unicode mappings for glyphs, and when there are many Unicode characters(more than 244 characters), the browser cannot correctly interpret the corresponding Unicode values. To address this issue, I suggest changing the cmap implementation to use bfchar, which resolved the problem.

@changeset-bot
Copy link

changeset-bot bot commented Oct 8, 2023

⚠️ No Changeset found

Latest commit: e5c8fde

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@chihyux chihyux changed the title fix: copy paste for registered font Fix copy paste for registered font Oct 9, 2023
irian-codes added a commit to irian-codes/personal-site that referenced this pull request Oct 18, 2023
So seems that some fonts cause issues when they are copied and
pasted. And ATS software reads text that way probably so I needed
to fix it. Otherwise I'd be stuck with default fonts.

I've created this patch package based on this pending PR
diegomura/react-pdf#2408 that fixes
the issue so now I can use any font!
@santialbo
Copy link

Please @diegomura have a look at this PR if you have some time. This has been a very problematic issue for us :(

@diegomura diegomura changed the title Fix copy paste for registered font fix: copy paste for registered font Oct 26, 2023
Copy link
Owner

@diegomura diegomura left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me comparing to latest pdfkit.Will try to test it today and merge it

@teq-vunguyen
Copy link

@diegomura I also got this issue, waiting for next release 🙏🏻

@teq-vunguyen
Copy link

@chihyux I have used your PR and customized it into a separate node module, and it works 100%. I really appreciate that 💯

@hazem3500
Copy link

Was anyone able to monkey patch this with patch-package by any chance? 🤔 or deployed a forked package to NPM?

Found the code on the node_modules minified so would need some dissecting to patch 😅

@sandeepdotcode
Copy link

@hazem3500 If you haven't already got it to work, I have it in patches folder in this repo. You can just raw download the file and run patch-package.

Thanks for the code @chihyux !

@cupcakearmy
Copy link

cupcakearmy commented Nov 9, 2023

Everything works great

We added the patch to our codebase, and all tests are running fine + we can now actually copy test. can confirm the fix works.

How to use the patched version for now?

git clone [email protected]:chihyux/react-pdf.git
cd react-pdf
yarn
yarn build
tar -czvf renderer.tar.gz ./packages/renderer

Then simply create a tarball of the relevant packages. and -> "@react-pdf/renderer": "./renderer.tar.gz" in your package json.

As a bonus you probably should add git lfs for tarballs, and also into your checkout action, to not pollute your git repo

Hope this gets merged soon :)

EDIT: Actually did not solve it.

What helped: go to https://www.tophix.com/font-tools/font-editor just import/export the font and everything seems to work.

@teq-vunguyen
Copy link

I use the patch-package library to perform a find-and-replace operation, similar to the pull request mentioned above, in the file node_modules/@react-pdf/pdfkit/lib/pdfkit.browser.es.js. It is truly effective with a success rate of 100%. I don't know if there's anything wrong here 🤔 @hazem3500

@kaloyanBozhkov
Copy link

+1 waiting on this

@kaloyanBozhkov
Copy link

What was mentioned by @teq-vunguyen worked for me as well. I had to do this since bun was not happy with patch-package, but overall everything went good. Also thanks to @sandeepdotcode's patch.

Let's get this merged! Probably many resume websites are having issues due to this 🤯

@hyalen
Copy link

hyalen commented Dec 7, 2023

Do we have plans in merging this PR asap? I'm facing some weird copy+paste issues right now, and it'd be awesome to have it fixed 😄

@Elelan
Copy link

Elelan commented Jan 5, 2024

When will this be merged?

@kaloyanBozhkov
Copy link

Let's merge this bad boy

@diegomura
Copy link
Owner

Changes merged in #2488

Sorry guys, specially @chihyux, messed up this PR branch when rebasing and testing changes. All looked good. Thanks so much for the contribution!

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.

10 participants