From 513477d28039db9788f042a6f5e91b157faeca00 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 9 Mar 2026 09:45:25 +0100 Subject: [PATCH] review: Use targetWidth/Height instead of listening to element bounds --- src/grid/TileWrapper.tsx | 6 ++++++ src/state/media/UserMediaViewModel.ts | 18 ++++++++++-------- src/tile/GridTile.test.tsx | 18 +----------------- src/tile/GridTile.tsx | 20 ++++++++------------ src/tile/SpotlightTile.test.tsx | 18 +----------------- src/tile/SpotlightTile.tsx | 22 +++++++++++----------- 6 files changed, 37 insertions(+), 65 deletions(-) diff --git a/src/grid/TileWrapper.tsx b/src/grid/TileWrapper.tsx index 1bed08da..00689a78 100644 --- a/src/grid/TileWrapper.tsx +++ b/src/grid/TileWrapper.tsx @@ -27,7 +27,13 @@ interface Props { state: Parameters>[0], ) => void > | null; + /** + * The width this tile will have once its animations have settled. + */ targetWidth: number; + /** + * The width this tile will have once its animations have settled. + */ targetHeight: number; model: M; Tile: ComponentType>; diff --git a/src/state/media/UserMediaViewModel.ts b/src/state/media/UserMediaViewModel.ts index 1b9f6cbb..16af7f26 100644 --- a/src/state/media/UserMediaViewModel.ts +++ b/src/state/media/UserMediaViewModel.ts @@ -62,12 +62,12 @@ export interface BaseUserMediaViewModel extends MemberMediaViewModel { RTCInboundRtpStreamStats | RTCOutboundRtpStreamStats | undefined >; /** - * Set the actual dimensions of the HTML element. + * Set the target dimensions of the HTML element (final dimension after anim). * This can be used to determine the best video fit (fit to frame / keep ratio). - * @param width - The actual width of the HTML element displaying the video. - * @param height - The actual height of the HTML element displaying the video. + * @param targetWidth - The target width of the HTML element displaying the video. + * @param targetHeight - The target height of the HTML element displaying the video. */ - setActualDimensions: (width: number, height: number) => void; + setTargetDimensions: (targetWidth: number, targetHeight: number) => void; } export interface BaseUserMediaInputs extends Omit< @@ -98,7 +98,9 @@ export function createBaseUserMedia( ); const toggleCropVideo$ = new Subject(); - const actualSize$ = new BehaviorSubject< + // The target size of the video element, used to determine the best video fit. + // The target size is the final size of the HTML element after any animations have completed. + const targetSize$ = new BehaviorSubject< { width: number; height: number } | undefined >(undefined); @@ -130,7 +132,7 @@ export function createBaseUserMedia( videoFit$: videoFit$( scope, videoSizeFromParticipant$(participant$), - actualSize$, + targetSize$, ), toggleCropVideo: () => toggleCropVideo$.next(), rtcBackendIdentity, @@ -155,8 +157,8 @@ export function createBaseUserMedia( return observeRtpStreamStats$(p, Track.Source.Camera, statsType); }), ), - setActualDimensions: (width: number, height: number): void => { - actualSize$.next({ width, height }); + setTargetDimensions: (targetWidth: number, targetHeight: number): void => { + targetSize$.next({ width: targetWidth, height: targetHeight }); }, }; } diff --git a/src/tile/GridTile.test.tsx b/src/tile/GridTile.test.tsx index 2c6e0d15..02f09a17 100644 --- a/src/tile/GridTile.test.tsx +++ b/src/tile/GridTile.test.tsx @@ -6,7 +6,7 @@ Please see LICENSE in the repository root for full details. */ import { type RemoteTrackPublication } from "livekit-client"; -import { test, expect, beforeAll } from "vitest"; +import { test, expect } from "vitest"; import { render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; import { type MatrixRTCSession } from "matrix-js-sdk/lib/matrixrtc"; @@ -28,22 +28,6 @@ global.IntersectionObserver = class MockIntersectionObserver { public disconnect(): void {} } as unknown as typeof IntersectionObserver; -// Mock ResizeObserver as it is needed by the useMeasure hook used in the GridTile, but is not implemented in JSDOM. -// We just need to mock it with empty methods as we don't need to test its functionality here. -beforeAll(() => { - window.ResizeObserver = class ResizeObserver { - public observe(): void { - // do nothing - } - public unobserve(): void { - // do nothing - } - public disconnect(): void { - // do nothing - } - }; -}); - test("GridTile is accessible", async () => { const vm = mockRemoteMedia( mockRtcMembership("@alice:example.org", "AAAA"), diff --git a/src/tile/GridTile.tsx b/src/tile/GridTile.tsx index a9ae188b..c8052a65 100644 --- a/src/tile/GridTile.tsx +++ b/src/tile/GridTile.tsx @@ -37,7 +37,6 @@ import { Menu, } from "@vector-im/compound-web"; import { useObservableEagerState } from "observable-hooks"; -import useMeasure from "react-use-measure"; import styles from "./GridTile.module.css"; import { Slider } from "../Slider"; @@ -88,6 +87,8 @@ const UserMediaTile: FC = ({ displayName, mxcAvatarUrl, focusable, + targetWidth, + targetHeight, ...props }) => { const { toggleRaisedHand } = useReactionsSender(); @@ -110,19 +111,12 @@ const UserMediaTile: FC = ({ const handRaised = useBehavior(vm.handRaised$); const reaction = useBehavior(vm.reaction$); - // We need to keep track of the tile size. - // We use this to get the tile ratio, and compare it to the video ratio to decide - // whether to fit the video to frame or keep the ratio. - const [measureRef, bounds] = useMeasure(); - // There is already a ref being passed in, so we need to merge it with the measureRef. - const tileRef = useMergedRefs(ref, measureRef); - // Whenever bounds change, inform the viewModel useEffect(() => { - if (bounds.width > 0 && bounds.height > 0) { - vm.setActualDimensions(bounds.width, bounds.height); + if (targetWidth > 0 && targetHeight > 0) { + vm.setTargetDimensions(targetWidth, targetHeight); } - }, [bounds.width, bounds.height, vm]); + }, [targetWidth, targetHeight, vm]); const AudioIcon = playbackMuted ? VolumeOffSolidIcon @@ -155,7 +149,7 @@ const UserMediaTile: FC = ({ const tile = ( = ({ audioStreamStats={audioStreamStats} videoStreamStats={videoStreamStats} rtcBackendIdentity={rtcBackendIdentity} + targetWidth={targetWidth} + targetHeight={targetHeight} {...props} /> ); diff --git a/src/tile/SpotlightTile.test.tsx b/src/tile/SpotlightTile.test.tsx index 6fa26695..a5332194 100644 --- a/src/tile/SpotlightTile.test.tsx +++ b/src/tile/SpotlightTile.test.tsx @@ -5,7 +5,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { test, expect, vi, beforeAll } from "vitest"; +import { test, expect, vi } from "vitest"; import { isInaccessible, render, screen } from "@testing-library/react"; import { axe } from "vitest-axe"; import userEvent from "@testing-library/user-event"; @@ -27,22 +27,6 @@ global.IntersectionObserver = class MockIntersectionObserver { public unobserve(): void {} } as unknown as typeof IntersectionObserver; -// Mock ResizeObserver as it is needed by the useMeasure hook used in the SpotlightTile, but is not implemented in JSDOM. -// We just need to mock it with empty methods as we don't need to test its functionality here. -beforeAll(() => { - window.ResizeObserver = class ResizeObserver { - public observe(): void { - // do nothing - } - public unobserve(): void { - // do nothing - } - public disconnect(): void { - // do nothing - } - }; -}); - test("SpotlightTile is accessible", async () => { const vm1 = mockRemoteMedia( mockRtcMembership("@alice:example.org", "AAAA"), diff --git a/src/tile/SpotlightTile.tsx b/src/tile/SpotlightTile.tsx index a0b1309b..16a8679d 100644 --- a/src/tile/SpotlightTile.tsx +++ b/src/tile/SpotlightTile.tsx @@ -27,7 +27,6 @@ import { useObservableRef } from "observable-hooks"; import { useTranslation } from "react-i18next"; import classNames from "classnames"; import { type TrackReferenceOrPlaceholder } from "@livekit/components-core"; -import useMeasure from "react-use-measure"; import FullScreenMaximiseIcon from "../icons/FullScreenMaximise.svg?react"; import FullScreenMinimiseIcon from "../icons/FullScreenMinimise.svg?react"; @@ -152,7 +151,13 @@ const SpotlightRemoteScreenShareItem: FC< interface SpotlightItemProps { ref?: Ref; vm: MediaViewModel; + /** + * The width this tile will have once its animations have settled. + */ targetWidth: number; + /** + * The height this tile will have once its animations have settled. + */ targetHeight: number; focusable: boolean; intersectionObserver$: Observable; @@ -175,21 +180,16 @@ const SpotlightItem: FC = ({ }) => { const ourRef = useRef(null); - // We need to keep track of the tile size. - // We use this to get the tile ratio, and compare it to the video ratio to decide - // whether to fit the video to frame or keep the ratio. - const [measureRef, bounds] = useMeasure(); - - // Whenever bounds change, inform the viewModel + // Whenever target bounds change, inform the viewModel useEffect(() => { - if (bounds.width > 0 && bounds.height > 0) { + if (targetWidth > 0 && targetHeight > 0) { if (vm.type != "screen share") { - vm.setActualDimensions(bounds.width, bounds.height); + vm.setTargetDimensions(targetWidth, targetHeight); } } - }, [bounds.width, bounds.height, vm]); + }, [targetWidth, targetHeight, vm]); - const ref = useMergedRefs(ourRef, theirRef, measureRef); + const ref = useMergedRefs(ourRef, theirRef); const focusUrl = useBehavior(vm.focusUrl$); const displayName = useBehavior(vm.displayName$); const mxcAvatarUrl = useBehavior(vm.mxcAvatarUrl$);