Find and remove child widgets from the document context #1002
Conversation
When removing a field from the document, we need to remove the children of the field first; otherwise, some renderers will still see the remnants and display these dangling widgets.
|
I saw you mentioned there was a failing integration test (over here #1001 (comment)). Is that still the case, or were you able to fix it? |
|
@Hopding I was able to fix the issue. It was occurring because I was simply removing the appearance reference from the document context, but the widget reference itself was still registered and referring to the now missing reference. This new approach looks for the widget reference itself by looping through the field's Kids and removes that instead. This now passes all unit tests and I didn't spot any errors in the integration tests. |
There was a problem hiding this comment.
@Trapfether this looks great! Thanks so much for making a PR. Lots of folks post issues, but it's relatively rare that anybody makes a PR to fix them 🙂
My only ask is that you add some unit tests to exercise this logic. It would also be nice if you could update an integration test so that if there's a regression here we'll be more likely to catch it.
|
I can absolutely do that. A question on the Integration test. As the weird behavior only seemed to appear when using the pdf.js library to render the pdf, I would need to add that as a testing asset. My question would be whether to make this another web integration test in the existing app:web suite, or should it be a new suite such as app:pdfjs. As pdfjs is currently actively maintained, I don't think it hurts either way; however, the second method may be easier to deprecate should pdfjs stop being actively maintained at some point. There is also the question of which version of pdf.js to test against. Alternatively, because we know that the visual aberration is the result of a dangling reference to a field's widget in the document context, I theoretically should be able to cover possible regressions through the unit test alone by testing whether child widget references are left after field removal. Thank you for the encouragement and for taking the time to answer my questions. |
|
@Trapfether Good question! Just adding it to the existing web suite should suffice. All of the web tests are run in Firefox (and other browsers) prior to cutting a release, and Firefox uses pdf.js to render PDFs. Of course, the test would no longer work if pdf.js is updated to correct the issue. But the unit tests will still prevent regressions within pdf-lib in that scenario. So just adding a bit of code to one existing integration test will do the trick. Note, however, that all the integration tests are kept in sync across all the apps. So you'll have to update the test in P.S. I just noticed you're located in Wichita, KS. That's awesome! I grew up nearby in Derby, KS 🙂 |
We add and remove a widget to page 5. Text above the location should direct the individual to look for any visual remnants of a field
|
@Hopding I have added a unit test to PDFForm to specifically check for dangling child references. Interestingly, we already had a test to do that in PDFForm; however, it loaded an existing PDF and removed the pre-existing fields from it. I essentially duplicated this test and created a PDF from scratch and added forms via the library instead. I added a section in integration test 1 that adds and removes a field. There is text above the placement of this field saying "There should be no remnant of a field below this text". I made sure that text and field were the only thing on the bottom right of that page to make sure no future test additions should cause confusion. Derby is awesome, I go to the theatre there every once in a while because they do throwback showings. |
Fixes #1001
When removing a field from the document, we need to remove the children of the field first; otherwise, some renderers will still see the remnants and display these dangling widgets.
What?
Finds and removes child widgets of a field and removes them from the document context before removing field itself.
Why?
Stops some renderers from still finding the dangling widgets and rendering their default appearances
How?
In the removeField function, right before removing the field reference from the document context, we look for the field children by getting it's normalizedEntries. We check each child to see if it is indeed a reference; in which case, it is removed from the document context
Testing?
I tested the changes against the automated testing suite and the integrated tests, especially integration test 18 which removes several fields as part of it's test.
New Dependencies?
No
Screenshots
Before Fix.pdf
After Fix.pdf
Suggested Reading?
Partly
Anything Else?
Linter found style issue in the /src/api/index.ts file. I did not push that change as it was unrelated to this fix.
Checklist