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

Implement text rise for the SVG back-end #8796

Merged
merged 1 commit into from
Aug 26, 2017

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Aug 19, 2017

The property and the setter for text rise were already present, but they were never used or called. This patch completes the implementation by calling the setter when the operator is encountered and by using the text rise value when rendering text.

To verify the current behavior:

  1. Open https://mozilla.github.io/pdf.js/web/viewer.html
  2. Run PDFViewerApplication.preferences.set('renderer', 'svg'); in the console and refresh.
  3. Download https://www.pdflib.com/fileadmin/pdflib/Cookbook/pdf/footnotes_in_text.pdf and open it using the Open File button in the viewer. Notice that the 1 and 2 in the footnotes are not in superscript and that the console contains a message about the unimplemented operator setTextRise.

To verify the new behavior:

  1. Open the preview viewer.
  2. Run PDFViewerApplication.preferences.set('renderer', 'svg'); in the console and refresh.
  3. Open the footnotes_in_text.pdf fule using the Open File button in the viewer. Notice that the 1 and 2 in the footnotes are in superscript and that the console message is gone.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@timvandermeij
Copy link
Contributor Author

/botio test

@timvandermeij
Copy link
Contributor Author

@Rob--W Since you're also working on the SVG-backend, would you perhaps have time to review this?

/botio-windows test

// original text matrix.
let matrix = current.textMatrix.slice();
if (current.textRise !== 0) {
matrix[5] += current.textRise;
Copy link
Member

Choose a reason for hiding this comment

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

How often is this called? If setTextRise is called quite often, then you could relieve the GC by only making a copy when it is needed:

let matrix = current.textMatrix;
if (current.textRise) {
    matrix = matrix.slice();
    matrix[5] += current.textRise;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Fixed in the new commit.

The property and the setter for text rise were already present, but they
were never used or called. This patch completes the implementation by
calling the setter when the operator is encountered and by using the
text rise value when rendering text.
@Rob--W
Copy link
Member

Rob--W commented Aug 22, 2017

Why is the unit test in Chrome failing on Windows?

TEST-UNEXPECTED-FAIL | gets destinations, from /Names (NameTree) dictionary | Error: close should have stream controller thrown 

@timvandermeij
Copy link
Contributor Author

This is a known intermittent after the stream API changes landed, but somehow I forgot to file it before. I just did that in #8816. It's not related to this patch.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@mozilla mozilla deleted a comment from pdfjsbot Aug 26, 2017
@timvandermeij
Copy link
Contributor Author

The intermittent unit test failure is fixed, so it should be fine now.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/9d7e28f30c4e4ba/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/9d7e28f30c4e4ba/output.txt

Total script time: 2.37 mins

Published

@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/0ae1cf678c21930/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/a4f2d999e512641/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/0ae1cf678c21930/output.txt

Total script time: 16.45 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/a4f2d999e512641/output.txt

Total script time: 29.58 mins

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

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

@Rob--W Rob--W merged commit 7cc7260 into mozilla:master Aug 26, 2017
@Rob--W
Copy link
Member

Rob--W commented Aug 26, 2017

Thanks for the patch. I wish that we also had reftesting for the SVG backend.

@timvandermeij timvandermeij deleted the svg-text-rise branch August 26, 2017 20:06
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Implement text rise for the SVG back-end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants