Draft: Fix fragment rendering on scaled canvases in bookview #3356
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3356 +/- ##
==========================================
+ Coverage 89.20% 89.23% +0.03%
==========================================
Files 196 196
Lines 3380 3391 +11
==========================================
+ Hits 3015 3026 +11
Misses 365 365
Continue to review full report at Codecov.
|
jbaiter
changed the title
| @@ -104,9 +105,12 @@ export default class CanvasAnnotationDisplay { | |||
|
|
|||
| /** */ | |||
| fragmentContext() { | |||
| const fragment = this.resource.fragmentSelector; | |||
| let fragment = this.resource.fragmentSelector; | |||
| fragment[0] += this.offset.x; | |||
There was a problem hiding this comment.
I'm wondering now if perhaps we are calculating the offset incorrectly? Scaling the offsets before applying seem to be a useful transformation.
There was a problem hiding this comment.
So I'm thinking we might need to do the following:
- Better calculate an offset based off of the scaling
- Resize the fragment/svg based off of scaling relationship to the canvas size
|
I'm afraid I have to give up on this issue for now :-/ Hopefully someone smarter than me can pick this up :-) |
Previously, when one of the two canvases was scaled in book-view, the search annotations on this canvas would not match the page image.
This PR fixes this by passing a scaling factor from
CanvasWorldtoCanvasAnnotationDisplay, multiplication with which fixes the offset fragment so it matches the rendered canvas image.Before
After
Unfortunately the test manifest with the content search service is not public (yet), so I can't add it to the integration test suite :-/
To be honest, I'm not completely confident that this change is the best way to fix this behavior, since I have not yet completely understood how the scaling in bookview relates to the annotation overlay, but this seems to fix it in the cases I tested. If there's a better way to do this, let me know and I'll implement it :-)
-- Edit: Don't merge yet, I just found a case where this fix doesn't work, investigating...