wasd fix

web/packages/gallery/components/viewer/FileViewer.tsx

@@ -704,6 +704,10 @@ const FileViewer: React.FC<FileViewerProps> = ({
             };
         }
     }, [
+        // Be careful with adding new dependencies here, or changing the source
+        // of existing ones. If any of these dependencies change unnecessarily,
+        // then the file viewer will start getting reloaded even when it is
+        // already open.
         open,
         onClose,
         user,
@@ -906,9 +910,9 @@ const Shortcuts: React.FC<ModalVisibilityProps> = ({ open, onClose }) => (
             <Shortcut action="Pan" shortcut="WASD, Drag" />
             <Shortcut action="Toggle favorite" shortcut="L" />
             <Shortcut action="View info" shortcut="I" />
-            <Shortcut action="Download" shortcut="^D, ⌘D" />
+            <Shortcut action="Download" shortcut="K" />
             <Shortcut action="Delete" shortcut="Delete, Backspace" />
-            <Shortcut action="Copy as PNG" shortcut="^C, ⌘C" />
+            <Shortcut action="Copy as PNG" shortcut="^C / ⌘C" />
             <Shortcut action="Toggle fullscreen" shortcut="F" />
         </ShortcutsContent>
     </Dialog>

web/packages/gallery/components/viewer/photoswipe.ts

@@ -451,7 +451,7 @@ export class FileViewerPhotoSwipe {
         // State needed to hide the caption when a video is playing.
         let videoElement: HTMLVideoElement | undefined;
         let onVideoPlayback: EventHandler | undefined;
-        let captionElementRef: HTMLElement | undefined;
+        let captionElement: HTMLElement | undefined;
 
         pswp.on("change", (e) => {
             const itemData = this.pswp.currSlide.content.data;
@@ -466,7 +466,7 @@ export class FileViewerPhotoSwipe {
             }
 
             // Reset.
-            showIf(captionElementRef, true);
+            showIf(captionElement, true);
 
             // Attach new listeners, if needed.
             if (itemData.fileType == FileType.video) {
@@ -474,7 +474,7 @@ export class FileViewerPhotoSwipe {
                 videoElement = contentElement.getElementsByTagName("video")[0];
                 if (videoElement) {
                     onVideoPlayback = (e) => {
-                        showIf(captionElementRef, !!videoElement?.paused);
+                        showIf(captionElement, !!videoElement?.paused);
                     };
                     videoElement.addEventListener("play", onVideoPlayback);
                     videoElement.addEventListener("pause", onVideoPlayback);
@@ -594,10 +594,12 @@ export class FileViewerPhotoSwipe {
                     isButton: true,
                     html: createPSRegisterElementIconHTML("favorite"),
                     onClick: handleToggleFavorite,
-                    onInit: (buttonElement) =>
+                    onInit: (buttonElement) => {
+                        favoriteButtonElement = buttonElement;
                         pswp.on("change", () =>
                             showFavoriteIf(buttonElement, false),
-                        ),
+                        );
+                    },
                 });
                 pswp.ui.registerElement({
                     name: "unfavorite",
@@ -606,10 +608,12 @@ export class FileViewerPhotoSwipe {
                     isButton: true,
                     html: createPSRegisterElementIconHTML("unfavorite"),
                     onClick: handleToggleFavorite,
-                    onInit: (buttonElement) =>
+                    onInit: (buttonElement) => {
+                        unfavoriteButtonElement = buttonElement;
                         pswp.on("change", () =>
                             showFavoriteIf(buttonElement, true),
-                        ),
+                        );
+                    },
                 });
             } else {
                 // When we don't have a user (i.e. in the context of public
@@ -674,18 +678,16 @@ export class FileViewerPhotoSwipe {
                 order: 30,
                 appendTo: "root",
                 tagName: "p",
-                onInit: (captionElement, pswp) => {
+                onInit: (element, pswp) => {
-                    captionElementRef = captionElement;
+                    captionElement = element;
                     pswp.on("change", () => {
                         const { fileType, alt } = pswp.currSlide.content.data;
-                        captionElement.innerText = alt ?? "";
+                        element.innerText = alt ?? "";
-                        captionElement.style.visibility = alt
+                        element.style.visibility = alt ? "visible" : "hidden";
-                            ? "visible"
-                            : "hidden";
                         // Add extra offset for video captions so that they do
                         // not overlap with the video controls. The constant is
                         // an ad-hoc value that looked okay-ish across browsers.
-                        captionElement.style.bottom =
+                        element.style.bottom =
                             fileType === FileType.video ? "36px" : "0";
                     });
                 },
@@ -708,13 +710,17 @@ export class FileViewerPhotoSwipe {
             const d = 80;
             switch (key) {
                 case "w":
-                    slide.pan.y -= d;
+                    slide.pan.y += d;
+                    break;
                 case "a":
                     slide.pan.x += d;
+                    break;
                 case "s":
-                    slide.pan.x += d;
+                    slide.pan.y -= d;
+                    break;
                 case "d":
-                    slide.pan.y += d;
+                    slide.pan.x -= d;
+                    break;
             }
             slide.panTo(slide.pan.x, slide.pan.y);
         };
@@ -734,17 +740,30 @@ export class FileViewerPhotoSwipe {
             // Even though we ignore shift, Caps lock might still be on.
             const key = e.key.toLowerCase();
 
+            console.log(
+                e,
+                key,
+                e.shiftKey,
+                e.altKey,
+                e.metaKey,
+                e.ctrlKey,
+                e.code,
+            );
+
+            // Keep the keybindings such that they don't use modifiers, because
+            // these are more likely to interfere with browser shortcuts.
+            //
+            // For example, Cmd-D adds a bookmark, which is why we don't use it
+            // for download.
+            //
+            // An exception is Ctrl/Cmd-C, which we intercept to copy the image
+            // since that should match the user's expectation.
+
             let cb: (() => void) | undefined;
-            if (e.shiftKey || e.altKey || e.metaKey) {
+            if (e.shiftKey || e.altKey || e.metaKey || e.ctrlKey) {
-                // ignore
+                // Ignore except using Ctrl/Cmd-D for copy.
-            } else if (e.ctrlKey) {
+                if ((e.metaKey || e.ctrlKey) && key == "c") {
-                switch (key) {
-                    case "d":
-                        cb = handleDownloadIfEnabled;
-                        break;
-                    case "c":
                     cb = handleCopy;
-                        break;
                 }
             } else {
                 switch (key) {
@@ -760,6 +779,13 @@ export class FileViewerPhotoSwipe {
                     case "i":
                         cb = handleViewInfo;
                         break;
+                    case "k":
+                        cb = handleDownloadIfEnabled;
+                        break;
+                    case "Backspace":
+                    case "Delete":
+                        cb = handleDelete;
+                        break;
                     case "f":
                         cb = handleToggleFullscreen;
                         break;