CP-14715 - Fix auto-detected form fields on rotated PDFs - #84
Conversation
What When a PDF page is stored with a rotation flag, auto-detected fillable fields now line up with the on-page widgets in the template editor, and filled values land in the correct boxes when the document is completed. Upright pages are unchanged. Why Auto-detect placed fields in the PDF's unrotated coordinate space while the editor and signing output use the rotated visual layout, so fields and values appeared offset on rotated pages. How to test 1. Create a DocuSeal paperwork task in ATS and open the template editor iframe. 2. Upload a fillable PDF with a rotated page (e.g. docuseal/tmp/paper_audit/fixtures/acro-rot90.pdf). 3. Confirm auto-detected field boxes overlay the on-page widgets (not floating elsewhere). 4. Optionally complete a signing and confirm values sit inside the boxes. 5. Upload an upright fillable PDF (e.g. acro-letter.pdf) and confirm fields still align. 6. Or run: bundle exec rspec spec/lib/templates/find_acro_fields_spec.rb
spaulsandhu
left a comment
spaulsandhu
left a comment
There was a problem hiding this comment.
🤖 This is a semi-automated review done by Paul, with assistance from a review agent. Everything is manually signed off on, but if you feel this feedback isn't helpful, please let him know!
I checked the corner math against hexapdf 1.0.3's Page#rotate rather than taking the comment's word for it, and it lines up exactly. Their flatten matrices evaluate to (y, W - x) for 90, (W - x, H - y) for 180 and (H - y, x) for 270, which is precisely what apply_page_rotation does. The premise holds up too: generate_result_attachments.rb really does page.rotate(0, flatten: true) before it reads page.box.width, and previews come off Pdfium, so visual space is the right target for all three surfaces. 👍🏽
Two other things I went looking for and was happy not to find: page[:Rotate] picks up rotation inherited from the /Pages node (hexapdf walks the tree for its inheritable fields), and % 360 means a stored -90 lands on 270 rather than falling through. Worked example per rotation in the spec comments made this a genuinely quick read.
Approving. None of the below needs to gate it: a coverage ask, one thing the fix doesn't quite reach, and a question about where this patch lives long term.
Process question rather than a code one: git log on lib/templates/find_acro_fields.rb is upstream commits the whole way down ("fix form cells", "fix standalone radio"), so as far as I can tell this would be our first fork-local change to that file. Which means a conflict to hand-resolve on every sync from docusealco/docuseal from here on.
Given the fix is self-contained and not CareerPlug-specific at all, is it worth opening it upstream too? Even if they sit on it, having the PR filed makes the eventual merge somebody else's problem instead of ours.
Not asking you to hold this one up for that, ship it either way.
—Paul-bot
|
|
||
| def build_pdf(rotate: nil) | ||
| doc = HexaPDF::Document.new | ||
| page = doc.pages.add([0, 0, page_w, page_h]) |
There was a problem hiding this comment.
Every case here uses a [0, 0, page_w, page_h] box, which leaves the one assumption in the new math untested.
apply_page_rotation takes width and height as extents, while hexapdf's own flatten uses the absolute box edges (pbox.bottom, pbox.right, pbox.top, pbox.left). Those two only agree because correct_coordinates has already shifted the Rect onto a 0-origin box before we get here. Nothing in the spec would catch it if that ordering ever changed.
Can we get one case with an offset MediaBox, say [10, 20, 622, 812] at /Rotate 90? A CropBox that differs from the MediaBox would be the other one worth having, since media_box up top is actually the CropBox whenever there is one.
| rotation = (page[:Rotate] || 0).to_i % 360 | ||
| if rotation != 0 | ||
| x0, y0, x1, y1 = apply_page_rotation(x0, y0, x1, y1, rotation, page_width, page_height) | ||
| page_width, page_height = page_height, page_width if SWAP_DIMENSION_ROTATIONS.include?(rotation) |
There was a problem hiding this comment.
One thing the swap doesn't reach, worth naming so it doesn't look covered: cell_w further down at line 91.
After a 90 or 270, w is the visual width, which for a comb field whose cells ran along media-x is now the short axis. So w / page_width / MaxLen divides the wrong side, and comb text on a rotated page still gets cells in the wrong place.
Niche enough that I wouldn't grow this PR for it, and it was equally broken before. But the description says fields line up, so I'd either carve out the comb case or drop a note here.
|
|
||
| # Map a media-space Rect through PDF /Rotate (clockwise degrees) into visual | ||
| # page space. Mirrors HexaPDF::Type::Page#rotate(0, flatten: true) corner math. | ||
| def apply_page_rotation(llx, lly, urx, ury, rotation, width, height) |
There was a problem hiding this comment.
nitpick: the "mirrors Page#rotate(0, flatten: true)" line is accurate, and I'd add the caveat that makes it true. Hexapdf builds its matrix from box edges; this takes extents, and the two coincide only because the caller hands over coordinates already normalized to a 0-origin box.
Half a sentence in the comment saves the next person the trip through correct_coordinates that I just took.
2 participants
What
When a PDF page is stored with a rotation flag, auto-detected fillable fields now line up with the on-page widgets in the template editor, and filled values land in the correct boxes when the document is completed. Upright pages are unchanged.
Why
Auto-detect placed fields in the PDF's unrotated coordinate space while the editor and signing output use the rotated visual layout, so fields and values appeared offset on rotated pages.
How to test