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

Make sure that Popup is rendered next to trigger for textAnnotation #12564

Merged
merged 1 commit into from
Nov 5, 2020
Merged

Make sure that Popup is rendered next to trigger for textAnnotation #12564

merged 1 commit into from
Nov 5, 2020

Conversation

hakubo
Copy link
Contributor

@hakubo hakubo commented Nov 3, 2020

Popup for textAnnotations can be rendered in a wrong position.
Example:
abc (2).pdf

After this fix all popup annotations are rendered in a correct place.

@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 3, 2020 •
edited
Loading

Looks good, but please add a reference test for this to prevent future regressions. You can use this commit as a reference for how to do that. In short:

  • Put the file from the PR description in the test/pdfs folder as pr12564.pdf.
  • Add this same file to test/pdfs/.gitignore to be able to commit it.
  • Add an entry to test/test_manifest that describes your test, similar to 34918a6#diff-9bc50ce4ba8a4ba8d322114a3cc05fa62005a40b52223059200ca07087dd5a61. In this case also add "firstPage": 1 and "lastPage": 1 to the entry because we only need to test the first page since the problem already occurs there. The following should work:
    {  "id": "pr12564",
       "file": "pdfs/pr12564.pdf",
       "md5": "8033857922eb03d050f6a04c2f5e9851",
       "rounds": 1,
       "firstPage": 1,
       "lastPage": 1,
       "type": "eq",
       "annotations": true
    },

@hakubo
Copy link
Contributor Author

hakubo commented Nov 4, 2020

@timvandermeij sure!
I compressed the pdf a bit so it's a bit smaller in the repo.

Thanks for looking into it.

Snuffleupagus
Snuffleupagus reviewed Nov 4, 2020
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Also, please see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits

@hakubo
Copy link
Contributor Author

hakubo commented Nov 4, 2020

@Snuffleupagus squished to one commit! :)

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/0449f7e34f4ef58/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://3.101.106.178:8877/739c7c0473bff57/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/0449f7e34f4ef58/output.txt

Total script time: 2.29 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/0449f7e34f4ef58/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/739c7c0473bff57/output.txt

Total script time: 2.82 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/739c7c0473bff57/reftest-analyzer.html#web=eq.log

@hakubo
Copy link
Contributor Author

hakubo commented Nov 4, 2020

/botio test

@hakubo
Copy link
Contributor Author

hakubo commented Nov 4, 2020

looks I don't have that kind of power to run tests :)

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://3.101.106.178:8877/37c2ef7982f1f9f/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/0ceddc49f7ed4e4/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/0ceddc49f7ed4e4/output.txt

Total script time: 2.17 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/0ceddc49f7ed4e4/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Nov 4, 2020

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/37c2ef7982f1f9f/output.txt

Total script time: 2.77 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/37c2ef7982f1f9f/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator

Once more, please make sure that your patch passes all tests locally; more information at https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing

There's no point in attempting to run tests here again, until you've confirmed that your patch passes all tests locally!

@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 4, 2020 •
edited
Loading

This PR needs a rebase onto the current master. Unfortunately another patch also touched the same code, but on a positive note it already added the popupTop variable, which should make this patch even smaller. If I looked at it correctly, just setting the top line should now be enough.

Moreover, I think we should also add the top change to https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js#L303 so it also works properly for annotations without explicit popup annotations.

@hakubo
Copy link
Contributor Author

hakubo commented Nov 5, 2020

@timvandermeij rebased on current master.

If I get it right, https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js#L303 this renders popup within annotation section element. To these are positioned correctly as they will be positioned relative to the annotation section element.

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://3.101.106.178:8877/8e8af756f14461d/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/f2eca61bd0940e1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/f2eca61bd0940e1/output.txt

Total script time: 24.82 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/f2eca61bd0940e1/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/8e8af756f14461d/output.txt

Total script time: 28.64 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/8e8af756f14461d/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit be0794c into mozilla:master Nov 5, 2020
@timvandermeij
Copy link
Contributor

Thank you for your contribution! (The reference test failures are not relevant to this PR.)

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 2

Live output at: http://3.101.106.178:8877/4348f7f95c0fe58/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Linux m4)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/65fe7f5db650a35/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/65fe7f5db650a35/output.txt

Total script time: 23.90 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

pdfjsbot commented Nov 5, 2020

From: Bot.io (Windows)

Success

Full output at http://3.101.106.178:8877/4348f7f95c0fe58/output.txt

Total script time: 28.36 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@hakubo hakubo deleted the patch-1 branch November 6, 2020 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus left review comments

@timvandermeij timvandermeij timvandermeij approved these changes

Assignees
No one assigned
Labels
annotations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hakubo @timvandermeij @pdfjsbot @Snuffleupagus