Fix issue with space offsets in TJs #6019

kalley · 2015-05-15T05:47:25Z

We were having issues in multiple documents with spaces in a table-like text block that used TJs when selecting text.

master:

this branch:

because of the math involved in calculating the offsets in spaced text (TJ), you can't really separate out that logic into two separate operations.

Reference equation from the spec is on page 252.

I wasn't sure if just adding the pdf to the test_manifest was enough, so let me know if you need more than that.

Snuffleupagus · 2015-05-15T10:44:22Z

src/core/evaluator.js

        var font = textState.font;
+        var offset = (typeof previousChars === 'string' ? 0 :
+                      previousChars / 1000) || 0;


I think that you can actually simplify this expression by instead doing e.g.

var offset = (isNum(previousChars) ? previousChars / 1000 : 0);

timvandermeij · 2015-05-15T11:04:55Z

#5813 is also (partially) touching this code, however, in a code block that you removed here. Will this PR also fix the issue mentioned there?

Snuffleupagus · 2015-05-15T11:14:03Z

It really great that you've included a reduced test-case!
However the PDF file is unfortunately slightly malformed, since Adobe Reader won't render anything and PDF.js complains about it as well.

Since I know from first hand experiences that it's sometimes difficult to create working test-cases, I took the liberty of providing a cleaned-up version of the PDF file: https://www.dropbox.com/s/efo05mrfo1fa2k4/pr6019.pdf?dl=0. (If you use this file instead, please don't forget to update the md5 entry in the manifest.)

Snuffleupagus · 2015-05-15T11:29:51Z

src/core/evaluator.js

-                        textChunk.str.push(' ');
-                      }
-                    } else if (fakeSpaces > SPACE_FACTOR) {
+                  buildTextGeometry(items[j], textChunk, items[j - 1]);


When j === 0, this will result in j - 1 being out of range of the array. As we've seen before, e.g. in #5663 (comment), that can slow things down in some browsers. Hence I'm wondering if it makes sense to try and avoid that, e.g. do this instead:

buildTextGeometry(items[j], textChunk, (j > 0 ? items[j - 1] : 0));

kalley · 2015-05-15T12:45:02Z

@timvandermeij in regards to #5813, it doesn't completely fix his issue, but they do work in concert when you just take the changes from within buildTextGeometry.

@Snuffleupagus what did you use to make your pdf? I only have a grasp of the obj streams well enough to screw with them.

timvandermeij · 2015-05-15T19:05:05Z

/botio test

pdfjsbot · 2015-05-15T19:05:05Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/2719f1e31e43d89/output.txt

pdfjsbot · 2015-05-15T19:05:05Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2015-05-15T19:23:00Z

From: Bot.io (Windows)

Failed

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

Total script time: 17.91 mins

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

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

pdfjsbot · 2015-05-15T19:23:54Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/2719f1e31e43d89/output.txt

Total script time: 18.81 mins

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

Image differences available at: http://107.21.233.14:8877/2719f1e31e43d89/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2015-05-15T19:54:10Z

The test results seem both good and (perhaps) less good. In e.g. taro-text there are improvements in the horizontal text, but the vertical text (on pages 2 and 4) seem to regress a bit.

kalley · 2015-05-15T20:01:45Z

In the vertical, if I add the offset instead of subtract it, it looks fine. But that doesn't seem to match up with the spec. I'll keep digging.

kalley · 2015-05-15T20:43:01Z

I don't understand the vmetric property on the glyph, which in about every case in the taro-text document seems to be glyph.width * -1. I'm wondering if that could be part of it. I just need some more context around it. It could be as easy and flipping the offset if vmetric exists.

timvandermeij · 2015-10-13T18:17:38Z

Let's get this PR rolling again. @brendandahl Do you have any idea on how the vmetric property works as asked above?

brendandahl · 2015-10-14T21:49:09Z

Vmetrics come from DW2 array in the font dictionary. There's alot more info in 9.7.4.3 Glyph Metrics in CIDFonts, but the number you're seeing is negative is because in the PDF coordinate system the origin starts at the bottom left, so a negative moves down.

Snuffleupagus · 2015-10-15T10:32:07Z

Oh, it's actually possible that some of the movement, in the vertical text, seen in the earlier testing of this patch exposed an issue which has since been fixed by PR #6391.

/botio test

pdfjsbot · 2015-10-15T10:32:09Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2015-10-15T10:32:09Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/35637fd01871341/output.txt

Snuffleupagus · 2015-10-15T10:37:36Z

src/core/evaluator.js

@@ -1130,7 +1133,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
              var offset;
              for (var j = 0, jj = items.length; j < jj; j++) {
                if (typeof items[j] === 'string') {
-                  buildTextGeometry(items[j], textChunk);
+                  buildTextGeometry(items[j], textChunk,
+                                   (j > 0 ? items[j - 1] : 0));


How about passing in null instead when j === 0, i.e changing it to
(j > 0 ? items[j - 1] : null)? That way, you'll avoid a pointless division at line 958.

Also, can you please add one more space to the indentation of this line.

pdfjsbot · 2015-10-15T10:50:33Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/35637fd01871341/output.txt

Total script time: 18.40 mins

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

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

pdfjsbot · 2015-10-15T10:52:09Z

From: Bot.io (Linux)

Failed

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

Total script time: 20.01 mins

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

Image differences available at: http://107.21.233.14:8877/a366e510b2f5a6d/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2015-10-15T13:50:30Z

/botio-linux preview

pdfjsbot · 2015-10-15T13:50:32Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2015-10-15T13:51:13Z

From: Bot.io (Linux)

Success

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

Total script time: 0.68 mins

Published

kalley · 2015-10-15T18:23:59Z

Let me know if you're ready for me to squash this. Or if there are any additional comments.

yurydelendik · 2015-10-15T18:57:27Z

src/core/evaluator.js

@@ -953,8 +953,9 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
          });
      }

-      function buildTextGeometry(chars, textChunk) {
+      function buildTextGeometry(chars, textChunk, previousChars) {


Is there a way to handle previousChar in the if below? It does not look logical handle that in two places

I could handle it below, but I felt like figuring out the offset made sense here, where as below it's just passing in the previous character. It doesn't have a concept of an offset. Though that is debatable, looking at the else down there. I'll push another commit that changes this. There's still going to end up being a check up here to make a decision on 0 or not.

SamyCookie · 2015-10-16T09:11:40Z

@kalley In the generated preview above and with the tracemonkey demo paper, text selection on the first paragrapher (and others) is larger than before and seems buggy. Is it normal ?

kalley · 2015-10-16T09:18:51Z

@SamyCookie nope, that's not normal. I may need to go back to the drawing board a little after #6391. I think originally I had completely removed any offset logic from that if block, but when I rebased, I thought that needed to be there. I'll dig in tomorrow.

kalley · 2015-10-16T14:29:02Z

Looks like what @SamyCookie found is fixed now, and my initial visual test of the taro-text looks correct as well.

Though, there now seems to be a problem with issue6387.pdf.

kalley · 2015-10-16T15:09:40Z

Strangely, changing the - to a + here: https://github.com/mozilla/pdf.js/pull/6019/files#diff-0b94c2e77a5259f7a728122fdbf9f46aR1045 fixes the issues with issue6387.pdf, but breaks page 2 and 4 of taro-text. I'll keep digging around, but if someone has any insight, that would be awesome.

yurydelendik · 2015-11-03T15:40:39Z

The broken PDF was fixed by #6588. Thanks for looking into the issue.

timvandermeij added the text-selection label May 15, 2015

Snuffleupagus reviewed May 15, 2015
View reviewed changes

kalley force-pushed the fix-whitespace-offset branch from 4e416de to 1f8a26b Compare May 15, 2015 12:49

Fix issue with space offsets in TJs

726beb4

kalley force-pushed the fix-whitespace-offset branch from 1f8a26b to 726beb4 Compare October 14, 2015 22:28

Snuffleupagus reviewed Oct 15, 2015
View reviewed changes

changed 0 to null, fixed formatting

a697c44

yurydelendik reviewed Oct 15, 2015
View reviewed changes

move offset logic into switch statement

d819506

Offset is used in buildTextGeometry only

85fa33c

kalley force-pushed the fix-whitespace-offset branch from 2426ba7 to 85fa33c Compare October 16, 2015 14:27

timvandermeij assigned brendandahl Oct 18, 2015

yurydelendik mentioned this pull request Nov 2, 2015

Better spacing in text layer. #6588

Merged

yurydelendik added a commit to yurydelendik/pdf.js that referenced this pull request Nov 2, 2015

Test case from mozilla#6019.

63627c6

yurydelendik added a commit to yurydelendik/pdf.js that referenced this pull request Nov 3, 2015

Test case from mozilla#6019.

da4de84

yurydelendik added a commit to yurydelendik/pdf.js that referenced this pull request Nov 3, 2015

Test case from mozilla#6019.

27efe56

yurydelendik closed this Nov 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with space offsets in TJs #6019

Fix issue with space offsets in TJs #6019

kalley commented May 15, 2015

Snuffleupagus May 15, 2015

timvandermeij commented May 15, 2015

Snuffleupagus commented May 15, 2015

Snuffleupagus May 15, 2015

kalley commented May 15, 2015

timvandermeij commented May 15, 2015

pdfjsbot commented May 15, 2015

pdfjsbot commented May 15, 2015

pdfjsbot commented May 15, 2015

pdfjsbot commented May 15, 2015

Snuffleupagus commented May 15, 2015

kalley commented May 15, 2015

kalley commented May 15, 2015

timvandermeij commented Oct 13, 2015

brendandahl commented Oct 14, 2015

Snuffleupagus commented Oct 15, 2015

pdfjsbot commented Oct 15, 2015

pdfjsbot commented Oct 15, 2015

Snuffleupagus Oct 15, 2015

pdfjsbot commented Oct 15, 2015

pdfjsbot commented Oct 15, 2015

Snuffleupagus commented Oct 15, 2015

pdfjsbot commented Oct 15, 2015

pdfjsbot commented Oct 15, 2015

kalley commented Oct 15, 2015

yurydelendik Oct 15, 2015

kalley Oct 16, 2015

SamyCookie commented Oct 16, 2015

kalley commented Oct 16, 2015

kalley commented Oct 16, 2015

kalley commented Oct 16, 2015

yurydelendik commented Nov 3, 2015

Fix issue with space offsets in TJs #6019

Fix issue with space offsets in TJs #6019

Conversation

kalley commented May 15, 2015

Snuffleupagus May 15, 2015

Choose a reason for hiding this comment

timvandermeij commented May 15, 2015

Snuffleupagus commented May 15, 2015

Snuffleupagus May 15, 2015

Choose a reason for hiding this comment

kalley commented May 15, 2015

timvandermeij commented May 15, 2015

pdfjsbot commented May 15, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented May 15, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented May 15, 2015

From: Bot.io (Windows)

Failed

pdfjsbot commented May 15, 2015

From: Bot.io (Linux)

Failed

Snuffleupagus commented May 15, 2015

kalley commented May 15, 2015

kalley commented May 15, 2015

timvandermeij commented Oct 13, 2015

brendandahl commented Oct 14, 2015

Snuffleupagus commented Oct 15, 2015

pdfjsbot commented Oct 15, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 15, 2015

From: Bot.io (Windows)

Received

Snuffleupagus Oct 15, 2015

Choose a reason for hiding this comment

pdfjsbot commented Oct 15, 2015

From: Bot.io (Windows)

Failed

pdfjsbot commented Oct 15, 2015

From: Bot.io (Linux)

Failed

Snuffleupagus commented Oct 15, 2015

pdfjsbot commented Oct 15, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 15, 2015

From: Bot.io (Linux)

Success

Published

kalley commented Oct 15, 2015

yurydelendik Oct 15, 2015

Choose a reason for hiding this comment

kalley Oct 16, 2015

Choose a reason for hiding this comment

SamyCookie commented Oct 16, 2015

kalley commented Oct 16, 2015

kalley commented Oct 16, 2015

kalley commented Oct 16, 2015

yurydelendik commented Nov 3, 2015