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

Fixed annotation getting rotated when moving between pages #18272

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

Conversation

pilipovicmilan04
Copy link

@pilipovicmilan04 pilipovicmilan04 commented Jun 18, 2024

BUG: It has an issue that dragging an ink annotation between rotated pages causes weird behavior.

How to reproduce:
Open this pdf https://github.com/mozilla/pdf.js/blob/master/test/pdfs/rotation.pdf
Draw ink annotation
Try to drag the ink annotation between rotated pages and it will not work properly.

pdf4.webm

@Snuffleupagus
Copy link
Collaborator

It has an issue that dragging an ink annotation between rotated pages causes weird behavior.

Is this actually specific to the InkEditor though, since it seems that all Editors are similarly affected?

Also, so that this can be reviewed, please squash the commits such that the commit history is clean.

@pilipovicmilan04
Copy link
Author

Is this actually specific to the InkEditor though, since it seems that all Editors are similarly affected?

Also, so that this can be reviewed, please squash the commits such that the commit history is clean.

Seems others are also not working. Checking if I can fix them. @Snuffleupagus

@calixteman
Copy link
Contributor

Firstly, thank you for your contribution and for having found this bug.
It'd be nice to add some integration tests.
You can try in using the rotation.pdf file, in setting the odd spread mode (to have the two pages in the viewport) and in setting zoom to pageFit (which is the default with integration tests).
You can use dragAndDropAnnotation function.

@pilipovicmilan04
Copy link
Author

Fixed for all editor types @Snuffleupagus @calixteman

@pilipovicmilan04
Copy link
Author

pilipovicmilan04 commented Jun 18, 2024

Just to know, is there any other way to fix this issue? I'd like to know if there's any other optimized way to do this. @calixteman @Snuffleupagus It would be great to get your feedback on my solution 🙏

this.x = (x - layerX) / width;
this.y = (y - layerY) / height;

if (newRotation === 90) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simplify the two cases 90 and 270 in using newRotation % 180 !== 0

@calixteman
Copy link
Contributor

I added some code to change the spread mode when loading the pdf in the integration tests:
https://github.com/mozilla/pdf.js/pull/18275/files#diff-6ead6f27780ca257e3f28e9f66dac1b8d6b5af135e4bf4e8ae0844dc37540caeR758-R768

@calixteman
Copy link
Contributor

Just to know, is there any other way to fix this issue? I'd like to know if there's any other optimized way to do this. @calixteman @Snuffleupagus It would be great to get your feedback on my solution 🙏

Overall, it looks good to me but with a test for freetext and ink annotations.
Once the tests are written I'll review your patch.

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

Successfully merging this pull request may close these issues.

3 participants