fix(highlight): Ignore empty rects when calculating bounding rect #696
Conversation
src/highlight/highlightUtil.ts
Outdated
| @@ -10,6 +10,11 @@ export const getBoundingRect = (shapes: Shape[]): Shape => { | |||
| const x2 = x + width; | |||
| const y2 = y + height; | |||
|
|
|||
| // Removing extra rects | |||
There was a problem hiding this comment.
@karelee7, I think that the textLayerMode option we pass to PDF.js from Preview SDK is supposed to help with this. Is that still working properly in the latest version?
There was a problem hiding this comment.
No, unfortunately :( In the latest version, I still see the extra rects that have widths of 0 and heights of 0; confirmed that after filtering out these extra rects, the tooltip is positioned correctly..
There was a problem hiding this comment.
We should follow-up on it and see if we're missing something. Did the option change somehow or get removed?
There was a problem hiding this comment.
Hmm looks like the option's still there and the enum values for TextLayerMode look the same
There was a problem hiding this comment.
@jstoffan Yeah..it looks like the text layer is created with the same dimensions as it has in the current version of pdfjs and I can see that the pdf viewer is created with the ENABLE_ENHANCE option..hm...
There was a problem hiding this comment.
Do we see this "empty rect" behavior in the latest version of pdfjs? If we do, maybe we should open an issue in that repo to bring awareness
There was a problem hiding this comment.
Yeah, opened up an issue! I saw someone else on Stack Overflow also had a similar issue with rect values with a fairly recent version of pdfjs, but their question was never resolved aha, hopefully someone gets to the issue on the repo~ Here's a link to the issue on the repo: mozilla/pdf.js#15138
There was a problem hiding this comment.
Response received per the pdfjs team for the issue link mentioned above: Please keep in mind that the ENABLE_ENHANCE mode is essentially unmaintained at this point, and no real development has happened for years. Furthermore, in the next major PDF.js release that text-selection mode is also most likely going to be removed.
There was a problem hiding this comment.
Thanks @karelee7. I'd be curious to know how the regular ENABLE mode performs at this point?
There was a problem hiding this comment.
No problem! Sure, I can see if they have any updates about that!
src/highlight/highlightUtil.ts
Outdated
| @@ -10,6 +10,11 @@ export const getBoundingRect = (shapes: Shape[]): Shape => { | |||
| const x2 = x + width; | |||
| const y2 = y + height; | |||
|
|
|||
| // Removing extra rects | |||
There was a problem hiding this comment.
Do we see this "empty rect" behavior in the latest version of pdfjs? If we do, maybe we should open an issue in that repo to bring awareness
|
Maybe the commit should be |
karelee7
changed the title
|
@karelee7, nice job. Let's make sure this goes out to production prior to the Preview SDK changes. |
|
@jstoffan Thanks for all the help! Yes, I'll get this one in first! ;) |
No description provided.