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

Only draw glyphs on canvas if they are in the font or the font file is missing. #7023

Merged
merged 1 commit into from
Mar 2, 2016

Conversation

brendandahl
Copy link
Contributor

Fixes #6721

I could have sworn there were more PDF's with this issue, but I'm not finding any.

@brendandahl
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/96bde2b0a1f2ca5/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/7ff619c0548d387/output.txt

@@ -2836,7 +2840,7 @@ var Font = (function FontClosure() {
!glyph.matchesForCache(fontChar, unicode, accent, width, vmetric,
operatorListId, isSpace)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you need to add isInFont here as well (and also update the comparison in Glyph.prototype.matchesForCache)?

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/7ff619c0548d387/output.txt

Total script time: 22.13 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://107.22.172.223:8877/96bde2b0a1f2ca5/output.txt

Total script time: 22.63 mins

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

Image differences available at: http://107.22.172.223:8877/96bde2b0a1f2ca5/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator

This also fixes #5642.
It somewhat "regresses" #4402, but given that it has never worked properly it might not matter too much.

@Snuffleupagus
Copy link
Collaborator

r=me, with the comment addressed and passing test runs.

@brendandahl
Copy link
Contributor Author

Good catch
/botio test

@pdfjsbot
Copy link

pdfjsbot commented Mar 1, 2016

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/61396c6a8aa0ef6/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Mar 1, 2016

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/96df55a6f8da6af/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Mar 1, 2016

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/96df55a6f8da6af/output.txt

Total script time: 20.16 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 1, 2016

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/61396c6a8aa0ef6/output.txt

Total script time: 21.81 mins

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

@brendandahl
Copy link
Contributor Author

It somewhat "regresses" #4402,

I have a fix for this I think, will do in another PR though.

@brendandahl
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Mar 2, 2016

From: Bot.io (Windows)


Received

Command cmd_makeref from @brendandahl received. Current queue size: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 2, 2016

From: Bot.io (Linux)


Received

Command cmd_makeref from @brendandahl received. Current queue size: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 2, 2016

From: Bot.io (Windows)


Success

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

Total script time: 20.06 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 2, 2016

From: Bot.io (Linux)


Success

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

Total script time: 21.29 mins

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

brendandahl added a commit that referenced this pull request Mar 2, 2016
Only draw glyphs on canvas if they are in the font or the font file is missing.
@brendandahl brendandahl merged commit a6acf74 into mozilla:master Mar 2, 2016
@Snuffleupagus Snuffleupagus mentioned this pull request Mar 2, 2016
Rob--W added a commit to Rob--W/pdf.js that referenced this pull request Aug 16, 2017
This bug is similar to the canvas bug of mozilla#6721.
I found this bug when I tried to run pdf2svg on a SVG file, and the generated
SVG could not be viewed in Chrome due to a SVG/XML parsing error:
"PCDATA invalid Char value 3"

Reduced test case:
- https://github.com/mozilla/pdf.js/files/1229507/pcdatainvalidchar.pdf
- expected: "hardware performance"
- Actual SVG source: "hardware\x03performance"
  (where "\x03" is a non-printable character, and invalid XML).

In terms of rendering, this bug is similar to mozilla#6721, where an unexpected glyph
appeared in the canvas renderer. This was fixed by mozilla#7023, which skips over
missing glyphs. This commit follows a similar logic.

The test case from mozilla#6721 can be used here too:
- https://github.com/mozilla/pdf.js/files/52205/issue6721_reduced.pdf
  expected: "Issue   6721"
  actual (before this patch): "Issue ààà6721"
Rob--W added a commit to Rob--W/pdf.js that referenced this pull request Aug 16, 2017
This bug is similar to the canvas bug of mozilla#6721.
I found this bug when I tried to run pdf2svg on a SVG file, and the generated
SVG could not be viewed in Chrome due to a SVG/XML parsing error:
"PCDATA invalid Char value 3"

Reduced test case:
- https://github.com/mozilla/pdf.js/files/1229507/pcdatainvalidchar.pdf
- expected: "hardware performance"
- Actual SVG source: "hardware\x03performance"
  (where "\x03" is a non-printable character, and invalid XML).

In terms of rendering, this bug is similar to mozilla#6721, where an unexpected glyph
appeared in the canvas renderer. This was fixed by mozilla#7023, which skips over
missing glyphs. This commit follows a similar logic.

The test case from mozilla#6721 can be used here too:
- https://github.com/mozilla/pdf.js/files/52205/issue6721_reduced.pdf
  expected: "Issue   6721"
  actual (before this patch): "Issue ààà6721"
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
This bug is similar to the canvas bug of mozilla#6721.
I found this bug when I tried to run pdf2svg on a SVG file, and the generated
SVG could not be viewed in Chrome due to a SVG/XML parsing error:
"PCDATA invalid Char value 3"

Reduced test case:
- https://github.com/mozilla/pdf.js/files/1229507/pcdatainvalidchar.pdf
- expected: "hardware performance"
- Actual SVG source: "hardware\x03performance"
  (where "\x03" is a non-printable character, and invalid XML).

In terms of rendering, this bug is similar to mozilla#6721, where an unexpected glyph
appeared in the canvas renderer. This was fixed by mozilla#7023, which skips over
missing glyphs. This commit follows a similar logic.

The test case from mozilla#6721 can be used here too:
- https://github.com/mozilla/pdf.js/files/52205/issue6721_reduced.pdf
  expected: "Issue   6721"
  actual (before this patch): "Issue ààà6721"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants