Implement vertical writing for CJK text #2686

vyv03354 · 2013-02-06T16:28:51Z

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=770409 .

jviereck · 2013-02-07T09:11:33Z

/botio test

pdfjsbot · 2013-02-07T09:11:34Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/536e9ba863d7cc6/output.txt

pdfjsbot · 2013-02-07T09:11:34Z

From: Bot.io (Linux)

Received

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

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

jviereck · 2013-02-07T09:12:54Z

src/canvas.js

            switch (textRenderingMode) {
              default: // other unsupported rendering modes
              case TextRenderingMode.FILL:
              case TextRenderingMode.FILL_ADD_TO_PATH:
-                ctx.fillText(character, scaledX, 0);
+                if (vertical) {
+                  ctx.fillText(character, 0 + vx, scaled + vy);


What the reason for the extra 0 + here and the following lines?

jviereck · 2013-02-07T09:15:37Z

Thanks a lot for this patch!

The code for the rendering part looks good to me, but I don't know anything about fonts, so someone else has to do a review on that part.

jviereck · 2013-02-07T09:20:11Z

Copy over image from https://bugzilla.mozilla.org/show_bug.cgi?id=770409 as reference:

pdfjsbot · 2013-02-07T09:31:20Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/536e9ba863d7cc6/output.txt

Total script time: 19.77 mins

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

pdfjsbot · 2013-02-07T09:33:15Z

From: Bot.io (Linux)

Success

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

Total script time: 21.67 mins

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

jviereck · 2013-02-07T10:30:04Z

/botio-linux preview

pdfjsbot · 2013-02-07T10:30:06Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/1c3c0545e1a1422/output.txt

pdfjsbot · 2013-02-07T10:30:18Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/1c3c0545e1a1422/output.txt

Total script time: 0.20 mins

Published

Viewer: http://107.21.233.14:8877/1c3c0545e1a1422/web/viewer.html
Extension: http://107.21.233.14:8877/1c3c0545e1a1422/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/1c3c0545e1a1422/extensions/firefox/pdf.js.amo.xpi

jviereck · 2013-02-07T12:03:41Z

@vyv03354 the rendering looks good but the textLayer/selection is broken. The div is placed horizontal and not vertically. What's the correct behavior here? Should the div containing the letters have a new line-break after each letter or should the div be rotated by 90 degrees?

vyv03354 · 2013-02-07T13:36:42Z

Obviously "writing-mode: vertical-rl" would be the correct choice. But Firefox is unlikely to support writing-mode in near future, so I'll rotate the div at the moment. Individual rotated glyphs would not be a problem because textLayer texts are invisible anyway.

mduan · 2013-02-07T20:39:39Z

/botio test

pdfjsbot · 2013-02-07T20:39:40Z

From: Bot.io (Windows)

Received

Command cmd_test from @mduan received. Current queue size: 1

Live output at: http://107.22.172.223:8877/cd21954d23be9ba/output.txt

pdfjsbot · 2013-02-07T20:39:41Z

From: Bot.io (Linux)

Received

Command cmd_test from @mduan received. Current queue size: 1

Live output at: http://107.21.233.14:8877/3ad38673ed08ad7/output.txt

pdfjsbot · 2013-02-07T21:10:04Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/cd21954d23be9ba/output.txt

Total script time: 19.67 mins

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

vyv03354 · 2013-02-07T22:33:34Z

socket.error: [Errno 98] Address already in use

Infra error?

yurydelendik · 2013-02-07T23:04:55Z

Infra error?

I think so

/botio-linux test

pdfjsbot · 2013-02-07T23:04:57Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/359ea46350a5552/output.txt

pdfjsbot · 2013-02-07T23:26:31Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/359ea46350a5552/output.txt

Total script time: 21.55 mins

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

vyv03354 · 2013-02-08T20:21:29Z

What do I have to do to merge this? Just waiting for a review?

gigaherz · 2013-02-08T20:44:31Z

You should never merge anything yourself, even if you are allowed to. Let someone else review the code and decide. In this case, someone from the dev team (from Mozilla) may merge it if they like the PR after a review.

vyv03354 · 2013-02-08T20:55:01Z

I did not mean to say "merge it myself". I should have said "to be merged".

jviereck · 2013-02-09T16:43:26Z

/botio-linux preview

pdfjsbot · 2013-02-09T16:43:27Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/14401a07bd060a9/output.txt

pdfjsbot · 2013-02-09T16:43:37Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/14401a07bd060a9/output.txt

Total script time: 0.16 mins

Published

Viewer: http://107.21.233.14:8877/14401a07bd060a9/web/viewer.html
Extension: http://107.21.233.14:8877/14401a07bd060a9/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/14401a07bd060a9/extensions/firefox/pdf.js.amo.xpi

jviereck · 2013-02-09T16:44:12Z

What do I have to do to merge this? Just waiting for a review?

Yes --- and if it takes too long for you taste (> 1 week?) poke someone kindly :)

yurydelendik · 2013-02-09T17:19:26Z

(... as mentioned at https://github.com/mozilla/pdf.js/wiki/Contributing item 7)

jviereck · 2013-02-09T18:04:49Z

Obviously "writing-mode: vertical-rl" would be the correct choice. But Firefox is unlikely to support writing-mode in near future [...]

Do you know if there is a bug on file for this already? Would you mind to open one if it is not? Sometimes magic things happens once a bug is filed and things get implemented faster then expected.

brendandahl · 2013-02-20T19:49:59Z

/botio test

pdfjsbot · 2013-02-20T19:50:00Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/916604a032a632e/output.txt

pdfjsbot · 2013-02-20T19:50:01Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/30e234c279c3dff/output.txt

pdfjsbot · 2013-02-20T20:09:44Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/916604a032a632e/output.txt

Total script time: 19.74 mins

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

Image differences available at: http://107.22.172.223:8877/916604a032a632e/reftest-analyzer.xhtml#web=eq.log

pdfjsbot · 2013-02-20T20:11:42Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/30e234c279c3dff/output.txt

Total script time: 21.68 mins

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

vyv03354 · 2013-02-20T20:52:56Z

I was unable to reproduce the reftest failure locally.
Also I have no idea why this patch affects PDFs without vertical text. Very weird...

gigaherz · 2013-02-20T20:56:11Z

The failure may be an intermittent issue unrelated to your changes. There's still a few of those, I think.

brendandahl · 2013-02-20T21:05:45Z

The changes look like they're from the FF19 update. I'll open a pr to update them and re-run here.

vyv03354 · 2013-02-23T23:44:59Z

Could you execute the test again? The reference image should be updated by #2785.
Like #2785, failures are differences between missing glyphs and blanks.

yurydelendik · 2013-02-24T02:46:54Z

/botio test

pdfjsbot · 2013-02-24T02:46:55Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/01a58d3201b5515/output.txt

pdfjsbot · 2013-02-24T02:46:56Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2013-02-24T03:06:32Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/01a58d3201b5515/output.txt

Total script time: 19.61 mins

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

pdfjsbot · 2013-02-24T03:06:34Z

From: Bot.io (Linux)

Success

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

Total script time: 19.64 mins

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

brendandahl · 2013-02-25T18:03:03Z

/botio makeref

pdfjsbot · 2013-02-25T18:03:05Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2013-02-25T18:03:05Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/18d48e6b1a3c10d/output.txt

pdfjsbot · 2013-02-25T18:22:54Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/18d48e6b1a3c10d/output.txt

Total script time: 19.82 mins

Lint: Ignored
Make references: Passed
Check references: Passed

pdfjsbot · 2013-02-25T18:28:30Z

From: Bot.io (Linux)

Success

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

Total script time: 25.41 mins

Lint: Passed
Make references: Passed
Check references: Passed

Implement vertical writing

brendandahl · 2013-02-25T20:48:55Z

Thanks for implementing this.

mduan · 2013-03-11T23:28:25Z

For reference, linking 2 bugzilla issues related to this:

mitar · 2013-03-31T20:08:22Z

As explained in #2817, this has not fixed the issue of displaying the vertical text for latin fonts (the hidden layer text, rendering itself is OK). BidiResult does not get "vertical" flag set. Examples are arXiv PDFs: http://arxiv.org/pdf/0707.3023.pdf

mitar · 2013-03-31T20:13:13Z

Uh, even the PDF which was used for testing in #2686 does have a bug and does not display text layer correctly.

vyv03354 · 2013-03-31T23:07:42Z

#2817 is a different problem from this bug. Latin vertical text is rendered by a different method from CJK one even if they are mixed (like http://blogs.adobe.com/CCJKType/files/2012/07/TaroUTR50SortedList112.pdf ).

jviereck reviewed Feb 7, 2013
View reviewed changes

Implements vertical writing

c5b8ee6

brendandahl added a commit that referenced this pull request Feb 25, 2013

Merge pull request #2686 from vyv03354/bug770409

a13f796

Implement vertical writing

brendandahl merged commit a13f796 into mozilla:master Feb 25, 2013

mduan mentioned this pull request Feb 28, 2013

Problems with displaying vertical text #2817

Closed

vyv03354 mentioned this pull request Mar 12, 2013

Fix the calculation of default vertical metrics #2932

Merged

yurydelendik mentioned this pull request Jul 19, 2013

Fixing the offset of vertical CJK text #3494

Merged

Implement vertical writing for CJK text #2686

Implement vertical writing for CJK text #2686

Conversation

vyv03354 commented Feb 6, 2013

jviereck commented Feb 7, 2013

pdfjsbot commented Feb 7, 2013

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 7, 2013

From: Bot.io (Linux)

Received

jviereck Feb 7, 2013

Choose a reason for hiding this comment

jviereck commented Feb 7, 2013

jviereck commented Feb 7, 2013

pdfjsbot commented Feb 7, 2013

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 7, 2013

From: Bot.io (Linux)

Success

jviereck commented Feb 7, 2013

pdfjsbot commented Feb 7, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 7, 2013

From: Bot.io (Linux)

Success

Published

jviereck commented Feb 7, 2013

vyv03354 commented Feb 7, 2013

mduan commented Feb 7, 2013

pdfjsbot commented Feb 7, 2013

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 7, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 7, 2013

From: Bot.io (Windows)

Success

vyv03354 commented Feb 7, 2013

yurydelendik commented Feb 7, 2013

pdfjsbot commented Feb 7, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 7, 2013

From: Bot.io (Linux)

Success

vyv03354 commented Feb 8, 2013

gigaherz commented Feb 8, 2013

vyv03354 commented Feb 8, 2013

jviereck commented Feb 9, 2013

pdfjsbot commented Feb 9, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 9, 2013

From: Bot.io (Linux)

Success

Published

jviereck commented Feb 9, 2013

yurydelendik commented Feb 9, 2013

jviereck commented Feb 9, 2013

brendandahl commented Feb 20, 2013

pdfjsbot commented Feb 20, 2013

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 20, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 20, 2013

From: Bot.io (Windows)

Failed

pdfjsbot commented Feb 20, 2013

From: Bot.io (Linux)

Success

vyv03354 commented Feb 20, 2013

gigaherz commented Feb 20, 2013

brendandahl commented Feb 20, 2013

vyv03354 commented Feb 23, 2013