Created attachment 134120 [details] [review]
patch

Created attachment 134121 [details]
test.pdf

Created attachment 134122 [details]
test.pdf

Created attachment 134123 [details]
test.cpp

Test case:
1. compile and run test.cpp with test.pdf in the same folder
2. output is the border width of the input box in the pdf as read by the library
The border width should be 3.

Alternative test method:
1. open test.pdf in evince or okular
2. type some text in the input box and save it
3. open it again and observe if the border is still there
If the border is gone, the library did not parse the border width correctly.

PS: test.pdf is generated by adobe acrobat and then decompressed with qpdf.

jrmuizel, since you wrote the original commit, what do you think?

This causes a regression on the rendering of the last page of file https://bugs.freedesktop.org/attachment.cgi?id=21065

Created attachment 134284 [details]
acrobat-screenshot.png

Interestingly enough, adobe acrobat just ignores /Border when /BS is present, so it draws a 1px border. The pdf specs allows /Border to be ignored in favor of /BS.

I don't consider this to be a regression, rather the original implementation is bugged. In fact the original implementation also ignored /Border, it just assumed there is no border because /W is missing in /BS, instead of using the default value of a 1px border.

Snippet from pdf file:
>  /A <<
>    /S /URI
>    /URI (http://www.ti.com/wireless)
>  >>
>  /BS <<
>    /Type /Border
>  >>
>  /Border [
>    0
>    0
>    0
>  ]

(In reply to Andrew Chen from comment #7)
> I don't consider this to be a regression, 

But you're not the maintainer and I am. There's a file that worked and with your patch does not. That's a regression, until that gets fixed the patch described here can't get merged in the codebase.

I apologize if my previous comment has offended you.

The reason I don't want to call it a regression is because I don't see any definitive standard we can adhere to in this case. So it is worth discussing this instead of blindly following what the original behavior is.

I can see two possible courses of action:
1. Follow acrobat: ignore /Border when /BS is present. This leads to a (default) 1px border rendered in your pdf.
2. Preferentially use values from /BS but also use values from /Border if the corresponding values are not present in /BS. This would cause no border to be rendered in your pdf.

The second one seems like the more logical thing to do but the first option makes for better compatibility.

I personally think maintaining compatibility with acrobat is the better option, but I can also see why it would be a good idea to just try make the most sense of of the pdf.

Rendering compatibility with Adobe Reader is crucial

You could argue that the getters should still return the "correct" values from the PDF and be ignored somewhere else for drawing, i wouldn't disagree if you had a patch that implemented that properly for all the rendering backends.

Created attachment 134452 [details] [review]
initial implementation

I separated the field Annot::border into two fields borderArray and borderBS. That will allow both /Border and /BS to be retrieved by getters. getBorder() returns borderBS if available, and borderArray.

There are still a few problems with this:
1. If both /Border and /BS are present in a pdf file, only /BS will be kept when it is saved again.
2. borderArray and borderBS are not kept in sync, changes to one will not be reflected in the other.

Do you have a plan to solve those issues?

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/544.