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

Text widget annotations: implement maximum length and text alignment #7622

Merged
merged 1 commit into from
Sep 11, 2016

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Sep 11, 2016

Moreover, we refactor the code a bit to extract code that is shared between the two branches and we only apply text alignment (and create the array) when it is actually defined, since it's optional and left is already the default.

@Snuffleupagus Could you review this PR? This patch is a part of #7613 and improves the support for text widget annotations. You can test this by opening the f1040.pdf file and noticing that you can now only enter two characters into the top right input box (with 20 before it) and that most fields in the "Income" section and now right-aligned.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Sep 11, 2016

This seems fine, thank you!
It would be great to have tests for this feature, but I realize that it might not be too easy to add tests at this stage and I'm not sure if a unit-test would bring much (in any) value here!?
(Once we've got more complete support for forms, it might make sense to add a modified f1040.pdf file with the various form elements already filled in (e.g. via fieldValue) in the PDF file itself, since I'm assuming that this would give us the opportunity to check that e.g. things like justification/text colour/text style/etc. works as intended.)

r=me, with passing tests and with the CONSTANTcomment addressed.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/display/annotation_layer.js, line 461 at r1 (raw file):

        var font = null;
        if (this.data.fontRefName) {

Just something I noticed in passing:
Since this is existing code you don't need to worry about it in this PR, but it doesn't appear that there's currently any code in core/annotation.js that actually sets this fontRefName property.

This could perhaps be something worth to look into in a future PR, since there's also a TODO comment in the _setTextStyle method, and it doesn't seem that we currently even try to load/use the correct font information here in display/annotation_layer.js.


src/display/annotation_layer.js, line 468 at r1 (raw file):

      if (this.data.textAlignment) {
        var alignments = ['left', 'center', 'right'];

Would it make sense to make this a constant instead, e.g. TEXT_ALIGNMENT or similar, and define it outside of the render method?


Comments from Reviewable

Moreover, we refactor the code a bit to extract code that is shared
between the two branches and we only apply text alignment (and create
the array) when it is actually defined, since it's optional and left is
already the default.
@timvandermeij
Copy link
Contributor Author

timvandermeij commented Sep 11, 2016

The comment has been addressed and I followed the specification better by making sure that the values we use for maximum length and text alignment are integers (as other values would not work). This is trivial and there are probably no such broken PDFs, but let's be safe nevertheless.

In the meta issue, I added a note about the (probably dead) font code and about sanitizing the values for maximum length/text alignment in the core layer (so that this display layer code is only getting either null, which doesn't pass isInt, or a valid integer), and adding unit tests for that.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

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

Total script time: 1.27 mins

Published

@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/8b2a55d4d07816b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/8b2a55d4d07816b/output.txt

Total script time: 24.23 mins

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

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

Total script time: 39.34 mins

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

@timvandermeij timvandermeij merged commit 6b05cfd into mozilla:master Sep 11, 2016
@timvandermeij timvandermeij deleted the interactive-forms-text branch September 11, 2016 19:42
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.

3 participants