From 5f48504883ba2ab1324973915c645f1b728dcece Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Wed, 1 Feb 2023 13:55:10 +0100 Subject: [PATCH] Add PiP move threshold (#10040) --- .../structures/PictureInPictureDragger.tsx | 13 + .../models/VoiceBroadcastPlayback.ts | 4 + .../PictureInPictureDragger-test.tsx | 59 ++-- .../models/VoiceBroadcastPlayback-test.tsx | 266 +++++++++++------- 4 files changed, 209 insertions(+), 133 deletions(-) diff --git a/src/components/structures/PictureInPictureDragger.tsx b/src/components/structures/PictureInPictureDragger.tsx index 19205229c8a..40c1caee6f9 100644 --- a/src/components/structures/PictureInPictureDragger.tsx +++ b/src/components/structures/PictureInPictureDragger.tsx @@ -70,6 +70,8 @@ export default class PictureInPictureDragger extends React.Component { () => this.animationCallback(), () => requestAnimationFrame(() => this.scheduledUpdate.trigger()), ); + private startingPositionX = 0; + private startingPositionY = 0; private _moving = false; public get moving(): boolean { @@ -192,11 +194,22 @@ export default class PictureInPictureDragger extends React.Component { event.stopPropagation(); this.mouseHeld = true; + this.startingPositionX = event.clientX; + this.startingPositionY = event.clientY; }; private onMoving = (event: MouseEvent): void => { if (!this.mouseHeld) return; + if ( + Math.abs(this.startingPositionX - event.clientX) < 5 && + Math.abs(this.startingPositionY - event.clientY) < 5 + ) { + // User needs to move the widget by at least five pixels. + // Improves click detection when using a touchpad or with nervous hands. + return; + } + event.preventDefault(); event.stopPropagation(); diff --git a/src/voice-broadcast/models/VoiceBroadcastPlayback.ts b/src/voice-broadcast/models/VoiceBroadcastPlayback.ts index 94ccaac1fb2..cba7c6593d5 100644 --- a/src/voice-broadcast/models/VoiceBroadcastPlayback.ts +++ b/src/voice-broadcast/models/VoiceBroadcastPlayback.ts @@ -396,7 +396,11 @@ export class VoiceBroadcastPlayback } if (!this.playbacks.has(eventId)) { + // set to buffering while loading the chunk data + const currentState = this.getState(); + this.setState(VoiceBroadcastPlaybackState.Buffering); await this.loadPlayback(event); + this.setState(currentState); } const playback = this.playbacks.get(eventId); diff --git a/test/components/structures/PictureInPictureDragger-test.tsx b/test/components/structures/PictureInPictureDragger-test.tsx index 9b92fefd797..9f426152879 100644 --- a/test/components/structures/PictureInPictureDragger-test.tsx +++ b/test/components/structures/PictureInPictureDragger-test.tsx @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React from "react"; +import React, { MouseEventHandler } from "react"; import { screen, render, RenderResult } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; @@ -82,28 +82,39 @@ describe("PictureInPictureDragger", () => { }); }); - it("doesn't leak drag events to children as clicks", async () => { - const clickSpy = jest.fn(); - render( - - {[ - ({ onStartMoving }) => ( -
- Hello -
- ), - ]} - , - ); - const target = screen.getByText("Hello"); - - // A click without a drag motion should go through - await userEvent.pointer([{ keys: "[MouseLeft>]", target }, { keys: "[/MouseLeft]" }]); - expect(clickSpy).toHaveBeenCalled(); - - // A drag motion should not trigger a click - clickSpy.mockClear(); - await userEvent.pointer([{ keys: "[MouseLeft>]", target }, { coords: { x: 60, y: 60 } }, "[/MouseLeft]"]); - expect(clickSpy).not.toHaveBeenCalled(); + describe("when rendering the dragger", () => { + let clickSpy: jest.Mocked; + let target: HTMLElement; + + beforeEach(() => { + clickSpy = jest.fn(); + render( + + {[ + ({ onStartMoving }) => ( +
+ Hello +
+ ), + ]} + , + ); + target = screen.getByText("Hello"); + }); + + it("and clicking without a drag motion, it should pass the click to children", async () => { + await userEvent.pointer([{ keys: "[MouseLeft>]", target }, { keys: "[/MouseLeft]" }]); + expect(clickSpy).toHaveBeenCalled(); + }); + + it("and clicking with a drag motion above the threshold of 5px, it should not pass the click to children", async () => { + await userEvent.pointer([{ keys: "[MouseLeft>]", target }, { coords: { x: 60, y: 2 } }, "[/MouseLeft]"]); + expect(clickSpy).not.toHaveBeenCalled(); + }); + + it("and clickign with a drag motion below the threshold of 5px, it should pass the click to the children", async () => { + await userEvent.pointer([{ keys: "[MouseLeft>]", target }, { coords: { x: 4, y: 4 } }, "[/MouseLeft]"]); + expect(clickSpy).toHaveBeenCalled(); + }); }); }); diff --git a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx index ac4217e0485..bf0e4a94f4e 100644 --- a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx +++ b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx @@ -18,6 +18,7 @@ import { mocked } from "jest-mock"; import { screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { MatrixClient, MatrixEvent, MatrixEventEvent, Room } from "matrix-js-sdk/src/matrix"; +import { defer } from "matrix-js-sdk/src/utils"; import { Playback, PlaybackState } from "../../../src/audio/Playback"; import { PlaybackManager } from "../../../src/audio/PlaybackManager"; @@ -31,9 +32,10 @@ import { VoiceBroadcastPlaybackState, VoiceBroadcastRecording, } from "../../../src/voice-broadcast"; -import { filterConsole, flushPromises, stubClient } from "../../test-utils"; +import { filterConsole, flushPromises, flushPromisesWithFakeTimers, stubClient } from "../../test-utils"; import { createTestPlayback } from "../../test-utils/audio"; import { mkVoiceBroadcastChunkEvent, mkVoiceBroadcastInfoStateEvent } from "../utils/test-utils"; +import { LazyValue } from "../../../src/utils/LazyValue"; jest.mock("../../../src/utils/MediaEventHelper", () => ({ MediaEventHelper: jest.fn(), @@ -49,6 +51,7 @@ describe("VoiceBroadcastPlayback", () => { let playback: VoiceBroadcastPlayback; let onStateChanged: (state: VoiceBroadcastPlaybackState) => void; let chunk1Event: MatrixEvent; + let deplayedChunk1Event: MatrixEvent; let chunk2Event: MatrixEvent; let chunk2BEvent: MatrixEvent; let chunk3Event: MatrixEvent; @@ -58,6 +61,7 @@ describe("VoiceBroadcastPlayback", () => { const chunk1Data = new ArrayBuffer(2); const chunk2Data = new ArrayBuffer(3); const chunk3Data = new ArrayBuffer(3); + let delayedChunk1Helper: MediaEventHelper; let chunk1Helper: MediaEventHelper; let chunk2Helper: MediaEventHelper; let chunk3Helper: MediaEventHelper; @@ -97,8 +101,8 @@ describe("VoiceBroadcastPlayback", () => { }; const startPlayback = () => { - beforeEach(async () => { - await playback.start(); + beforeEach(() => { + playback.start(); }); }; @@ -127,11 +131,36 @@ describe("VoiceBroadcastPlayback", () => { }; }; + const mkDeplayedChunkHelper = (data: ArrayBuffer): MediaEventHelper => { + const deferred = defer>(); + + setTimeout(() => { + deferred.resolve({ + // @ts-ignore + arrayBuffer: jest.fn().mockResolvedValue(data), + }); + }, 7500); + + return { + sourceBlob: { + cachedValue: new Blob(), + done: false, + // @ts-ignore + value: deferred.promise, + }, + }; + }; + + const simulateFirstChunkArrived = async (): Promise => { + jest.advanceTimersByTime(10000); + await flushPromisesWithFakeTimers(); + }; + const mkInfoEvent = (state: VoiceBroadcastInfoState) => { return mkVoiceBroadcastInfoStateEvent(roomId, state, userId, deviceId); }; - const mkPlayback = async () => { + const mkPlayback = async (fakeTimers = false): Promise => { const playback = new VoiceBroadcastPlayback( infoEvent, client, @@ -140,7 +169,7 @@ describe("VoiceBroadcastPlayback", () => { jest.spyOn(playback, "removeAllListeners"); jest.spyOn(playback, "destroy"); playback.on(VoiceBroadcastPlaybackEvent.StateChanged, onStateChanged); - await flushPromises(); + fakeTimers ? await flushPromisesWithFakeTimers() : await flushPromises(); return playback; }; @@ -152,6 +181,7 @@ describe("VoiceBroadcastPlayback", () => { const createChunkEvents = () => { chunk1Event = mkVoiceBroadcastChunkEvent(infoEvent.getId()!, userId, roomId, chunk1Length, 1); + deplayedChunk1Event = mkVoiceBroadcastChunkEvent(infoEvent.getId()!, userId, roomId, chunk1Length, 1); chunk2Event = mkVoiceBroadcastChunkEvent(infoEvent.getId()!, userId, roomId, chunk2Length, 2); chunk2Event.setTxnId("tx-id-1"); chunk2BEvent = mkVoiceBroadcastChunkEvent(infoEvent.getId()!, userId, roomId, chunk2Length, 2); @@ -159,6 +189,7 @@ describe("VoiceBroadcastPlayback", () => { chunk3Event = mkVoiceBroadcastChunkEvent(infoEvent.getId()!, userId, roomId, chunk3Length, 3); chunk1Helper = mkChunkHelper(chunk1Data); + delayedChunk1Helper = mkDeplayedChunkHelper(chunk1Data); chunk2Helper = mkChunkHelper(chunk2Data); chunk3Helper = mkChunkHelper(chunk3Data); @@ -181,6 +212,7 @@ describe("VoiceBroadcastPlayback", () => { mocked(MediaEventHelper).mockImplementation((event: MatrixEvent): any => { if (event === chunk1Event) return chunk1Helper; + if (event === deplayedChunk1Event) return delayedChunk1Helper; if (event === chunk2Event) return chunk2Helper; if (event === chunk3Event) return chunk3Helper; }); @@ -488,11 +520,17 @@ describe("VoiceBroadcastPlayback", () => { describe("when there is a stopped voice broadcast", () => { beforeEach(async () => { + jest.useFakeTimers(); infoEvent = mkInfoEvent(VoiceBroadcastInfoState.Stopped); createChunkEvents(); - setUpChunkEvents([chunk2Event, chunk1Event, chunk3Event]); - room.addLiveEvents([infoEvent, chunk1Event, chunk2Event, chunk3Event]); - playback = await mkPlayback(); + // use delayed first chunk here to simulate loading time + setUpChunkEvents([chunk2Event, deplayedChunk1Event, chunk3Event]); + room.addLiveEvents([infoEvent, deplayedChunk1Event, chunk2Event, chunk3Event]); + playback = await mkPlayback(true); + }); + + afterEach(() => { + jest.useRealTimers(); }); it("should expose the info event", () => { @@ -504,152 +542,161 @@ describe("VoiceBroadcastPlayback", () => { describe("and calling start", () => { startPlayback(); - itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Playing); - - it("should play the chunks beginning with the first one", () => { - // assert that the first chunk is being played - expect(chunk1Playback.play).toHaveBeenCalled(); - expect(chunk2Playback.play).not.toHaveBeenCalled(); - }); + itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Buffering); - describe("and calling start again", () => { - it("should not play the first chunk a second time", () => { - expect(chunk1Playback.play).toHaveBeenCalledTimes(1); + describe("and the first chunk data has been loaded", () => { + beforeEach(async () => { + await simulateFirstChunkArrived(); }); - }); - describe("and the chunk playback progresses", () => { - beforeEach(() => { - chunk1Playback.clockInfo.liveData.update([11]); - }); + itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Playing); - it("should update the time", () => { - expect(playback.timeSeconds).toBe(11); + it("should play the chunks beginning with the first one", () => { + // assert that the first chunk is being played + expect(chunk1Playback.play).toHaveBeenCalled(); + expect(chunk2Playback.play).not.toHaveBeenCalled(); }); - }); - - describe("and skipping to the middle of the second chunk", () => { - const middleOfSecondChunk = (chunk1Length + chunk2Length / 2) / 1000; - beforeEach(async () => { - await playback.skipTo(middleOfSecondChunk); + describe("and calling start again", () => { + it("should not play the first chunk a second time", () => { + expect(chunk1Playback.play).toHaveBeenCalledTimes(1); + }); }); - it("should play the second chunk", () => { - expect(chunk1Playback.stop).toHaveBeenCalled(); - expect(chunk1Playback.destroy).toHaveBeenCalled(); - expect(chunk2Playback.play).toHaveBeenCalled(); - }); + describe("and the chunk playback progresses", () => { + beforeEach(() => { + chunk1Playback.clockInfo.liveData.update([11]); + }); - it("should update the time", () => { - expect(playback.timeSeconds).toBe(middleOfSecondChunk); + it("should update the time", () => { + expect(playback.timeSeconds).toBe(11); + }); }); - describe("and skipping to the start", () => { + describe("and skipping to the middle of the second chunk", () => { + const middleOfSecondChunk = (chunk1Length + chunk2Length / 2) / 1000; + beforeEach(async () => { - await playback.skipTo(0); + await playback.skipTo(middleOfSecondChunk); }); - it("should play the first chunk", () => { - expect(chunk2Playback.stop).toHaveBeenCalled(); - expect(chunk2Playback.destroy).toHaveBeenCalled(); - expect(chunk1Playback.play).toHaveBeenCalled(); + it("should play the second chunk", () => { + expect(chunk1Playback.stop).toHaveBeenCalled(); + expect(chunk1Playback.destroy).toHaveBeenCalled(); + expect(chunk2Playback.play).toHaveBeenCalled(); }); it("should update the time", () => { - expect(playback.timeSeconds).toBe(0); + expect(playback.timeSeconds).toBe(middleOfSecondChunk); }); - }); - }); - describe("and skipping multiple times", () => { - beforeEach(async () => { - return Promise.all([ - playback.skipTo(middleOfSecondChunk), - playback.skipTo(middleOfThirdChunk), - playback.skipTo(0), - ]); + describe("and skipping to the start", () => { + beforeEach(async () => { + await playback.skipTo(0); + }); + + it("should play the first chunk", () => { + expect(chunk2Playback.stop).toHaveBeenCalled(); + expect(chunk2Playback.destroy).toHaveBeenCalled(); + expect(chunk1Playback.play).toHaveBeenCalled(); + }); + + it("should update the time", () => { + expect(playback.timeSeconds).toBe(0); + }); + }); }); - it("should only skip to the first and last position", () => { - expect(chunk1Playback.stop).toHaveBeenCalled(); - expect(chunk1Playback.destroy).toHaveBeenCalled(); - expect(chunk2Playback.play).toHaveBeenCalled(); + describe("and skipping multiple times", () => { + beforeEach(async () => { + return Promise.all([ + playback.skipTo(middleOfSecondChunk), + playback.skipTo(middleOfThirdChunk), + playback.skipTo(0), + ]); + }); - expect(chunk3Playback.play).not.toHaveBeenCalled(); + it("should only skip to the first and last position", () => { + expect(chunk1Playback.stop).toHaveBeenCalled(); + expect(chunk1Playback.destroy).toHaveBeenCalled(); + expect(chunk2Playback.play).toHaveBeenCalled(); - expect(chunk2Playback.stop).toHaveBeenCalled(); - expect(chunk2Playback.destroy).toHaveBeenCalled(); - expect(chunk1Playback.play).toHaveBeenCalled(); - }); - }); + expect(chunk3Playback.play).not.toHaveBeenCalled(); - describe("and the first chunk ends", () => { - beforeEach(() => { - chunk1Playback.emit(PlaybackState.Stopped); + expect(chunk2Playback.stop).toHaveBeenCalled(); + expect(chunk2Playback.destroy).toHaveBeenCalled(); + expect(chunk1Playback.play).toHaveBeenCalled(); + }); }); - it("should play until the end", () => { - // assert first chunk was unloaded - expect(chunk1Playback.destroy).toHaveBeenCalled(); - - // assert that the second chunk is being played - expect(chunk2Playback.play).toHaveBeenCalled(); + describe("and the first chunk ends", () => { + beforeEach(() => { + chunk1Playback.emit(PlaybackState.Stopped); + }); - // simulate end of second and third chunk - chunk2Playback.emit(PlaybackState.Stopped); - chunk3Playback.emit(PlaybackState.Stopped); + it("should play until the end", () => { + // assert first chunk was unloaded + expect(chunk1Playback.destroy).toHaveBeenCalled(); - // assert that the entire playback is now in stopped state - expect(playback.getState()).toBe(VoiceBroadcastPlaybackState.Stopped); - }); - }); + // assert that the second chunk is being played + expect(chunk2Playback.play).toHaveBeenCalled(); - describe("and calling pause", () => { - pausePlayback(); - itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Paused); - itShouldEmitAStateChangedEvent(VoiceBroadcastPlaybackState.Paused); - }); + // simulate end of second and third chunk + chunk2Playback.emit(PlaybackState.Stopped); + chunk3Playback.emit(PlaybackState.Stopped); - describe("and calling stop", () => { - stopPlayback(); - itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Stopped); + // assert that the entire playback is now in stopped state + expect(playback.getState()).toBe(VoiceBroadcastPlaybackState.Stopped); + }); + }); - it("should stop the playback", () => { - expect(chunk1Playback.stop).toHaveBeenCalled(); + describe("and calling pause", () => { + pausePlayback(); + itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Paused); + itShouldEmitAStateChangedEvent(VoiceBroadcastPlaybackState.Paused); }); - describe("and skipping to somewhere in the middle of the first chunk", () => { - beforeEach(async () => { - mocked(chunk1Playback.play).mockClear(); - await playback.skipTo(1); + describe("and calling stop", () => { + stopPlayback(); + itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Stopped); + + it("should stop the playback", () => { + expect(chunk1Playback.stop).toHaveBeenCalled(); }); - it("should not start the playback", () => { - expect(chunk1Playback.play).not.toHaveBeenCalled(); + describe("and skipping to somewhere in the middle of the first chunk", () => { + beforeEach(async () => { + mocked(chunk1Playback.play).mockClear(); + await playback.skipTo(1); + }); + + it("should not start the playback", () => { + expect(chunk1Playback.play).not.toHaveBeenCalled(); + }); }); }); - }); - describe("and calling destroy", () => { - beforeEach(() => { - playback.destroy(); - }); + describe("and calling destroy", () => { + beforeEach(() => { + playback.destroy(); + }); - it("should call removeAllListeners", () => { - expect(playback.removeAllListeners).toHaveBeenCalled(); - }); + it("should call removeAllListeners", () => { + expect(playback.removeAllListeners).toHaveBeenCalled(); + }); - it("should call destroy on the playbacks", () => { - expect(chunk1Playback.destroy).toHaveBeenCalled(); - expect(chunk2Playback.destroy).toHaveBeenCalled(); + it("should call destroy on the playbacks", () => { + expect(chunk1Playback.destroy).toHaveBeenCalled(); + expect(chunk2Playback.destroy).toHaveBeenCalled(); + }); }); }); }); describe("and calling toggle for the first time", () => { beforeEach(async () => { - await playback.toggle(); + playback.toggle(); + await simulateFirstChunkArrived(); }); itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Playing); @@ -679,7 +726,8 @@ describe("VoiceBroadcastPlayback", () => { describe("and calling toggle", () => { beforeEach(async () => { mocked(onStateChanged).mockReset(); - await playback.toggle(); + playback.toggle(); + await simulateFirstChunkArrived(); }); itShouldSetTheStateTo(VoiceBroadcastPlaybackState.Playing);