Improved annotations' display/behavior. #4318
Conversation
|
could you provide couple of test documents with steps to replicate? /botio-windows preview |
From: Bot.io (Windows)ReceivedCommand cmd_preview from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/70b9ed8a784bb31/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/70b9ed8a784bb31/output.txt Total script time: 0.33 mins Published
|
|
This seems to break the display of normal links in PDFs, since they are no longer standard hyperlinks. For example, the mouse cursor doesn't change to indicating that links are clickable. Also, with these changes, the HTML code for standard links have become unnecessarily complex, which I guess might explain some of the issues. Btw, I tested with: http://mirrors.ctan.org/info/lshort/english/lshort.pdf. Edit: Note that the links that are not working corresponds to internal destinations, which is a common way to link to other parts of the PDF file. |
|
Here is a test file with different sorts of text annotations and a link: https://dl.dropboxusercontent.com/u/1910542/interactiveAnnotations.pdf This is what it looks like with all annotations open: And in Adobe Reader: The link annotations' behavior is now the same as in Adobe Reader. The only difference is the highlighting effect on mouseover that is common to all interactive annotation so they can programmatically be highlighted through a common interface. If this is not desirable, I can change it back. |
|
In my tests the cursor does change on links. Let me look what would be the difference with your test file. |
|
I had missed the particular case of internal links. It is now fixed and the sample PDF is updated. Thanks for your test. Edit: just squashed the commits |
ghost
mentioned this pull request
|
Really nice patch! Especially https://dl.dropboxusercontent.com/u/1910542/interactiveAnnotations.pdf has improved a lot. This patch also seems to fix #3654. |
|
@dferer I have added some comments about your code. The comments mainly address minor style issues as outlined in https://github.com/mozilla/pdf.js/wiki/Style-Guide. If you have addressed these comments, please squash your commits again to make review easier. Thanks! :) |
|
@timvandermeij Done (I answered one of your comments). Thanks for the review! |
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/c12f9e4084fafea/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/33dd9dd94b01582/output.txt |
|
FYI: some tests are expected to fail due to some text annotations' appearances (AP) that were previously ignored. |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/c12f9e4084fafea/output.txt Total script time: 23.58 mins
Image differences available at: http://107.21.233.14:8877/c12f9e4084fafea/reftest-analyzer.xhtml#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/33dd9dd94b01582/output.txt Total script time: 35.27 mins
Image differences available at: http://107.22.172.223:8877/33dd9dd94b01582/reftest-analyzer.xhtml#web=eq.log |
|
(ff/linux might fail for some other tests) |
|
Is there a way for me to see the image differences? The link in bot.io's output only shows an empty text area. |
|
The link http://107.22.172.223:8877/33dd9dd94b01582/reftest-analyzer.xhtml#web=eq.log starts with an empty text area, but should show the image differences after a second (or whenever the images are loaded). What browser are you using? |
|
I am using Safari 7.0.1 (OSX) and nothing happens even after a couple minutes. I guess that would explain it ;): It does work with Firefox, and I confirm, the errors are the ones I expected. |
|
@dferer I have just added some last review comments. It looks good to me when those are addressed. @yurydelendik Could you also review this? And can you reproduce the issue with the reftest analyzer in Safari? |
| item.colorCssRgb = Util.makeCssRgb(rgb); | ||
|
|
||
| var highlight = document.createElement('div'); | ||
| var highlightOffset = 4; // px |
There was a problem hiding this comment.
You might want to consider adding this as a constant (i.e. var HIGHLIGHT_OFFSET = 4; // px placed just below use strict;) at the top of the file instead, to make it easier to find and change this value in the future.
|
@dferer Could you squash the commits again? |
|
Done |
ghost
closed this
ghost
reopened this
|
Oops wrong button |
|
/botio test |
|
Done |
|
Awesome /botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/2a1c0b987850c6a/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/2a1c0b987850c6a/output.txt Total script time: 0.35 mins Published
|
|
/botio test Probably not a big deal. I'm kinda missing yellow box shadow for links, but that's probably because I used to see it for long time. If other people will miss it too, we will add something. |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/482ee73a283a329/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/5b6b8867cc1050d/output.txt |
ghost
closed this
ghost
reopened this
That can easily be added back, just change this line: https://github.com/mozilla/pdf.js/blob/master/web/viewer.css#L1257 to Also with this patch, can we remove the following lines: https://github.com/mozilla/pdf.js/blob/master/web/viewer.css#L1250-L1256? Edit: Removing the yellow box, as is done (inadvertently) by this PR, would fix: https://bugzilla.mozilla.org/show_bug.cgi?id=960580. |
|
Done |
Added an "InteractiveAnnotation" class to homogenize the annotations' structure (highlighting) and user interactions (for now, used for text and link annotations). Text annotations: The appearance (AP) has priority over the icon (Name). The popup extends horizontally (up to a limit) as well as vertically. Reduced the title's font size. The annotation's color (C) is used to color the popup's background. On top of the mouseover show/hide behavior, a click on the icon will lock the annotation open (for mobile purposes). It can be closed with another click on either the icon or the popup. An annotation printing is conditioned by its "print" bit Unsupported annotations are not displayed at all.
|
I put them back for now, although I would advocate removing them as it the default behavior in most other PDF readers. |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/5b6b8867cc1050d/output.txt Total script time: 35.83 mins
Image differences available at: http://107.22.172.223:8877/5b6b8867cc1050d/reftest-analyzer.html#web=eq.log |
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/c61129fd2805182/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/c61129fd2805182/output.txt Total script time: 0.36 mins Published
|
Improved annotations' display/behavior.
|
Thank you for the patch |
ghost
deleted the
|
/botio makeref |
From: Bot.io (Linux)ReceivedCommand cmd_makeref from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/b97d8887d029f43/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/dbd3de9151b9cbc/output.txt |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/b97d8887d029f43/output.txt Total script time: 0.09 mins |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/dbd3de9151b9cbc/output.txt Total script time: 0.12 mins |
Added an "InteractiveAnnotation" class to homogenize appearance (highlighting) and user interactions (for now, used for text and link annotations).
Text annotations:
The appearance (AP) has priority over the icon (Name).
The popup extends horizontally (up to a limit) as well as vertically.
Reduced the title's font size.
The annotation's color (C) is used to color the popup's background.
On top of the mouseover show/hide behavior, a click on the icon will lock the annotation open (for mobile purposes). It can be closed with another click on either the icon or the popup.