-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Set the text fields font size based on their height #14936
Conversation
src/display/annotation_layer.js
Outdated
// We don't use in the input the font which is in the pdf, so | ||
// using the fontSize can lead to bad display. | ||
// Hence, we set the fontSize in based on the field height. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's improve the grammar a little bit here, e.g. like so:
// We don't use the font, as specified in the PDF document, for the <input>
// element. Hence using the original `fontSize` could look bad, which is why
// it's instead based on the field height.
src/shared/util.js
Outdated
@@ -18,6 +18,11 @@ import "./compatibility.js"; | |||
const IDENTITY_MATRIX = [1, 0, 0, 1, 0, 0]; | |||
const FONT_IDENTITY_MATRIX = [0.001, 0, 0, 0.001, 0, 0]; | |||
|
|||
// Represent the percentage of the height of a single-line field over | |||
// the font size. | |||
// Acrobat seems to use this value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can be moved to the previous line.
@@ -59,10 +59,9 @@ | |||
background-image: var(--annotation-unfocused-field-background); | |||
border: 1px solid transparent; | |||
box-sizing: border-box; | |||
font-size: 9px; | |||
font: 9px sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this essentially "sneaking" in a fix for issue #14736 here?
If so, that's probably good to mention explicitly in the commit message (and linking the issue such that it's closed).
height: 100%; | ||
margin: 0; | ||
padding: 0 3px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this essentially "sneaking" in a fix for issue #14301 here?
If so, that's probably good to mention explicitly in the commit message (and linking the issue such that it's closed).
Given the discussion in the issue, does this generally work without problems?
Is this properly covered by existing tests, or does new reference test(s) need to be added?
/botio test |
7f68aed
to
d32dbb9
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/5b8a9612200a823/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/17a3ca8a91f3fef/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/5b8a9612200a823/output.txt Total script time: 27.11 mins
Image differences available at: http://54.241.84.105:8877/5b8a9612200a823/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/17a3ca8a91f3fef/output.txt Total script time: 29.61 mins
Image differences available at: http://54.193.163.58:8877/17a3ca8a91f3fef/reftest-analyzer.html#web=eq.log |
c5a64db
to
52aa598
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/45c80418c2cccaa/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/8157e09d3166735/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/45c80418c2cccaa/output.txt Total script time: 19.48 mins
Image differences available at: http://54.241.84.105:8877/45c80418c2cccaa/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/8157e09d3166735/output.txt Total script time: 27.73 mins
Image differences available at: http://54.193.163.58:8877/8157e09d3166735/reftest-analyzer.html#web=eq.log |
|
52aa598
to
12dd4ac
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.241.84.105:8877/fb19329172665ce/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.193.163.58:8877/1f1126e3bb423ec/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/fb19329172665ce/output.txt Total script time: 25.93 mins
Image differences available at: http://54.241.84.105:8877/fb19329172665ce/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/1f1126e3bb423ec/output.txt Total script time: 28.15 mins
Image differences available at: http://54.193.163.58:8877/1f1126e3bb423ec/reftest-analyzer.html#web=eq.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm overlooking something, I don't believe that the following override is necessary now:
pdf.js/web/annotation_layer_builder.css
Line 170 in d80035b
padding: 0; |
r=me, with the above fixed since this looks overall reasonable to me; thank you!
- right now we're using the font size from the pdf itself but we use an other font in the annotation layer. So this size doesn't really make sense and leads to bad rendering (see pdf in mozilla#14928); - use a sans-serif font for the fields containing text (fix issue mozilla#14736); - remove useless padding in text-based fields (fix issue mozilla#14301); - text fields allow/disallow scrolling bars (see bit 24 in Ff entry), so use this value to hide/show scrollbars in annotation layer.
12dd4ac
to
9d82106
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/30ebe9859ac825f/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/b5e56710e3a4703/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/30ebe9859ac825f/output.txt Total script time: 26.10 mins
Image differences available at: http://54.241.84.105:8877/30ebe9859ac825f/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/b5e56710e3a4703/output.txt Total script time: 28.04 mins
Image differences available at: http://54.193.163.58:8877/b5e56710e3a4703/reftest-analyzer.html#web=eq.log |
/botio makeref |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @calixteman received. Current queue size: 1 Live output at: http://54.193.163.58:8877/3c9f771ca4dcdaa/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/39b0d65d21af1af/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/39b0d65d21af1af/output.txt Total script time: 22.75 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/3c9f771ca4dcdaa/output.txt Total script time: 21.78 mins
|
in the annotation layer. So this size doesn't really make sense and leads to bad
rendering (see pdf in [annotation] Text widget background color is not taken into account when printed #14928);
value to hide/show scrollbars in annotation layer.