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

print: RFC: Center when printing a PDF with smaller size than the output page. #13102

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Mar 14, 2021

This builds on top of #13100, but this changes printing behavior intentionally
so I thought it was worth discussing separately, to improve the rendering on
test-cases like the one in https://bugzil.la/1697778.

This matches what e.g. Evince does when you print the PDF in there on an A4
printer.

We use margins to center horizontally, and flex to center vertically. The
reasoning for this is that it should have better browser support (though maybe
pdf.js no longer supports browsers without flex support?) and it's just as
simple.

@emilio
Copy link
Contributor Author

emilio commented Mar 14, 2021

@Snuffleupagus
Copy link
Collaborator

We use margins to center horizontally, and flex to center vertically. The
reasoning for this is that it should have better browser support (though maybe
pdf.js no longer supports browsers without flex support?) and it's just as
simple.

I think it's safe to assume that flex-related properties can be used without problems in PDF.js now, based on this compatibility information:

However it seems that Firefox-support is slightly limited, but I assume that's not a problem?

@emilio
Copy link
Contributor Author

emilio commented Mar 14, 2021

However it seems that Firefox-support is slightly limited, but I assume that's not a problem?

Limited in what sense? I wasn't able to infer it from the links :)

@emilio
Copy link
Contributor Author

emilio commented Mar 14, 2021

Anyhow using margins for horizontal centering should be fine too, and as I said is about the same amount of complexity.

@emilio
Copy link
Contributor Author

emilio commented Mar 14, 2021

(But happy to change to flex if you think it's better, it's a matter of setting min-width: 100% and align-content: center instead of margin-left / margin-right).

@Snuffleupagus
Copy link
Collaborator

However it seems that Firefox-support is slightly limited, but I assume that's not a problem?

Limited in what sense? I wasn't able to infer it from the links :)

https://developer.mozilla.org/en-US/docs/Web/CSS/flex-direction#browser_compatibility lists Firefox as "Partial support", hence why I mentioned it.

@emilio
Copy link
Contributor Author

emilio commented Mar 14, 2021

Ah:

Before Firefox 81, overflow with *-reverse is unsupported. See bug 1042151.

So that was about a particular bug that has been addressed a while ago.

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

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

Total script time: 4.21 mins

Published

@timvandermeij
Copy link
Contributor

I think centering the page looks much better, and this also makes sense given that other viewers seem to do this as well. I also think using display: flex is not a problem anymore given that we dropped support for IE completely and all modern browsers support it well. I do think it would be slightly better here to use display: flex also for centering the content horizontally so that the same mechanism is used.

…put page.

This builds on top of mozilla#13100, but this changes printing behavior intentionally
so I thought it was worth discussing separately, to improve the rendering on
test-cases like the one in https://bugzil.la/1697778.

This matches what e.g.  Evince does when you print the PDF in there on an A4
printer.

We use margins to center horizontally, and flex to center vertically. The
reasoning for this is that it should have better browser support (though maybe
pdf.js no longer supports browsers without flex support?) and it's just as
simple.
@emilio
Copy link
Contributor Author

emilio commented Mar 15, 2021

Thanks, updated to use flex to center horizontally too.

@emilio emilio force-pushed the print-small-page branch from 4afc737 to 1d70bfe Compare March 15, 2021 08:52
@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/77f85f7ccb31972/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/77f85f7ccb31972/output.txt

Total script time: 4.04 mins

Published

@brendandahl brendandahl merged commit dd3797a into mozilla:master Mar 16, 2021
@emilio emilio deleted the print-small-page branch March 16, 2021 09:39
emilio added a commit to emilio/pdf.js that referenced this pull request Mar 26, 2021
This improves and simplifies mozilla#13102 in order to make printing of test-cases
like the one in bug 1698414 (where the real page is bigger than the target
page) much better, see incoming screenshots.

The reason why we need to stop setting .style.width / .style.height is to get
the right auto-sizing behavior in both axes. This shouldn't change behavior as
long as the print resolution is >= the CSS resolution, which seems like a
reasonable assumption.

If you try to print with a lower resolution than CSS, then instead of an
stretched canvas, you'd get a centered CSS-quality canvas, which seems
sensible. This could maybe be fixed with some CSS hackery (some combination of
min / max and viewport units perhaps?), but I think it's more trouble than it's
worth.
emilio added a commit to emilio/pdf.js that referenced this pull request Mar 26, 2021
This improves and simplifies mozilla#13102 in order to make printing of test-cases
like the one in bug 1698414 (where the real page is bigger than the target
page) much better, see incoming screenshots.

The reason why we need to stop setting .style.width / .style.height is to get
the right auto-sizing behavior in both axes. This shouldn't change behavior as
long as the print resolution is >= the CSS resolution, which seems like a
reasonable assumption.

If you try to print with a lower resolution than CSS, then instead of an
stretched canvas, you'd get a centered CSS-quality canvas, which seems
sensible. This could maybe be fixed with some CSS hackery (some combination of
min / max and viewport units perhaps?), but I think it's more trouble than it's
worth.
emilio added a commit to emilio/pdf.js that referenced this pull request Mar 26, 2021
This improves and simplifies mozilla#13102 in order to make printing of test-cases
like the one in bug 1698414 (where the real page is bigger than the target
page) much better, see incoming screenshots.

The reason why we need to stop setting .style.width / .style.height is to get
the right auto-sizing behavior in both axes. This shouldn't change behavior as
long as the print resolution is >= the CSS resolution, which seems like a
reasonable assumption.

If you try to print with a lower resolution than CSS, then instead of an
stretched canvas, you'd get a centered CSS-quality canvas, which seems
sensible. This could maybe be fixed with some CSS hackery (some combination of
min / max and viewport units perhaps?), but I think it's more trouble than it's
worth.
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