Implement support for Popup annotations #6792

timvandermeij · 2015-12-22T21:10:24Z

Most code for Popup annotations is already present for Text annotations. This patch extracts the popup creation logic from the Text annotation code so it can be reused for Popup annotations.

Not only does this add support for Popup annotations, the Text annotation code is also considerably easier. If a Popup entry is available for a Text annotation, it will not be more than an image. The popup will be handled by the Popup annotation. However, it is also possible for Text annotations to not have a separate Popup annotation, in which case the Text annotation handles the popup creation itself.

Snuffleupagus · 2015-12-22T21:34:01Z

Would you be willing to review this patch?

I suppose I can try, but I won't have time until after Christmas.

However, it seems that this PR currently breaks most of the annotations in http://tug.ctan.org/tex-archive/macros/latex/contrib/pdfcomment/doc/pdfcomment.pdf, since nothing happens when they are hovered/clicked.

timvandermeij · 2015-12-22T21:46:32Z

It seems that it is also possible for Text annotations to not have an associated Popup annotation (which is the case in that PDF file), in which case the Text annotation needs to provide the popup and not the Popup annotation. This case can be discovered by checking if the Text annotation has a Popup dictionary element. If so, the Popup annotation takes care of the popup and otherwise the Text annotation should do that. Thanks a lot, Adobe... :(

Issues to address:

Solve the ZapfDingbats test failure

It turned out that the file is somewhat corrupt because the parent of the Popup annotation cannot be fetched; Adobe Acrobat ignores this invalid annotation, we now issue a console warning and ignore it too (see timvandermeij@991e501#diff-18ca06e2bd5be4c9132b51f78388b8f0R756).
Fix popup rendering for the tests

parentItem in src/display/annotation_layer.js was null because in Node.js document is different than in the regular browser case. We now perform the query selector on the actual annotation div, which is also a bit faster as we do not have to traverse the entire document anymore.
Handle absence of Popup annotations

The popup code has been factored out to let both Text and Popup annotations use it.
Wrapper width and word wrapping

The wrapper width is now correct and word wrapping is applied for very long words that do not fit in the container. In the current implementation such a text would overflow the container, but not anymore with this patch.

Snuffleupagus · 2015-12-22T22:31:57Z

src/core/annotation.js

+      warn('Popup annotation has an invalid parent annotation.');
+      return;
+    }
+    this.data.parentId = dict.map.Parent.num;


Please note that you should not access members of a Dict by directly using map (it should be considered private), use e.g. Dict_get instead.

The main problem with using Dict.get('Parent') is that the ID (num) is not in the returned object anymore (or at least I could not find it). Do you know of a better way of obtaining this (internal) annotation ID other than the method I used? I will happily use that then.

Edit: There is Dict.get('Parent').objId, but its value is 29R (string) instead of the correct 29 (integer). Something like parseInt(Dict.get('Parent').objId, 10); should yield the same result as my method, but I'm not sure if it's better because objId still seems like something private and it's more code. What do you think?

You probably want Dict_getRaw, since it looks like you want to access a Reference. And I don't think that you should use objId when you have access to a reference, since it's also somewhat private, and should probably not be relied on.

But the real question here is: if you have a Ref, why do you directly access just one of its properties? In practice, the gen is usually zero, but you cannot assume that that always holds, so I really think that you ought to use Ref_toString instead!

This leads me to look at the current code in annotation.js, and I thus think that https://github.com/mozilla/pdf.js/blob/master/src/core/annotation.js#L163 is actually wrong! That line really ought to read this.data.id = params.ref.toString();, which means that changing the current line to this.data.parentId = dict.getRaw('Parent').toString(); should then work just fine.

Done in the new commit. Really nice find!

Snuffleupagus · 2015-12-23T08:30:54Z

src/core/annotation.js

+    }
+
+    var parentItem = dict.get('Parent');
+    if (!parentItem) {


Is it necessary to have two different cases for when the Parent entry isn't present, or is invalid?
Would it not suffice to just replace both of them with e.g.

... var dict = parameters.dict; var parentDict = dict.get('Parent'); if (!parentDict) { warn(...); return; } this.data.parentId = ...

Done in the new commit. Much easier indeed.

Snuffleupagus · 2015-12-24T16:23:27Z

src/display/annotation_layer.js

+      // Attach the event listeners and position the container. A parent
+      // element is only available when its type is supported.
+      var selector = '[data-annotation-id="' + this.data.parentId + '"]';
+      var parentItem = this.layer.querySelectorAll(selector)[0];


Is querySelectorAll(...)[0] really necessary, couldn't you simply use querySelector instead, since those should be equivalent?

Done in the new commit.

Snuffleupagus · 2015-12-24T19:25:07Z

src/core/annotation.js

    }

-    if (dict.has('C')) {
-      data.hasBgColor = true;
+    this.data.popup = !!dict.get('Popup');


Since this just contains a boolean, how about naming it data.hasPopup instead?
Also, wouldn't dict.has('Popup) work here?

Could you add a linked test-case for this particular code-path (or if it's not too much trouble, create a reduced one)?

dict.has('Popup') is indeed working just fine. Not sure why I did not use that in the first place...

I'll try to create a reduced test case, but otherwise I will certainly add a linked test case.

You probably still need this.data.hasPopup though, since you've got code in annotation_layer.js that seem to depend on that flag being present :)

But I really like the general direction this PR is going!

You're right. Fixed in the new commit. I have managed to create a reduced test case, which is now also in the commit.

Snuffleupagus · 2015-12-24T19:28:00Z

src/display/annotation_layer.js

+          title: this.data.title,
+          contents: this.data.contents
+        };
+        var popupElement = new PopupElement(parameters);


As far as I'm concerned, these kind of temporary variables doesn't really add much in terms of readability. You could just do:

var popupElement = new PopupElement({ parentContainer: this.container, parentTrigger: image, color: this.data.color, title: this.data.title, contents: this.data.contents });

Done in the new commit, also for the other place where I did this for this commit. Looks better that way.

I'm just trying to find small things to make the total diff smaller ;)

Snuffleupagus · 2015-12-25T05:42:48Z

src/core/annotation.js

+      this.data.color = null;
+    }
+
+    this.data.popup = dict.has('Popup');


I'm sorry, but I'd still like this to be named this.data.hasPopup, given that it's a boolean.

Done in the new commit. I had missed this before, so thanks for reminding me!

Most code for Popup annotations is already present for Text annotations. This patch extracts the popup creation logic from the Text annotation code so it can be reused for Popup annotations. Not only does this add support for Popup annotations, the Text annotation code is also considerably easier. If a `Popup` entry is available for a Text annotation, it will not be more than an image. The popup will be handled by the Popup annotation. However, it is also possible for Text annotations to not have a separate Popup annotation, in which case the Text annotation handles the popup creation itself.

timvandermeij · 2015-12-25T12:32:24Z

I forgot to mention how you can test the reference image rendering locally, though you might already know this. First make a backup of your current reference images, then simply remove the test/ref and test/tmp directories, clear the test manifest to only leave the two at https://github.com/mozilla/pdf.js/pull/6792/files#diff-b19c886c212474d3b37d6026a795a246R2619 and run node make botmakeref. The results can be inspected in test/ref.

Thank you for the helpful comments so far and merry Christmas! :-)

timvandermeij · 2015-12-25T12:33:14Z

/botio-linux preview

pdfjsbot · 2015-12-25T12:33:14Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/8d36eb7e61e5580/output.txt

pdfjsbot · 2015-12-25T12:34:03Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/8d36eb7e61e5580/output.txt

Total script time: 0.81 mins

Published

timvandermeij · 2015-12-25T12:39:02Z

/botio test

pdfjsbot · 2015-12-25T12:39:02Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2015-12-25T12:39:02Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/0bad30f904f75a8/output.txt

pdfjsbot · 2015-12-25T12:59:33Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/0bad30f904f75a8/output.txt

Total script time: 20.52 mins

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

pdfjsbot · 2015-12-25T12:59:39Z

From: Bot.io (Linux)

Success

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

Total script time: 20.61 mins

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

Snuffleupagus · 2015-12-26T17:51:37Z

Thank you for the patch!

/botio makeref

pdfjsbot · 2015-12-26T17:51:37Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2015-12-26T17:51:37Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/1f892d746193e3e/output.txt

Implement support for Popup annotations

pdfjsbot · 2015-12-26T18:11:48Z

From: Bot.io (Windows)

Success

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

Total script time: 20.18 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2015-12-26T18:12:08Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/1f892d746193e3e/output.txt

Total script time: 20.51 mins

Lint: Passed
Make references: Passed
Check references: Passed

timvandermeij added the annotations label Dec 22, 2015

timvandermeij assigned Snuffleupagus Dec 22, 2015

timvandermeij force-pushed the popup-annotation branch from 3459427 to 97fc47d Compare December 22, 2015 22:14

Snuffleupagus reviewed Dec 22, 2015
View reviewed changes

timvandermeij force-pushed the popup-annotation branch from 97fc47d to 5cbcf15 Compare December 22, 2015 23:07

Snuffleupagus reviewed Dec 23, 2015
View reviewed changes

timvandermeij force-pushed the popup-annotation branch from 5cbcf15 to 00aa8e1 Compare December 24, 2015 15:32

Snuffleupagus reviewed Dec 24, 2015
View reviewed changes

timvandermeij force-pushed the popup-annotation branch 3 times, most recently from 7a681ed to 9994138 Compare December 24, 2015 19:16

Snuffleupagus reviewed Dec 24, 2015
View reviewed changes

timvandermeij force-pushed the popup-annotation branch from 9994138 to 51886ca Compare December 24, 2015 19:27

Snuffleupagus reviewed Dec 24, 2015
View reviewed changes

timvandermeij force-pushed the popup-annotation branch 8 times, most recently from 8897031 to c3ee03e Compare December 24, 2015 23:00

Snuffleupagus reviewed Dec 25, 2015
View reviewed changes

timvandermeij force-pushed the popup-annotation branch 2 times, most recently from 7a9282d to a231e26 Compare December 25, 2015 12:16

timvandermeij force-pushed the popup-annotation branch from a231e26 to 7d43971 Compare December 25, 2015 12:18

Snuffleupagus added a commit that referenced this pull request Dec 26, 2015

Merge pull request #6792 from timvandermeij/popup-annotation

cba8a87

Implement support for Popup annotations

Snuffleupagus merged commit cba8a87 into mozilla:master Dec 26, 2015

timvandermeij deleted the popup-annotation branch December 26, 2015 19:47

Snuffleupagus mentioned this pull request Dec 27, 2015

Popup annotations reserve space even when they are hidden #6806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for Popup annotations #6792

Implement support for Popup annotations #6792

timvandermeij commented Dec 22, 2015

Snuffleupagus commented Dec 22, 2015

timvandermeij commented Dec 22, 2015

Snuffleupagus Dec 22, 2015

timvandermeij Dec 22, 2015

Snuffleupagus Dec 23, 2015

timvandermeij Dec 24, 2015

Snuffleupagus Dec 23, 2015

timvandermeij Dec 24, 2015

Snuffleupagus Dec 24, 2015

timvandermeij Dec 24, 2015

Snuffleupagus Dec 24, 2015

Snuffleupagus Dec 24, 2015

timvandermeij Dec 24, 2015

Snuffleupagus Dec 24, 2015

timvandermeij Dec 24, 2015

Snuffleupagus Dec 24, 2015

timvandermeij Dec 24, 2015

Snuffleupagus Dec 24, 2015

Snuffleupagus Dec 25, 2015

timvandermeij Dec 25, 2015

timvandermeij commented Dec 25, 2015

timvandermeij commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

timvandermeij commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

Snuffleupagus commented Dec 26, 2015

pdfjsbot commented Dec 26, 2015

pdfjsbot commented Dec 26, 2015

pdfjsbot commented Dec 26, 2015

pdfjsbot commented Dec 26, 2015

Implement support for Popup annotations #6792

Implement support for Popup annotations #6792

Conversation

timvandermeij commented Dec 22, 2015

Snuffleupagus commented Dec 22, 2015

timvandermeij commented Dec 22, 2015

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Dec 25, 2015

timvandermeij commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 25, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Dec 25, 2015

pdfjsbot commented Dec 25, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 25, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 25, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 25, 2015

From: Bot.io (Linux)

Success

Snuffleupagus commented Dec 26, 2015

pdfjsbot commented Dec 26, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 26, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 26, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 26, 2015

From: Bot.io (Linux)

Success