search

box / box-content-preview

JavaScript library for rendering files stored on Box
https://developer.box.com/docs/box-content-preview
Other
106 stars 113 forks source link

feat(pdf): Move annotation styling to box-annotations #1496

Closed karelee7 closed 1 year ago

karelee7 commented 1 year ago

Description of issue: In v3.6.172 of PDFjs, a z-index styling was explicitly added to pdf_viewer.css: https://cdnjs.cloudflare.com/ajax/libs/pdf.js/3.6.172/pdf_viewer.css. This styling was not present in the previous version of PDFjs that we're using: https://cdnjs.cloudflare.com/ajax/libs/pdf.js/2.14.305/pdf_viewer.css and keeping this new styling causes issues with the other layers in preview (i.e. popup layer, highlighted layer, etc.). Updating styling for our layers resolves the issues that QA has discovered, but moving them to the box-annotations repo is preferable.

Have tested this change in the SDK with the stylings moved to box-annotations/linked with EUA.

jstoffan commented 1 year ago

@karelee7, it looks like we're in a familiar game of whack-a-mole with z-index. I have a couple concerns:

karelee7 commented 1 year ago

@jstoffan Yeah, I know that in the new PDFjs upgrade, one thing that changed was that the textLayer's z-index changed. I think that was the only update to z-index, and I think caused some issues with the other layers that we have. Hm, yeah, I'll take a look at box-annotations and see if it fits better over there

jstoffan commented 1 year ago

@karelee7, why did the z-index of the text layer change? Was it to support PDF annotations? Can we undo it safely?

bfoxx1906 commented 1 year ago

@karelee7 I was just about to say the same thing Jared did about the constant Z-index issues. We keep finding new issues. Have we looked through the code at all the z-index values set in the css and verified that they are still valid?

karelee7 commented 1 year ago

@bfoxx1906 Yeah, it's a good point, looking through it now...ah I see, so in the new PDFjs, there is an explicit styling of z-index: 2; for the textLayer in pdf_viewer.css (this z-index styling was not in the previous version of PDFjs). From the looks of it, they also added z-indexes to annotation related layers so I believe this change to the textLayer was to accomadate that. I tried removing the z-index: 2 from the textLayer styling and it seems to have resolved the issues that we were seeing. I'm going to run through all our PDFjs tests that we have in the upgrade note/double check the issues that QA ran into again to make sure