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

Incorrect spacing of vertial japanese text #11526

Closed
e-taka opened this issue Jan 21, 2020 · 7 comments
Closed

Incorrect spacing of vertial japanese text #11526

e-taka opened this issue Jan 21, 2020 · 7 comments

Comments

@e-taka
Copy link

e-taka commented Jan 21, 2020

Attach (recommended) or Link to PDF file here:
https://www.nta.go.jp/taxes/tetsuzuki/shinsei/shinkoku/zoyo/yoshiki2019/pdf/006.pdf

Configuration:

  • Web browser and its version: Firefox 72.0.1
  • Operating system and its version: macOS 10.14
  • PDF.js version: 2.2.228
  • Is a browser extension: No

Steps to reproduce the problem:

  1. Open this link https://www.nta.go.jp/taxes/tetsuzuki/shinsei/shinkoku/zoyo/yoshiki2019/pdf/006.pdf

What is the expected behavior? (add screenshot)
By Acrobat Reader, open above link:
スクリーンショット 2020-01-21 10 17 53

What went wrong? (add screenshot)
スクリーンショット 2020-01-21 10 18 12

@Snuffleupagus
Copy link
Collaborator

Possibly a duplicate of #7687.

@tamuratak
Copy link
Contributor

tamuratak commented Jan 27, 2020

The cause is that fontDirection in src/display/canvas.js is always 1 even when the writing mode is vertical. I have confirmed that the following patch using spacingDir instead of fontDirection solved #11526 and #7687. However, this patch is not an appropriate solution since it leaves fontDirection as it is. I am not sure what the purpose of fontDirection is. Is it for right-to-left texts?

diff --git a/src/display/canvas.js b/src/display/canvas.js
index eaec1a1d..60af1309 100644
--- a/src/display/canvas.js
+++ b/src/display/canvas.js
@@ -1719,7 +1719,7 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
           }
         }
 
-        var charWidth = width * widthAdvanceScale + spacing * fontDirection;
+        var charWidth = width * widthAdvanceScale - spacing * spacingDir;
         x += charWidth;
 
         if (restoreNeeded) {

I hope this would help your development.

@Snuffleupagus
Copy link
Collaborator

I have confirmed that the following patch using spacingDir instead of fontDirection solved #11526 and #7687.

Did it pass all existing PDF.js tests too?
See https://github.com/mozilla/pdf.js/wiki/Contributing and in particular https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing for additional information about running the test-suite.

@tamuratak
Copy link
Contributor

No, it didn't. Only eq issue1049 fails.

OS: macOS High Sierra 10.13.6
Firefox: 72.0.2

[
  {
    "name": "Firefox",
    "path": "/Applications/Firefox.app/Contents/MacOS/firefox",
    "headless": true
  }
]

Run npx gulp makeref at pdf.js/master. Run npx gulp test on my branch.

...
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 1 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 2 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 3 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 4 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 5 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 8 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 9 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 10 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 11 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 12 != reference rendering
TEST-UNEXPECTED-FAIL | eq issue1049 | in Firefox | rendering of page 13 != reference rendering
...
[Firefox] Going to wait until the browser process has exited.
[GFX1]: [Tiling:Client] Failed to allocate a TextureClient
[GFX1]: [Tiling:Client] Failed to allocate a TextureClient
[GFX1]: [Tiling:Client] Failed to allocate a TextureClient
[GFX1]: [Tiling:Client] Failed to allocate a TextureClient
[GFX1-]: Receive IPC close with reason=AbnormalShutdown
Exiting due to channel error.
[GFX1-]: Receive IPC close with reason=AbnormalShutdown
Exiting due to channel error.
Exiting due to channel error.
[GFX1-]: Receive IPC close with reason=AbnormalShutdown
[Firefox] Browser process exited in response to signal SIGTERM
Exiting due to channel error.
[Firefox] Clean-up finished. Going to call callback...

OHNOES!  Some tests failed!
  different ref/snapshot: 11
Runtime was 485.7 seconds

Starting reftest harness to examine 11 eq test failures.
Server running at http://127.0.0.1:55985/

@tamuratak
Copy link
Contributor

tamuratak commented Jan 28, 2020

The following patch passes all the tests.

diff --git a/src/display/canvas.js b/src/display/canvas.js
index eaec1a1d..a8213171 100644
--- a/src/display/canvas.js
+++ b/src/display/canvas.js
@@ -1719,7 +1719,7 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
           }
         }
 
-        var charWidth = width * widthAdvanceScale + spacing * fontDirection;
+        var charWidth = width * widthAdvanceScale - spacing * spacingDir * fontDirection;
         x += charWidth;
 
         if (restoreNeeded) {
[Firefox] Going to wait until the browser process has exited.
[GFX1-]: Receive IPC close with reason=AbnormalShutdown
Exiting due to channel error.
Exiting due to channel error.
[GFX1-]: Receive IPC close with reason=AbnormalShutdown
Exiting due to channel error.
Exiting due to channel error.
[Firefox] Browser process exited in response to signal SIGTERM
[Firefox] Clean-up finished. Going to call callback...

All regression tests passed.
Runtime was 508.0 seconds
[09:05:29] Finished '<anonymous>' after 9.38 min
[09:05:29] Finished 'test' after 9.85 min

tamuratak added a commit to tamuratak/pdf.js that referenced this issue Jan 28, 2020
Because the direction of the y-axis is reversed between
on the user space coordinate and the canvas coordinate,
we have to reverse the sign.
@janpe2
Copy link
Contributor

janpe2 commented Jan 28, 2020

I am not sure what the purpose of fontDirection is.

@tamuratak Its purpose is to support negative font sizes in operator Tf.

pdf.js/src/display/canvas.js

Lines 1427 to 1434 in 474fe17

// The spec for Tf (setFont) says that 'size' specifies the font 'scale',
// and in some docs this can be negative (inverted x-y axes).
if (size < 0) {
size = -size;
current.fontDirection = -1;
} else {
current.fontDirection = 1;
}

I think you have the correct approach in your pull request when you reverse the sign of spacing.

To summarize (like you have already debugged), the operator Tc (character spacing) is the culprit. It shifts characters in the wrong direction if the font is vertical.

tamuratak added a commit to tamuratak/pdf.js that referenced this issue Jan 28, 2020
When the writing mode is vertical, we have to reverse
the sign of spacing since we are subtracting it from
current.y. We have to add it to current.y.
See https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G7.1852045
tamuratak added a commit to tamuratak/pdf.js that referenced this issue Feb 11, 2020
When the writing mode is vertical, we have to reverse
the sign of spacing since we are subtracting it from
current.y. We have to add it to current.y.
See 9.4.4 Text Space Details, https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G8.1694762
timvandermeij added a commit that referenced this issue Feb 26, 2020
Fix text spacing with vertical fonts. #7687 and #11526.
@timvandermeij
Copy link
Contributor

Fixed by #11540.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants