Skip to content

Commit b4c1ffd

Browse files
authored
fix: Scrub on windows (#4847)
## Description https://discord.com/channels/955905230107738152/1337534586707644416 There are other bugs I see on Windows on my machine - `requestPointerLock` seems too buggy. 1. In my case, wrap-around is not working. 2. Fast clicks and movements can cause `pointerup` to not be intercepted (because of the fix above). Here’s how I see the issue: - `requestPointerLock` is asynchronous. - During `requestPointerLock`, we might get a `lostpointercapture` event. - This has a very low probability of happening on Mac. - On Windows, the probability is very high _(it can be caused by calling `requestPointerLock` itself, depending on the machine)_. - On Mac, if this happens, the `pointerup` event will most likely be lost. - On Windows, the `pointerup` event is usually preserved but can be lost as well. Currently, on Windows, instead of cleanup, we assume everything will be fine. On Mac, we perform cleanup on the `lostpointercapture` event. Windows' behavior is buggy, but I haven’t found a proper way to fix it. There's no way to detect `event.buttons` on move, and this happens independently of the `pointerleave` event. Using body as lock target etc doesn't work too. My suggestion - if this causes new bugs - is to remove `requestPointerLock` entirely on Windows. ## Steps for reproduction 1. click button 3. expect xyz ## Code Review - [ ] hi @kof, I need you to do - conceptual review (architecture, feature-correctness) - detailed review (read every line) - test it on preview ## Before requesting a review - [ ] made a self-review - [ ] added inline comments where things may be not obvious (the "why", not "what") ## Before merging - [ ] tested locally and on preview environment (preview dev login: 0000) - [ ] updated [test cases](https://github.com/webstudio-is/webstudio/blob/main/apps/builder/docs/test-cases.md) document - [ ] added tests - [ ] if any new env variables are added, added them to `.env` file
1 parent 6eb989d commit b4c1ffd

File tree

1 file changed

+62
-6
lines changed

1 file changed

+62
-6
lines changed

packages/design-system/src/components/primitives/numeric-gesture-control.ts

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,11 @@ type NumericScrubState = {
8181
cursor?: SVGElement;
8282
direction: NumericScrubDirection;
8383
status: "idle" | "scrubbing";
84+
/**
85+
* On Windows, requestPointerLock might already be called,
86+
* but document.pointerLockElement may not have been updated yet.
87+
*/
88+
pointerCaptureRequested: boolean;
8489
};
8590

8691
const getValueDefault = (
@@ -143,6 +148,14 @@ const addCursorUI = (direction: NumericScrubDirection) => {
143148
};
144149
};
145150

151+
const isWindows = () => {
152+
if (typeof window !== "undefined") {
153+
return navigator.platform.toLowerCase().includes("win");
154+
}
155+
156+
return false;
157+
};
158+
146159
export const numericScrubControl = (
147160
targetNode: HTMLElement | SVGElement,
148161
options: NumericScrubOptions
@@ -168,6 +181,7 @@ export const numericScrubControl = (
168181
cursor: undefined,
169182
direction,
170183
status: "idle",
184+
pointerCaptureRequested: false,
171185
};
172186

173187
// The appearance of the custom cursor is delayed, so we need to track the mouse position
@@ -182,6 +196,8 @@ export const numericScrubControl = (
182196
task();
183197
}
184198

199+
cleanupTasks.length = 0;
200+
185201
if (state.status === "scrubbing") {
186202
state.status = "idle";
187203
onStatusChange?.("idle");
@@ -209,8 +225,7 @@ export const numericScrubControl = (
209225
return;
210226
}
211227

212-
const { type, movementY, movementX } = event;
213-
const movement = direction === "horizontal" ? movementX : -movementY;
228+
const { type } = event;
214229

215230
switch (type) {
216231
case "pointerup": {
@@ -230,6 +245,8 @@ export const numericScrubControl = (
230245
break;
231246
}
232247
case "pointerdown": {
248+
cleanup();
249+
233250
if (
234251
event.target &&
235252
shouldHandleEvent?.(event.target as Node) === false
@@ -296,6 +313,11 @@ export const numericScrubControl = (
296313
break;
297314
}
298315
case "pointermove": {
316+
const { movementY, movementX } = event;
317+
318+
const movement = direction === "horizontal" ? movementX : -movementY;
319+
320+
// console.log("movement", movement, event);
299321
mouseState.x = event.clientX;
300322
mouseState.y = event.clientY;
301323

@@ -328,12 +350,12 @@ export const numericScrubControl = (
328350
// When cursor moves out of the browser window
329351
// we want it to come back from the other side
330352
const top = wrapAround(
331-
Number.parseFloat(state.cursor.style.top) + event.movementY,
353+
Number.parseFloat(state.cursor.style.top) + movementY,
332354
0,
333355
globalThis.innerHeight
334356
);
335357
const left = wrapAround(
336-
Number.parseFloat(state.cursor.style.left) + event.movementX,
358+
Number.parseFloat(state.cursor.style.left) + movementX,
337359
0,
338360
globalThis.innerWidth
339361
);
@@ -345,11 +367,21 @@ export const numericScrubControl = (
345367
}
346368
break;
347369
}
370+
348371
case "pointercancel": {
349372
cleanup();
350373
break;
351374
}
375+
352376
case "lostpointercapture": {
377+
// On Mac if this happens it's near 100% probability that pointerup event will not fire
378+
if (isWindows()) {
379+
// This Windows fix cause other bug, in some cases pointerup event will not fire
380+
if (state.pointerCaptureRequested) {
381+
break;
382+
}
383+
}
384+
353385
if (document.pointerLockElement === null) {
354386
cleanup();
355387
}
@@ -417,18 +449,36 @@ const requestPointerLock = (
417449
disposeOnCleanup(() => {
418450
targetNode.setPointerCapture(pointerId);
419451
return () => {
420-
targetNode.releasePointerCapture(pointerId);
452+
if (targetNode.hasPointerCapture(pointerId)) {
453+
targetNode.releasePointerCapture(pointerId);
454+
}
421455
};
422456
});
423457

458+
let isDisposed = false;
459+
disposeOnCleanup(() => () => {
460+
isDisposed = true;
461+
});
462+
424463
const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent);
464+
425465
// Safari supports pointer lock well, but the issue lies with the pointer lock banner.
426466
// It shifts the entire page down, which creates a poor user experience.
427467
if (!isSafari) {
428468
disposeOnCleanup(() => {
429469
const timerId = window.setTimeout(() => {
470+
state.pointerCaptureRequested = true;
430471
requestPointerLockSafe(targetNode)
431472
.then(() => {
473+
state.pointerCaptureRequested = false;
474+
475+
if (isDisposed) {
476+
if (targetNode.ownerDocument.pointerLockElement === targetNode) {
477+
targetNode.ownerDocument.exitPointerLock();
478+
}
479+
return;
480+
}
481+
432482
const cursorNode =
433483
(targetNode.ownerDocument.querySelector(
434484
"#numeric-guesture-control-cursor"
@@ -466,17 +516,23 @@ const requestPointerLock = (
466516
}
467517
})
468518
.catch((error) => {
519+
state.pointerCaptureRequested = false;
469520
console.error("requestPointerLock", error);
470521
});
471522
}, scrubTimeout);
472523

473524
return () => {
525+
state.pointerCaptureRequested = false;
526+
474527
if (state.cursor) {
475528
state.cursor.remove();
476529
state.cursor = undefined;
477530
}
478531

479-
targetNode.ownerDocument.exitPointerLock();
532+
if (targetNode.ownerDocument.pointerLockElement === targetNode) {
533+
targetNode.ownerDocument.exitPointerLock();
534+
}
535+
480536
clearTimeout(timerId);
481537
};
482538
});

0 commit comments

Comments
 (0)