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

Apply Patterns, if necessary, when rendering text #6820

Merged
merged 1 commit into from Jan 8, 2016
Merged

Apply Patterns, if necessary, when rendering text #6820

merged 1 commit into from Jan 8, 2016

Conversation

Snuffleupagus
Copy link
Collaborator

Currently we're not applying Patterns for text, but only for graphics.

This patch is unfortunately not a complete solution, but rather a step on the way, since there are still some PDF files where the Patterns look more like a solid colour, rather than the intended gradient.
I've been unable to fix these issues completely, and I've not managed to determine if the remaining issues are caused either by the pattern code, the canvas code, or perhaps both.

However, given that even this simple patch improves the current situation quite a bit, I figured that it couldn't hurt to submit it as-is.

@timvandermeij
Copy link
Contributor

I suspect that the two files that are not completely fixed by this PR have a solid color because they use a horizontal gradient instead of a vertical gradient in the two fixed files. Therefore I think that the horizontal gradient is now drawn vertically (also because the text has the color of the leftmost part of the gradient). You might need to specify the direction of the gradient somehow, but that can be done in a follow-up patch; I justed wanted to mention my observation.

@@ -1421,6 +1421,11 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
ctx.transform.apply(ctx, current.textMatrix);
ctx.translate(current.x, current.y + current.textRise);

if (current.patternFill) {
// TODO: Some shading patterns are not applied correctly to text.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you list the issue numbers here too for reference? Otherwise I'm afraid it will be hard to find those later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, fixed now.

@Snuffleupagus
Copy link
Collaborator Author

suspect that the two files that are not completely fixed by this PR have a solid color because they use a horizontal gradient instead of a vertical gradient in the two fixed files.

I'd actually be surprised if it's that simple, since there appears to be more issues at play here.
Edit: But I wouldn't mind being proved wrong here :-)

Look at the PDF files in #6234 (comment); the first one is included as a test-case in this patch.
Note that in the first one the gradient isn't clipped correctly (the text is starting in the wrong position, and is also continuing off the page), and in the second one the gradient doesn't have the right "turquoise" colour at the end of the text (it's as if only part of the gradient is being used), when compared to Adobe Reader.

Currently we're not applying Patterns for text, but only for graphics.

This patch is unfortunately not a complete solution, but rather a step on the way, since there are still some PDF files where the Patterns look more like a solid colour, rather than the intended gradient.
I've been unable to fix these issues completely, and I've not managed to determine if the remaining issues are caused either by the pattern code, the canvas code, or perhaps both.

However, given that even this simple patch improves the current situation quite a bit, I figured that it couldn't hurt to submit it as-is.

 - Fixes 5804.
 - Fixes 6130.
 - Improves 3988 a lot, since the text is now visible. However, it looks like the text is *one* solid colour, instead of the correct gradient.
 - Improves 5432, since the text is no longer gray. (This file also suffers from the same problem as the previous one.)
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2016

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/f6931be12cacab9/output.txt

@yurydelendik
Copy link
Contributor

IRC log:

<timvandermeij> According to the logs it might be the one after ecma262-pdf, which 
is https://github.com/mozilla/pdf.js/blob/master/test/test_manifest.json#L379. This did 
not happen before, so I wonder if an update broke things (I see we're at Firefox 45 on 
the bots now).

Checking it is the same test

/botio-linux test

@brendandahl
Copy link
Contributor

I tried out issue6286.pdf through x11 and it seems to load fine. It does take awhile, but I think that's just x11 being slow.

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/b83386fa0b72163/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/59a60032097219c/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/59a60032097219c/output.txt

Total script time: 20.98 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/b83386fa0b72163/output.txt

Total script time: 21.61 mins

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

@timvandermeij
Copy link
Contributor

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/82479ca587299e9/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/4e8c41fffda4135/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/4e8c41fffda4135/output.txt

Total script time: 20.79 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2016

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/82479ca587299e9/output.txt

Total script time: 21.11 mins

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

timvandermeij added a commit that referenced this pull request Jan 8, 2016
Apply Patterns, if necessary, when rendering text
@timvandermeij timvandermeij merged commit 30b8f41 into mozilla:master Jan 8, 2016
@timvandermeij
Copy link
Contributor

Thank you!

@Snuffleupagus Snuffleupagus deleted the showText-shadingPattern branch January 8, 2016 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants