Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image occlusion doesn't scale when page zoomed in #2588

Open
dae opened this issue Jul 24, 2023 · 6 comments
Open

Image occlusion doesn't scale when page zoomed in #2588

dae opened this issue Jul 24, 2023 · 6 comments

Comments

@dae
Copy link
Member

dae commented Jul 24, 2023

Because a max-height is set on the occlusion container, zooming the page in with ctrl+mousewheel results in the image actually shrinking slightly, due to the change in margins.

If we use an explicit height, instead of capping the height relative to the viewport, then scaling works, but images do not fit the screen by default.

diff --git a/ts/image-occlusion/review.scss b/ts/image-occlusion/review.scss
index 8e4065fd1..95da4ba5a 100644
--- a/ts/image-occlusion/review.scss
+++ b/ts/image-occlusion/review.scss
@@ -5,7 +5,8 @@
     transform: translate(-50%, 0);
     // allow for 20px margin on html element, or short windows can truncate
     // image
-    max-height: calc(95vh - 40px);
+    //max-height: calc(95vh - 40px);
+    height: 1000px;
 }
 
 #image-occlusion-container img {

If anyone has ideas on how we could both scale-to-fit by default (and when window is resized), but also support zooming, please let us know!

Heads up @krmanik @glutanimate

@krmanik
Copy link
Contributor

krmanik commented Jul 24, 2023

I have tested by start review and zoom in then go back to deck selection the zoom is correct but incorrect for reviewer (in io notetype). Again, restart review and zoom out and then again go back to deck selection and the zoom is again incorrect. Also removing CSS for all three class and checking zoom have different result than expected. The possible solution should be in JS implementation, and we may not need to change CSS of above class. Should this solution be used?

diff --git a/ts/image-occlusion/review.ts b/ts/image-occlusion/review.ts
index 580d46fe3..620fd6b16 100644
--- a/ts/image-occlusion/review.ts
+++ b/ts/image-occlusion/review.ts
@@ -11,6 +11,8 @@ import { Polygon } from "./shapes/polygon";
 import { Rectangle } from "./shapes/rectangle";
 import type { Size } from "./types";
 
+let scale = 1.0;
+
 export function setupImageCloze(): void {
     window.addEventListener("load", () => {
         window.addEventListener("resize", setupImageCloze);
@@ -32,6 +34,14 @@ function setupImageClozeInner(): void {
         return;
     }
 
+    container.addEventListener("wheel", (e) => {
+        e.preventDefault();
+        const step = 0.05;
+        const delta = Math.sign(e.deltaY) * step;
+        scale = Math.max(scale + delta, 0.1);
+        container.style.transform = `translate(-50%, 0) scale(${scale})`;
+    });
+
     // Enforce aspect ratio of image
     container.style.aspectRatio = `${image.naturalWidth / image.naturalHeight}`;
 

@dae
Copy link
Member Author

dae commented Jul 25, 2023

That seems to work well when using the mouse wheel. Unfortunately, users may also be zooming in via the View>Zoom In/Out or Reset Zoom options - I wonder if there's a way we can handle that case too?

Interestingly, no patch is required to make it work correctly when using pinch-to-zoom on a Mac, as some sort of different zooming strategy seems to be used for the pinch gesture.

glutanimate added a commit to glutanimate/anki that referenced this issue Jul 25, 2023
Introduces a custom JS-based zoom system for the reviewer that enables zooming into viewport-constrained images.

Additionally adds zoom persistence across Anki sessions.

Fixes Image occlusion doesn't scale when page zoomed in ankitects#2588

(but also addresses general image zoom issues across all note types)
@glutanimate
Copy link
Contributor

glutanimate commented Jul 25, 2023

I would say that handling this on an image-level could be a viable option for IO 👍

However, this definitely is an issue for other note types as well and has been a source of annoyance for me for a while now, so I tried to shoot for a more generic approach in #2591. Did not come out ideal given that I had to go with two zoom modes, but I do think it could be a reasonable option if we want to cover more problem cases other than IO.

@dae
Copy link
Member Author

dae commented Sep 18, 2023

Edit: sorry, ignore that - I confused this issue with a different one.

@dae dae closed this as completed Sep 18, 2023
@dae dae reopened this Sep 18, 2023
@BrayanDSO
Copy link
Contributor

BrayanDSO commented Oct 30, 2023

Maybe related, but I noticed that if you zoom in the page with Ctrl+Mouse wheel up, the masks resolution becomes low (Windows 11, Qt6, 23.10rc3)

2023-10-30.11-04-53.mp4

Also, the image zoom is reset every time the window zoom is changed with Ctrl+Mouse wheel up

2023-10-30.11-06-33.mp4

@krmanik
Copy link
Contributor

krmanik commented Nov 10, 2023

For mask editor, zoom issues fixed by #2809.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants