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 partial print issue on IE / Edge #7499

Closed
wants to merge 2 commits into from
Closed

Fixed partial print issue on IE / Edge #7499

wants to merge 2 commits into from

Conversation

pixelbits-mk
Copy link

@pixelbits-mk pixelbits-mk commented Jul 22, 2016

Fixed partial print issue for IE / Edge as described in issue #3983.

Issue Description
When printing a PDF from the PDF.js viewer, pages are being cutoff near the bottom of the page.

Steps to reproduce

  1. Open a PDF from PDF.js viewer
  2. Click the "Print" button
  3. Select a printer and continue to print
  4. On the printed output, observe that the bottom portion of the page is cutoff.

Fix Description
The issue was fixed by changing the rendered size of the canvas to 100%, setting the CSS scale factor to 1 for both the width and height, changing the PRINT_OUTPUT_SCALE to be dependent on DPI, and fixing the PT to PX conversion between the viewport and canvas measurements.

Notes
All tests pass.
Confirmed that the issue has been fixed on IE 11.494, Edge 25.1, FF 47.0.1, Chrome 51.0
The PRINT_OUTPUT_SCALE factor has not been changed.

@pixelbits-mk pixelbits-mk changed the title Fixed partial print issue on IE/Edge Fixed partial print issue on IE / Edge Jul 22, 2016
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/28fdb624aa3247a/output.txt

@pixelbits-mk
Copy link
Author

Any status on this PR? Please let me know if any changes are requested to move this forward.

@timvandermeij
Copy link
Contributor

timvandermeij commented Jul 27, 2016

The commit itself looks good to me. We mainly need to do more manual testing before merging this.

CustomStyle.setProp('transformOrigin' , canvas, '0% 0%');

var canvasWrapper = document.createElement('div');
canvasWrapper.style.width = '100%';
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jul 27, 2016

Choose a reason for hiding this comment

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

Is this actually necessary, given that it should already be set with CSS, see

height: 100%;
?

@Snuffleupagus
Copy link
Collaborator

The commit itself looks good to me.

Generally I agree, but I've added a couple of questions/comments above.

We mainly need to do more manual testing before merging this.

Agreed; and this is sort of time-consuming to do. We'd need to ensure that this isn't regressing other printing fixes, e.g. PR #7005.
Note: Given that we're close to the release of the next Firefox version, we probably don't want to land this just before the merge to avoid having to create uplift patches if there're any regressions.

@yurydelendik
Copy link
Contributor

The DPI shall be set now to 150%. See #7677. Thanks.

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.

5 participants