Implement support for line annotations #8228

timvandermeij · 2017-04-02T20:06:05Z

The commit messages contain more information about the changes.

Other unimplemented annotation types will require similar code. To prepare for this, https://github.com/mozilla/pdf.js/pull/8228/files#diff-1097e3b131ba22f8e8a6c2f20e3ab5ecR687 is an array.

timvandermeij · 2017-04-02T20:06:25Z

/botio-linux preview

timvandermeij · 2017-04-02T20:10:48Z

/botio test

timvandermeij · 2017-04-02T20:46:31Z

The reference failures are expected since the border width is now taken into account when creating the container.

TEST-UNEXPECTED-FAIL | test failed chrome has not responded in 120s at the end of the test run is not expected and is likely to be related to #8217.

Snuffleupagus

I've not really had time to review this properly yet, so I'm just adding quick drive-by comments for now.

The reference failures are expected since the border width is now taken into account when creating the container.

I'm not sure that I follow why this is expected, since you're still removing the border (but now in the display layer instead). It seems to me that currently we're ignoring the border before any rendering occurs, but with this patch you are now doing it after instead, meaning that all of this code is now being (unnecessarily?) run: https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js#L168.

Would it not make more sense to do e.g. something like this following diff instead: https://gist.github.com/Snuffleupagus/437b3073c3e587d8fca6507a30a574d5?
I might be totally off, but don't really understand the first patch!

TEST-UNEXPECTED-FAIL | test failed chrome has not responded in 120s at the end of the test run is not expected and is likely to be related to #8217.

Hmm, this is a bit problematic and I think it points to Chrome having issues with (probably the SVG) added in this PR, since that's where the timeout occurs. Hence we need Chrome updated on the bot first, before we can move forward with this PR.

Snuffleupagus · 2017-04-03T10:29:44Z

src/core/annotation.js

@@ -956,6 +959,7 @@ var PopupAnnotation = (function PopupAnnotationClosure() {
    }

    this.data.parentId = dict.getRaw('Parent').toString();
+    this.data.parentType = dict.get('Parent').get('Subtype').name;


Given that parentItem is defined just above, you can use that instead of getting the Parent again here :-)
Also, in issue #7446 we learned that the Subtype can be missing (or incorrect), hence it would be a good idea to some validation here to prevent bugs, e.g. like this instead:

var parentSubtype = parentItem.get('Subtype'); this.data.parentType = isName(parentSubtype) ? parentSubtype.name : null;

Snuffleupagus · 2017-04-03T10:38:10Z

src/display/annotation_layer.js

@@ -697,7 +704,7 @@ var PopupAnnotationElement = (function PopupAnnotationElementClosure() {

      var selector = '[data-annotation-id="' + this.data.parentId + '"]';
      var parentElement = this.layer.querySelector(selector);
-      if (!parentElement) {
+      if (!parentElement || IGNORE_TYPES.indexOf(this.data.parentType) >= 0) {


Possible idea for a (general) follow-up PR:
Convert the code-base to use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes, and similarly for strings, to make these kind of checks easier to read in cases where you don't care about the index (with suitable polyfills of course).

timvandermeij · 2017-04-03T20:18:16Z

Thank you for the comments! You raised some valid points and I agree that this may be improved, so I'll definitely look into that.

The display layer is responsible for creating the HTML elements for the annotations from the core layer. If we need to ignore border styling for the containers of certain elements, the display layer should do so and not the core layer. I noticed this during the implementation of line annotations, for which we actually need the original border width in the display layer, even though we ignore it for the container. If we set the border style to zero in the core layer, this becomes impossible. To prevent this, this patch moves the container border removal code from the core layer to the display layer. This makes the core layer output the unchanged annotation data and lets the display layer remove any border styling if necessary.

timvandermeij · 2017-04-09T17:23:50Z

/botio-linux preview

timvandermeij · 2017-04-09T17:27:22Z

/botio test

timvandermeij · 2017-04-09T17:59:11Z

I have resolved all the comments you added. The Chrome update also fixed the timeout issue, so I think we're good. Note that the remaining Linux failure is a known intermittent issue after the Chrome update which has nothing to do with this patch.

Snuffleupagus

Sorry for the delay here!
I wanted some time to look through the surrounding code, to consider if there's a solution that didn't require using IGNORE_TYPES (which feels slightly hacky). However, it seems that you already thought about this, since I'm not able to come up with something better :-)

r=me, with one optional comment, since the general approach seem fine to me. Thanks for the patch!

Snuffleupagus · 2017-04-12T14:00:01Z

src/display/annotation_layer.js

@@ -697,7 +706,7 @@ var PopupAnnotationElement = (function PopupAnnotationElementClosure() {

      var selector = '[data-annotation-id="' + this.data.parentId + '"]';
      var parentElement = this.layer.querySelector(selector);
-      if (!parentElement) {
+      if (!parentElement || IGNORE_TYPES.indexOf(this.data.parentType) >= 0) {


Nit: Perhaps a micro-optimization, but how about moving the IGNORE_TYPES check before doing querySelector to avoid a potentially useless element look-up?

Good find! Fixed in the new commit.

Snuffleupagus · 2017-04-12T14:07:41Z

src/core/annotation.js

@@ -955,6 +958,8 @@ var PopupAnnotation = (function PopupAnnotationClosure() {
      return;
    }

+    var parentSubtype = parentItem.get('Subtype');
+    this.data.parentType = isName(parentSubtype) ? parentSubtype.name : null;
    this.data.parentId = dict.getRaw('Parent').toString();


This is completely out-of-scope for this PR, but it's something that I noticed which we might want try and address in a future PR.

Given that there are bad PDF files where annotations are not Refs, see e.g. PR #7570, this probably doesn't work very well in that (rare) case. Ideally I think that we probably want to somehow tap into the id as generated above, but it might not be entirely simple given that annotations can be ordered arbitrarily.
Anyway, probably worth looking into later...

This patch implements support for line annotations. Other viewers only show the popup annotation when hovering over the line, which may have any orientation. To make this possible, we render an invisible line (SVG element) over the line on the canvas that acts as the trigger for the popup annotation. This invisible line has the same starting coordinates, ending coordinates and width of the line on the canvas.

timvandermeij · 2017-04-12T21:14:44Z

/botio-linux preview

pdfjsbot · 2017-04-12T21:14:44Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2017-04-12T21:17:36Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/3ce5d85721b25eb/output.txt

Total script time: 2.85 mins

Published

timvandermeij · 2017-04-12T21:17:56Z

/botio test

pdfjsbot · 2017-04-12T21:17:56Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/89c27df2a73b1da/output.txt

pdfjsbot · 2017-04-12T21:17:57Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2017-04-12T21:41:06Z

From: Bot.io (Windows)

Success

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

Total script time: 23.14 mins

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

pdfjsbot · 2017-04-12T21:48:27Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/89c27df2a73b1da/output.txt

Total script time: 30.50 mins

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

timvandermeij · 2017-04-12T21:49:29Z

/botio makeref

pdfjsbot · 2017-04-12T21:49:29Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/90f0f4be58c00ac/output.txt

pdfjsbot · 2017-04-12T21:49:30Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2017-04-12T22:12:01Z

From: Bot.io (Windows)

Success

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

Total script time: 22.51 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2017-04-12T22:18:16Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/90f0f4be58c00ac/output.txt

Total script time: 28.78 mins

Lint: Passed
Make references: Passed
Check references: Passed

Implement support for line annotations

timvandermeij added core annotations labels Apr 2, 2017

timvandermeij requested a review from Snuffleupagus April 2, 2017 20:06

Snuffleupagus reviewed Apr 3, 2017

View reviewed changes

timvandermeij force-pushed the line-annotations branch from 313c177 to e581240 Compare April 9, 2017 17:19

Snuffleupagus approved these changes Apr 12, 2017

View reviewed changes

timvandermeij force-pushed the line-annotations branch from e581240 to e15a2ec Compare April 12, 2017 21:09

timvandermeij merged commit 32e01cd into mozilla:master Apr 12, 2017

timvandermeij deleted the line-annotations branch April 12, 2017 22:18

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

Merge pull request mozilla#8228 from timvandermeij/line-annotations

8649bb2

Implement support for line annotations

Snuffleupagus mentioned this pull request Aug 29, 2022

Properly ignore PopupAnnotations with custom trigger-elements #15360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for line annotations #8228

Implement support for line annotations #8228

timvandermeij commented Apr 2, 2017 •

edited

Loading

timvandermeij commented Apr 2, 2017

timvandermeij commented Apr 2, 2017

timvandermeij commented Apr 2, 2017 •

edited

Loading

Snuffleupagus left a comment

Snuffleupagus Apr 3, 2017

Snuffleupagus Apr 3, 2017

timvandermeij commented Apr 3, 2017

timvandermeij commented Apr 9, 2017

timvandermeij commented Apr 9, 2017

timvandermeij commented Apr 9, 2017

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Apr 12, 2017 •

edited

Loading

timvandermeij Apr 12, 2017

Snuffleupagus Apr 12, 2017

timvandermeij commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

timvandermeij commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

timvandermeij commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

Implement support for line annotations #8228

Implement support for line annotations #8228

Conversation

timvandermeij commented Apr 2, 2017 • edited Loading

timvandermeij commented Apr 2, 2017

timvandermeij commented Apr 2, 2017

timvandermeij commented Apr 2, 2017 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Apr 3, 2017

Choose a reason for hiding this comment

Snuffleupagus Apr 3, 2017

Choose a reason for hiding this comment

timvandermeij commented Apr 3, 2017

timvandermeij commented Apr 9, 2017

timvandermeij commented Apr 9, 2017

timvandermeij commented Apr 9, 2017

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

timvandermeij Apr 12, 2017

Choose a reason for hiding this comment

Snuffleupagus Apr 12, 2017

Choose a reason for hiding this comment

timvandermeij commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Apr 12, 2017

From: Bot.io (Linux)

Success

Published

timvandermeij commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Apr 12, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 12, 2017

From: Bot.io (Windows)

Success

pdfjsbot commented Apr 12, 2017

From: Bot.io (Linux)

Success

timvandermeij commented Apr 12, 2017

pdfjsbot commented Apr 12, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Apr 12, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 12, 2017

From: Bot.io (Windows)

Success

pdfjsbot commented Apr 12, 2017

From: Bot.io (Linux)

Success

timvandermeij commented Apr 2, 2017 •

edited

Loading

timvandermeij commented Apr 2, 2017 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Apr 12, 2017 •

edited

Loading