Text widget annotations: implement comb support #7649

timvandermeij · 2016-09-19T22:11:05Z

This is another part of #7613.

Snuffleupagus · 2016-09-20T07:47:23Z

src/core/annotation.js

@@ -715,6 +715,8 @@ var TextWidgetAnnotation = (function TextWidgetAnnotationClosure() {
    // Process field flags for the display layer.
    this.data.readOnly = this.hasFieldFlag(AnnotationFieldFlag.READONLY);
    this.data.multiLine = this.hasFieldFlag(AnnotationFieldFlag.MULTILINE);
+    this.data.comb = this.hasFieldFlag(AnnotationFieldFlag.COMB) &&
+                     this.data.maxLen !== null && !this.data.multiLine;


I think that you're missing a number of field checks here, please see the specification text:

(PDF 1.5) May be set only if the MaxLen entry is present in the text field dictionary (see Table 229) and _if the Multiline, Password, and FileSelect flags are clear_. If set, the field shall be automatically divided into as many equally spaced positions, or combs, as the value of MaxLen, and the text is laid out into those combs.

Good point. I thought we didn't need it because we do not support the Password and FileSelect flags anyway, but it makes sense to add these checks anyway to prevent any issues. I'll update the patch.

Snuffleupagus · 2016-09-20T08:10:55Z

web/annotation_layer_builder.css

@@ -77,6 +77,22 @@
  border: 1px solid transparent;
 }

+.annotationLayer .textWidgetAnnotation input.comb {


Don't you want to add these rules to the corresponding file in /test as well, to enable testing of comb fields once we have better tests?

Yes, I'll do that.

timvandermeij · 2016-09-20T19:47:04Z

I have addressed the comments and managed to adjust the reference test for text widget annotations to include a field that uses combs.

timvandermeij · 2016-09-20T19:47:11Z

/botio-linux preview

pdfjsbot · 2016-09-20T19:47:12Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/434cf638d7a1547/output.txt

pdfjsbot · 2016-09-20T19:48:13Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/434cf638d7a1547/output.txt

Total script time: 1.02 mins

Published

timvandermeij · 2016-09-20T19:49:19Z

/botio test

pdfjsbot · 2016-09-20T19:49:21Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/7de759130fe7a06/output.txt

pdfjsbot · 2016-09-20T19:49:21Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2016-09-20T20:14:08Z

From: Bot.io (Windows)

Failed

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

Total script time: 24.78 mins

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

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

pdfjsbot · 2016-09-20T20:18:44Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/7de759130fe7a06/output.txt

Total script time: 29.39 mins

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

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

timvandermeij · 2016-09-20T20:35:31Z

I pushed a new commit to fix an issue in the a unit test. No other changes have been made.

/botio unittest

pdfjsbot · 2016-09-20T20:35:32Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-09-20T20:35:32Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/913c5d457a4bf41/output.txt

pdfjsbot · 2016-09-20T20:37:15Z

From: Bot.io (Linux)

Success

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

Total script time: 1.72 mins

Unit Tests: Passed

pdfjsbot · 2016-09-20T20:37:37Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/913c5d457a4bf41/output.txt

Total script time: 2.08 mins

Unit Tests: Passed

Snuffleupagus

Looks mostly fine, but I think that the format of AnnotationFieldFlag is making things more difficult to follow. Please see inline comments.

Snuffleupagus · 2016-09-21T08:01:29Z

test/unit/annotation_layer_spec.js

    });

    it('should set valid text alignment, maximum length and flags',
        function() {
+      var flags = 0;
+      flags |= 1 << (AnnotationFieldFlag.READONLY - 1);
+      flags |= 1 << (AnnotationFieldFlag.MULTILINE - 1);


As far as I'm concerted, the current way that the AnnotationFieldFlag are defined makes the unit-tests unnecessarily convoluted and difficult to understand.

What would be a lot clearer is if you changed AnnotationFieldFlag to the same form as AnnotationFlag, since that means you could change the hasFieldFlags method in src/core/annotation.js to just:

return !!(this.data.fieldFlags & flag);

and specifying the flags in the unit-tests would be so much more straightforward:

var flags = AnnotationFieldFlag.READONLY + AnnotationFieldFlag.MULTILINE;

Really good point. I'll definitely look into that.

I pushed a new commit that fixes this.

Snuffleupagus · 2016-09-21T08:05:21Z

web/annotation_layer_builder.css

+   * area, causing horizontal scrolling. We avoid this by extending the width
+   * when the element has focus and revert this when it loses focus.
+   */
+  width: 115%;


It's unfortunate that this sort of hack is necessary, but I'm also not able to come up with a better solution :-(

I agree, but after searching for a couple of hours there really isn't another way with CSS only. The real problem is that letter spacing is always applied on the right side of a character, whereas we would want to tune that or turn it off for the last character. Since that's not possible, we need this workaround. There are JS-based solutions, but they are even uglier and require much more code, so this is the cleanest solution IMO.

I just want to make it absolutely clear that I didn't mean to complain about your solution here, but rather about the (missing) features of the web platform that made the workaround necessary!

Snuffleupagus · 2016-09-21T08:10:41Z

test/unit/annotation_layer_spec.js

+      expect(textWidgetAnnotation.data.comb).toEqual(true);
+    });
+
+    it('should only accept comb fields when the flags are valid', function() {


It might be possible to simplify this particular test a bit, so I'd like to see an updated version based on the AnnotationFieldFlag comment above.

I'll try that. I'm not sure how much simpler it will become though as we still need the loops to remove the flags one-by-one, but we'll see.

I pushed a new commit that fixes this.

Directly use the hexadecimal representation, just like the `AnnotationFlags`, to avoid calculations and to improve readability. This allows us to simplify the unit tests for text widget annotations as well.

timvandermeij · 2016-09-21T19:18:47Z

/botio-linux preview

pdfjsbot · 2016-09-21T19:18:47Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/6ed923ea3044355/output.txt

pdfjsbot · 2016-09-21T19:19:47Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/6ed923ea3044355/output.txt

Total script time: 1.00 mins

Published

timvandermeij · 2016-09-21T19:20:07Z

/botio test

pdfjsbot · 2016-09-21T19:20:08Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/48fa394f3e9134e/output.txt

pdfjsbot · 2016-09-21T19:20:08Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-09-21T19:44:45Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/48fa394f3e9134e/output.txt

Total script time: 24.63 mins

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

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

pdfjsbot · 2016-09-21T19:49:13Z

From: Bot.io (Linux)

Failed

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

Total script time: 29.08 mins

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

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

Snuffleupagus · 2016-09-22T09:36:19Z

Much better, thank you! With the changes to AnnotationFieldFlags, the code and especially the unit-tests are now much simpler to deal with.

/botio makeref

pdfjsbot · 2016-09-22T09:36:20Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/6841f4c21ab4c77/output.txt

pdfjsbot · 2016-09-22T09:36:20Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/81cb41574c520d9/output.txt

pdfjsbot · 2016-09-22T10:01:13Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/6841f4c21ab4c77/output.txt

Total script time: 24.88 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2016-09-22T10:04:42Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/81cb41574c520d9/output.txt

Total script time: 28.37 mins

Lint: Passed
Make references: Passed
Check references: Passed

…tx-comb Text widget annotations: implement comb support

timvandermeij added annotations form-acroform labels Sep 19, 2016

timvandermeij force-pushed the interactive-forms-tx-comb branch from dc691f9 to 90a2107 Compare September 19, 2016 22:24

timvandermeij mentioned this pull request Sep 19, 2016

Interactive form (AcroForm) support #7613

Closed

23 tasks

Snuffleupagus reviewed Sep 20, 2016

View reviewed changes

timvandermeij force-pushed the interactive-forms-tx-comb branch from 90a2107 to 5b1f510 Compare September 20, 2016 19:43

timvandermeij assigned Snuffleupagus Sep 20, 2016

Text widget annotations: implement comb support

6100ab4

timvandermeij force-pushed the interactive-forms-tx-comb branch from 5b1f510 to 6100ab4 Compare September 20, 2016 20:34

Snuffleupagus requested changes Sep 21, 2016

View reviewed changes

Widget annotations: simplify field flag handling

375229d

Directly use the hexadecimal representation, just like the `AnnotationFlags`, to avoid calculations and to improve readability. This allows us to simplify the unit tests for text widget annotations as well.

timvandermeij force-pushed the interactive-forms-tx-comb branch from d38d0f8 to 375229d Compare September 21, 2016 19:15

Snuffleupagus approved these changes Sep 22, 2016

View reviewed changes

Snuffleupagus merged commit 6c263c1 into mozilla:master Sep 22, 2016

timvandermeij deleted the interactive-forms-tx-comb branch September 22, 2016 20:18

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#7649 from timvandermeij/interactive-forms-…

650df92

…tx-comb Text widget annotations: implement comb support

Text widget annotations: implement comb support #7649

Text widget annotations: implement comb support #7649

Conversation

timvandermeij commented Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Sep 20, 2016

timvandermeij commented Sep 20, 2016

pdfjsbot commented Sep 20, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 20, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Sep 20, 2016

pdfjsbot commented Sep 20, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 20, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 20, 2016

From: Bot.io (Windows)

Failed

pdfjsbot commented Sep 20, 2016

From: Bot.io (Linux)

Failed

timvandermeij commented Sep 20, 2016

pdfjsbot commented Sep 20, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 20, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 20, 2016

From: Bot.io (Linux)

Success

pdfjsbot commented Sep 20, 2016

From: Bot.io (Windows)

Success

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Sep 21, 2016

pdfjsbot commented Sep 21, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 21, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Sep 21, 2016

pdfjsbot commented Sep 21, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 21, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 21, 2016

From: Bot.io (Windows)

Failed

pdfjsbot commented Sep 21, 2016

From: Bot.io (Linux)

Failed

Snuffleupagus commented Sep 22, 2016

pdfjsbot commented Sep 22, 2016

From: Bot.io (Windows)

timvandermeij commented Sep 19, 2016 •

edited

Loading

Snuffleupagus Sep 21, 2016 •

edited

Loading

timvandermeij Sep 21, 2016 •

edited

Loading