-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Fix pattern-filled text (embedded fonts only) #9090
Conversation
Thank you for looking into this! I'm just adding that at #3988 a new link to the PDF file has been put in the comments. |
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/fd85f4d3c7a93a6/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/fd85f4d3c7a93a6/output.txt Total script time: 2.45 mins Published |
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/59d8d498cf861cc/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/c90791774c74c38/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/59d8d498cf861cc/output.txt Total script time: 17.27 mins
Image differences available at: http://54.67.70.0:8877/59d8d498cf861cc/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.
Sorry for the review delay here!
This looks like a great improvement over the current state; thank you for the patch!
I've left a couple of comments, basically nits, but other than that the general approach seem fine to me :-)
src/core/evaluator.js
Outdated
@@ -584,7 +584,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() { | |||
var glyphs = font.charsToGlyphs(chars); | |||
var isAddToPathSet = !!(state.textRenderingMode & | |||
TextRenderingMode.ADD_TO_PATH_FLAG); | |||
if (font.data && (isAddToPathSet || this.options.disableFontFace)) { | |||
if (font.data && (isAddToPathSet || this.options.disableFontFace || | |||
state.fillColorSpace.name === 'Pattern')) { |
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: To make it easier to interpret this condition, can you please re-align it as follows instead?
if (font.data && (isAddToPathSet || this.options.disableFontFace ||
state.fillColorSpace.name === 'Pattern')) {
src/display/canvas.js
Outdated
@@ -1347,7 +1347,8 @@ var CanvasGraphics = (function CanvasGraphicsClosure() { | |||
this.moveText(0, this.current.leading); | |||
}, | |||
|
|||
paintChar: function CanvasGraphics_paintChar(character, x, y) { | |||
paintChar: function CanvasGraphics_paintChar(character, x, y, |
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: Taking full advantage of ES6 syntax, the entire method signature can be simplified to just:
paintChar(character, x, y, patternTransform) {
src/display/canvas.js
Outdated
if (accent) { | ||
scaledAccentX = scaledX + accent.offset.x / fontSizeScale; | ||
scaledAccentY = scaledY - accent.offset.y / fontSizeScale; | ||
this.paintChar(accent.fontChar, scaledAccentX, scaledAccentY); | ||
this.paintChar(accent.fontChar, scaledAccentX, scaledAccentY, | ||
patternTransform); |
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: Let's align this with the arguments above instead.
this.paintChar(accent.fontChar, scaledAccentX, scaledAccentY,
patternTransform);
src/display/canvas.js
Outdated
// TODO: Some shading patterns are not applied correctly to text, | ||
// e.g. issues 3988 and 5432, and ShowText-ShadingPattern.pdf. | ||
ctx.fillStyle = current.fillColor.getPattern(ctx, this); | ||
// TODO: Patterns are not applied correctly to text if a system font |
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: Let's replace system font
with non-embedded font
instead here, since that's more inline with the notation used elsewhere in PDF.js.
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/c90791774c74c38/output.txt Total script time: 24.53 mins
Image differences available at: http://54.215.176.217:8877/c90791774c74c38/reftest-analyzer.html#web=eq.log |
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/92c21060baa45b8/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/fc697e83aaed119/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/92c21060baa45b8/output.txt Total script time: 17.34 mins
Image differences available at: http://54.67.70.0:8877/92c21060baa45b8/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/fc697e83aaed119/output.txt Total script time: 23.87 mins
Image differences available at: http://54.215.176.217:8877/fc697e83aaed119/reftest-analyzer.html#web=eq.log |
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/52f8bd48d88aef9/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/4898f4b4b973534/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/52f8bd48d88aef9/output.txt Total script time: 16.14 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/4898f4b4b973534/output.txt Total script time: 21.73 mins
|
Is it possible to do the same for pattern-stroked text? |
Fix pattern-filled text (embedded fonts only)
Fixes #8940
Fixes #5432 (AkzoNobel file)
Fixes #3988 (texts now have gradient fill instead of solid color)
This PR fixes the rendering of texts that have a tiling or shading pattern as a fill color and an embedded font is used.
The problems in the reported issues are caused by
ctx.fillText()
. The glyphs and the pattern would need their own transforms butctx.fillText()
uses the same transform for them both. Thus either the text or the pattern gets incorrectly transformed. My solution is to draw glyphs as paths, so I can use a different transform for the pattern and the glyphs. However I can't fix texts that use system fonts because it's impossible to get glyphs as paths from system fonts.The file test/pdfs/ShowText-ShadingPattern.pdf (test case
ShowText-ShadingPattern
) is rendered better now. The "Arial" text is correct because it uses an embedded font. The "Helvetica" text is still wrong since it uses a system font.This PR continues the work that was started in PR #6820.