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

Ensure that Popup annotations, where the parent annotation is a polyline, will always be possible to open/close (issue 11122) #11313

Merged
merged 1 commit into from Nov 10, 2019

Conversation

Snuffleupagus
Copy link
Collaborator

For Popup annotation trigger elements consisting of an arbitrary polyline, you need to ensure that the 'stroke-width' is always non-zero since otherwise it's impossible to actually open/close the popup.

Unfortunately I don't believe that any of the test-suites can be used to test this, hence why no tests are included in the patch.

Fixes #11122

…ine, will always be possible to open/close (issue 11122)

For Popup annotation trigger elements consisting of an arbitrary polyline, you need to ensure that the 'stroke-width' is always non-zero since otherwise it's impossible to actually open/close the popup.

Unfortunately I don't believe that any of the test-suites can be used to test this, hence why no tests are included in the patch.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Nov 9, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/75d4ec4684ab641/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 9, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/75d4ec4684ab641/output.txt

Total script time: 1.67 mins

Published

@timvandermeij
Copy link
Contributor

Initially I was a bit confused because I thought the default border width was 1 if it is not found. It turns out that I added 3a6eed6 later on to fix another issue, which might have had this issue as an effect. Looking at #6179, the original issue that required that change, there is also #6179 (comment). Perhaps that's a better fix, i.e., if no border is present consider it to be width 1, but set its color to transparent? This might actually be more in line with the specification and would address both issues. We could even add a test case for the file in #6179 to verify this; I don't know why I didn't do that for that commit. What do you think?

@Snuffleupagus
Copy link
Collaborator Author

What do you think?

Hmm, interesting idea!
However would that actually be guaranteed to always work correctly, since I suppose that an annotation could explicitly set the border to 0 in which case wouldn't this problem just re-surface!?

@timvandermeij
Copy link
Contributor

Yes, but if the annotation explicitly sets a border width of 0, I think that should be respected and I don't think any other PDF viewer will show the popup in that case. This patch only applies to the case where no Border/BS entries are set at all.

@Snuffleupagus
Copy link
Collaborator Author

[...] and I don't think any other PDF viewer will show the popup in that case.

Assuming that this exact behaviour can be reproduced in e.g. Adobe Reader as well, then your solution definitely looks better :-)

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Nov 9, 2019

[...] and I don't think any other PDF viewer will show the popup in that case.

Assuming that this exact behaviour can be reproduced in e.g. Adobe Reader as well, then your solution definitely looks better :-)

I've just tried this, by modifying the PDF document from #11122, and it seems that even when setting the BS entry explicitly to /W 0 the popup can still be toggled just fine in Adobe Reader; hence I'm unfortunately not sure that this idea is viable (despite the fact that I really like it a lot).

Edit: Having slept on this, I believe that there may be another snag with the idea of always defaulting to a border width of 1 and just ignoring the colour. This will effectively reduce the size of most annotations, since the border width is usually used when computing the annotation size.

@timvandermeij timvandermeij merged commit 6763e16 into mozilla:master Nov 10, 2019
@timvandermeij
Copy link
Contributor

I also thought about this some more, but found that this patch is probably closest to what Adobe Reader does. Even if we chose the one pixel border without color, the explicit /W 0 case wouldn't be covered whereas this patch does. Thank you for fixing this!

@Snuffleupagus Snuffleupagus deleted the issue-11122 branch November 10, 2019 12:38
@Snuffleupagus
Copy link
Collaborator Author

Thanks for landing this, despite the implementation not being that great.

Following up on #11313 (comment): Do you have time to add a test-case for issue #6179 now, or should I just submit a PR with a "linked" annotation reference test for now?

@timvandermeij
Copy link
Contributor

I'm hoping it's not too difficult to create a reduced test case for this, so I'll try to make that today. If it doesn't work out, we can always use a linked test case.

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.

Ink annotation popup not shown
3 participants