diff --git a/web/apps/photos/src/styles/photoswipe.css b/web/apps/photos/src/styles/photoswipe.css index d2276b61c9..af8fe69137 100644 --- a/web/apps/photos/src/styles/photoswipe.css +++ b/web/apps/photos/src/styles/photoswipe.css @@ -155,23 +155,24 @@ bottom: 0px; right: 0; margin: 20px 24px; - padding: 6px 16px; - border-radius: 2px; + padding-inline: 16px; + border-radius: 3px; /* Same opacity as the other controls. */ color: rgb(255 255 255 / 0.85); background-color: rgb(0 0 0 / 0.2); backdrop-filter: blur(10px); - /* 4 lines max, ellipsis on overflow. */ - word-break: break-word; text-align: right; max-width: 375px; max-height: 200px; - overflow: hidden; - text-overflow: ellipsis; - display: -webkit-box; - -webkit-box-orient: vertical; - -webkit-line-clamp: 4; - line-clamp: 4; + p { + /* 4 lines max, ellipsis on overflow. */ + word-break: break-word; + overflow: hidden; + display: -webkit-box; + -webkit-box-orient: vertical; + -webkit-line-clamp: 4; + line-clamp: 4; + } } /* Change the cursor on the thumbnail to the default arrow to indicate that it is @@ -221,10 +222,14 @@ [Note: Showing menus outside of media-controller] - This is all very hacky. None of the media controller examples currently - show menus work for our specific scenario (the controls detached from the - the media controller itself, and shown at the bottom of the screen). As - such, a lot of undocumented and arbitrary tweaks are needed. This is a + This is all very hacky. The standalone control example + (https://media-chrome.mux.dev/examples/vanilla/standalone-controls.html), + doesn't (as of now) include a menu, so it is possible this is just not + supported. None of the media controller examples I could find show menus + work for our specific scenario (the controls detached from the the media + controller itself, and shown at the bottom of the screen). + + As such, a lot of undocumented and arbitrary tweaks are needed. This is a house of cards, and might fall when the media-chrome version is updated; on the other hand, they might also add better support for our scenario, in which case this should be possible straightforwardly. @@ -281,7 +286,6 @@ media-time-range:hover { they trigger. */ media-pip-button[mediapipunavailable], -media-airplay-button[mediaairplayunavailable], media-fullscreen-button[mediafullscreenunavailable] { display: none; } @@ -302,3 +306,15 @@ media-settings-menu { */ z-index: 1; } + +/* Hide the caption when hovering over the video controls since they occupy + similar real estate */ +body:has( + media-control-bar:hover, + media-settings-menu:hover, + media-settings-menu:focus-within + ) { + & .pswp-ente .pswp__caption { + display: none; + } +} diff --git a/web/packages/base/log-web.ts b/web/packages/base/log-web.ts index 12801ab08c..be63612875 100644 --- a/web/packages/base/log-web.ts +++ b/web/packages/base/log-web.ts @@ -31,6 +31,19 @@ export const logStartupBanner = (userID?: number) => { */ export const logUnhandledErrorsAndRejections = (attach: boolean) => { const handleError = (event: ErrorEvent) => { + // [Note: Spurious media chrome resize observer errors] + // + // When attaching media chrome controls to the DOM, we get an (AFAICT) + // spurious error in the log. Ignore it. FWIW, the media control tests + // themselves do the same. + // https://github.com/muxinc/elements/blob/f602519f544509f15add8fcc8cbbf7379843dcd3/packages/mux-player/test/player.test.js#L6-L12C5 + if ( + event.message == + "ResizeObserver loop completed with undelivered notifications." + ) { + return; + } + log.error("Unhandled error", event.error ?? event.message); }; diff --git a/web/packages/gallery/components/viewer/photoswipe.ts b/web/packages/gallery/components/viewer/photoswipe.ts index 37e1fcbff4..19fbb14420 100644 --- a/web/packages/gallery/components/viewer/photoswipe.ts +++ b/web/packages/gallery/components/viewer/photoswipe.ts @@ -470,9 +470,9 @@ export class FileViewerPhotoSwipe { const { fileID, fileType, videoURL, mediaControllerID } = asItemData(e.content.data); - // For the initial slide, "contentAppend" will get called after - // "change", so we need to wire up the controls (or hide them) for - // the initial slide here also (in addition to in "change"). + // For the initial slide, "contentAppend" will get called after the + // "change" event, so we need to wire up the controls, or hide them, + // for the initial slide here also (in addition to in "change"). if (currSlideData().fileID == fileID) { // For reasons possibily related to the 1 tick waits in the // hls-video implementation (`await Promise.resolve()`), the @@ -483,11 +483,11 @@ export class FileViewerPhotoSwipe { // See also: https://github.com/muxinc/media-chrome/issues/940 // // As a workaround, defer the association to the next tick. - // setTimeout(() => updateMediaControls(mediaControllerID), 0); } // Rest of this function deals with live photos. + if (fileType != FileType.livePhoto) return; if (!videoURL) return; @@ -544,6 +544,24 @@ export class FileViewerPhotoSwipe { video.style.height = `${height}px`; }); + /** + * Get the video element, if any, that is a descendant of the given HTML + * element. + * + * While the return type is an {@link HTMLVideoElement}, the result can + * also be an instance of a media-chrome `CustomVideoElement`, + * specifically a {@link HlsVideoElement}. + * https://github.com/muxinc/media-elements/blob/main/packages/hls-video-element/hls-video-element.js + * + * The media-chrome `CustomVideoElement`s provide the same API as the + * browser's built-in {@link HTMLVideoElement}s, so we can use the same + * methods on them. + * + * For ergonomic use at call sites, it accepts an optional. + */ + const queryVideoElement = (element: HTMLElement | undefined) => + element?.querySelector("video, hls-video"); + pswp.on("contentDeactivate", (e) => { // Reset failures, if any, for this file so that the fetch is tried // again when we come back to it^. @@ -558,8 +576,7 @@ export class FileViewerPhotoSwipe { // Pause the video element, if any, when we move away from the // slide. - const video = - e.content.slide?.container.getElementsByTagName("video")[0]; + const video = queryVideoElement(e.content.slide?.container); video?.pause(); }); @@ -576,9 +593,26 @@ export class FileViewerPhotoSwipe { forgetExifForItemData(asItemData(e.content.data)), ); + /** + * The media-chrome-button elements (e.g the play button) retain focus + * after clicking on them. e.g., if I click the "media-mute-button" to + * activate it, then later press Space or Enter, then the mute button + * activates again instead of toggling video playback. + * + * I'm not sure who is at fault here, but this behaviour ends up being + * irritating. To prevent this from happening, drop the focus from any + * media chrome button when playback starts. + */ + const resetFocus = () => { + const activeElement = document.activeElement; + if (activeElement instanceof HTMLElement) activeElement.blur(); + }; + /** * If the current slide is showing a video, then the DOM video element * showing that video. + * + * See also {@link queryVideoElement}. */ let videoVideoEl: HTMLVideoElement | undefined; @@ -633,11 +667,13 @@ export class FileViewerPhotoSwipe { // It works subsequently, which is why, e.g., we can use it to // pause the video in "contentDeactivate". const contentElement = pswp.currSlide?.content.element; - videoVideoEl = contentElement?.getElementsByTagName("video")[0]; + videoVideoEl = queryVideoElement(contentElement) ?? undefined; if (videoVideoEl) { - onVideoPlayback = () => + onVideoPlayback = () => { + resetFocus(); showIf(captionElement!, !!videoVideoEl?.paused); + }; videoVideoEl.addEventListener("play", onVideoPlayback); videoVideoEl.addEventListener("pause", onVideoPlayback); @@ -890,31 +926,10 @@ export class FileViewerPhotoSwipe { }, }); - ui.registerElement({ - name: "caption", - // Arbitrary order towards the end (it doesn't matter anyways - // since we're absolutely positioned). - order: 30, - appendTo: "root", - tagName: "p", - onInit: (element, pswp) => { - captionElement = element; - pswp.on("change", () => { - const { fileType, alt } = currSlideData(); - element.innerText = alt ?? ""; - element.style.visibility = alt ? "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. - element.style.bottom = - fileType === FileType.video ? "36px" : "0"; - }); - }, - }); - ui.registerElement({ name: "media-controls", - order: 31, + // Arbitrary order towards the end. + order: 30, appendTo: "root", html: hlsVideoControlsHTML(), onInit: (element, pswp) => { @@ -925,6 +940,35 @@ export class FileViewerPhotoSwipe { }); }, }); + + ui.registerElement({ + name: "caption", + // After the video controls so that we don't get occluded by + // them (nb: the caption will hide when the video is playing). + order: 31, + appendTo: "root", + // The caption uses the line-clamp CSS property, which behaves + // unexpectedly when we also assign padding to the "p" element + // on which we're setting the line clamp: the "clipped" lines + // show through in the padding area. + // + // As a workaround, wrap the p in a div. Set the line-clamp on + // the p, and the padding on the div. + html: "

", + onInit: (element, pswp) => { + captionElement = element; + pswp.on("change", () => { + const { fileType, alt } = currSlideData(); + element.querySelector("p")!.innerText = alt ?? ""; + element.style.visibility = alt ? "visible" : "hidden"; + // Add extra offset for video captions so that they do + // not overlap with the video controls. The constant is + // such that it lies above the media controls. + element.style.bottom = + fileType === FileType.video ? "44px" : "0"; + }); + }, + }); }); // Pan action handlers @@ -951,14 +995,44 @@ export class FileViewerPhotoSwipe { // Actions we handle ourselves. + const handlePreviousSlide = () => pswp.prev(); + + const handleNextSlide = () => pswp.next(); + + const handleSeekBackOrPreviousSlide = () => { + // TODO(HLS): Behind temporary flag + // const vid = videoVideoEl; + const vid = process.env.NEXT_PUBLIC_ENTE_WIP_VIDEO_STREAMING + ? videoVideoEl + : undefined; + if (vid) { + vid.currentTime = Math.max(vid.currentTime - 5, 0); + } else { + handlePreviousSlide(); + } + }; + + const handleSeekForwardOrNextSlide = () => { + // TODO(HLS): Behind temporary flag + // const vid = videoVideoEl; + const vid = process.env.NEXT_PUBLIC_ENTE_WIP_VIDEO_STREAMING + ? videoVideoEl + : undefined; + if (vid) { + vid.currentTime = vid.currentTime + 5; + } else { + handleNextSlide(); + } + }; + const handleTogglePlayIfPossible = () => { switch (currentAnnotatedFile().itemData.fileType) { case FileType.video: videoTogglePlayIfPossible(); - return; + break; case FileType.livePhoto: livePhotoTogglePlayIfPossible(); - return; + break; } }; @@ -966,10 +1040,10 @@ export class FileViewerPhotoSwipe { switch (currentAnnotatedFile().itemData.fileType) { case FileType.video: videoToggleMuteIfPossible(); - return; + break; case FileType.livePhoto: livePhotoToggleMuteIfPossible(); - return; + break; } }; @@ -1021,15 +1095,25 @@ export class FileViewerPhotoSwipe { // 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. + // There are some exception, e.g. 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) { // Ignore except "?" for help. if (key == "?") cb = handleHelp; } else if (e.altKey) { - // Ignore. + // Ignore except if for arrow keys since when showing a video, + // the arrow keys are used for seeking, and the normal arrow key + // function (slide movement) needs the Alt/Opt modifier. + switch (key) { + case "ArrowLeft": + cb = handlePreviousSlide; + break; + case "ArrowRight": + cb = handleNextSlide; + break; + } } else if (e.metaKey || e.ctrlKey) { // Ignore except Ctrl/Cmd-C for copy if (lkey == "c") cb = handleCopy; @@ -1046,6 +1130,16 @@ export class FileViewerPhotoSwipe { case "Delete": cb = handleDelete; break; + case "ArrowLeft": + cb = handleSeekBackOrPreviousSlide; + // Prevent PhotoSwipe's default handling of this key. + pswpEvent.preventDefault(); + break; + case "ArrowRight": + cb = handleSeekForwardOrNextSlide; + // Prevent PhotoSwipe's default handling of this key. + pswpEvent.preventDefault(); + break; // We check for "?"" both with an without shift, since some // keyboards might have it emittable without shift. case "?": @@ -1151,9 +1245,8 @@ const videoHTML = (url: string, disableDownload: boolean) => ` // import "media-chrome"; // import "media-chrome/menu"; // -// TODO(HLS): Update code above that searches for the video element const hlsVideoHTML = (url: string, mediaControllerID: string) => ` - + `; @@ -1164,18 +1257,26 @@ const hlsVideoHTML = (url: string, mediaControllerID: string) => ` * To make these functional, the `media-control-bar` requires the * `mediacontroller="${mediaControllerID}"` attribute. * - * TODO(HLS): Tooltips get clipped - * TODO(HLS): Translation - * TODO(HLS): Spurious console warning - * * Notes: * * - Examples: https://media-chrome.mux.dev/examples/vanilla/ * * - When PiP is active and the video moves out, the browser displays some - * indicator (browser specific) in the in-page video element. + * indicator (browser specific) in the in-page video element. This element and + * text is not under our control. * * - The media-cast-button currently doesn't work with the `hls-video` player. + * + * - Media chrome has mechanism for statically providing translations but it + * wasn't working when I tried with 4.9.0. The media chrome tooltips also get + * clipped for the cornermost buttons. Finally, the rest of the buttons on + * this screen don't have a tooltip either. + * + * Revisit this when we have a custom tooltip element we can then also use on + * this screen, which can also be used enhancement for the other buttons on + * this screen which use "title" (which get clipped when they are multi-word). + * + * - See: [Note: Spurious media chrome resize observer errors] */ const hlsVideoControlsHTML = () => `
@@ -1194,16 +1295,15 @@ const hlsVideoControlsHTML = () => ` - - + + ${settingsSVGPath} - - - + +
`; diff --git a/web/packages/gallery/package.json b/web/packages/gallery/package.json index c9a87b6ba3..c7a9ba0a4f 100644 --- a/web/packages/gallery/package.json +++ b/web/packages/gallery/package.json @@ -11,7 +11,7 @@ "hls-video-element": "^1.5.0", "leaflet": "^1.9.4", "leaflet-defaulticon-compatibility": "^0.1.2", - "media-chrome": "^4.8.0", + "media-chrome": "^4.9.0", "photoswipe": "^5.4.4", "react-dropzone": "14.2.10" }, diff --git a/web/yarn.lock b/web/yarn.lock index 997ce4d01f..50abd1b02e 100644 --- a/web/yarn.lock +++ b/web/yarn.lock @@ -2884,10 +2884,10 @@ math-intrinsics@^1.1.0: resolved "https://registry.yarnpkg.com/math-intrinsics/-/math-intrinsics-1.1.0.tgz#a0dd74be81e2aa5c2f27e65ce283605ee4e2b7f9" integrity sha512-/IXtbwEk5HTPyEwyKX6hGkYXxM9nbj64B+ilVJnC/R6B0pH5G4V3b0pVbL7DBj4tkhBAppbQUlf6F6Xl9LHu1g== -media-chrome@^4.8.0: - version "4.8.0" - resolved "https://registry.yarnpkg.com/media-chrome/-/media-chrome-4.8.0.tgz#ff881a5466fe02ad07344a370a8a1106988ca245" - integrity sha512-oioEGlluW+1RqknqsszrKHDs3NZ9AaatEaE2kYYOSWxnwvVmhRTfDWT4JeMgtUr5r3i2dAI3e/qbeb1j+a0MhA== +media-chrome@^4.9.0: + version "4.9.0" + resolved "https://registry.yarnpkg.com/media-chrome/-/media-chrome-4.9.0.tgz#05f692781234dfb02baf1bdbc578af62b7edeeb7" + integrity sha512-KSGZUDEt0vg0Ogx+B8YYj0O32l0ppPLR8s5rXBMOEpkLYbJrKfhSPfwTmjJGcp2Nf3F/X2ddwO96YvUXHYeYvw== dependencies: ce-la-react "^0.1.3"