GoogleChromeLabs / browser-fs-access

File System Access API with legacy fallback in the browser
https://googlechromelabs.github.io/browser-fs-access/demo/
Apache License 2.0
1.38k stars 84 forks source link

AbortError on opening files on legacy browsers #53

Closed dwelle closed 3 years ago

dwelle commented 3 years ago

It seems the https://github.com/GoogleChromeLabs/browser-fs-access/issues/36 isn't fully fixed as we're still getting reports on Excalidraw. Can't reproduce on my system.

One confirmed case: https://github.com/excalidraw/excalidraw/issues/3740#issuecomment-863131754, and one probable case https://github.com/excalidraw/excalidraw/issues/3513. Both Firefox.

tomayac commented 3 years ago

I just downloaded Firefox 78.11.0esr (64-bit) on macOS 12.0 Beta (21A5248p) and ran through the demo. All features worked, but for directoryOpen() (not used by Excalidraw) it seems like there is some filtering in place for certain directories like Pictures. It works fine with subfolders, though. I have a hard time figuring out where fileOpen() could fail. Maybe at this point it's worth opening a bug for Firefox directly?

dwelle commented 3 years ago

Yeah, I cannot reproduce either but that doesn't help us. Before I'd open a Firefox issue I'll want to take some time to review the hack we're using to polyfill the rejection, and whether it's reasonable to expect browsers to behave deterministically in this case.

tomayac commented 3 years ago

I have created a reduced test case that you can try and step through in Firefox. For all the Firefox instances I could test this with, it worked correctly (the screenshots below are from Firefox 89.0.1 (64-bit) on macOS 12.0 Beta (21A5248p)):

teramawi commented 3 years ago

I'm the reporter of the linked excalidraw issue https://github.com/excalidraw/excalidraw/issues/3740 and I am able to reproduce the AbortError problem using your reduced test case in 78.11.0esr.

The problem Is, that selecting a file using double click leads to a pointer event being triggered before the change event of the file dialog, therefore aborting the selection.

I was able to watch this by enabling event logging in the debugger for "Control -> change" and "Pointer -> Move". There were no other events interfering:

pointermove { target: html, buttons: 1, clientX: 717, clientY: 72, layerX: 717, layerY: 72 }      script.js:17:35
change { target: input, isTrusted: true, srcElement: input, currentTarget: input, eventPhase: 2, bubbles: true, cancelable: false, returnValue: true, defaultPrevented: false, composed: false, … }         script.js:24:6

The selection does work if I submit my selection using the "open" button rather than doing a double click. I was also able to reproduce this behaviour in excalidraw as well.

tomayac commented 3 years ago

I tried doubleclick file selection and it just worked fine for me. No matter if the file open dialog is over the Firefox window, just overlapping with it, or completely distant from it. It worked in all cases. What platform are you on?

dwelle commented 3 years ago

I can intermittently reproduce in Firefox when using the double-click on the file.

And another interesting thing. Trying to reproduce in Brave I was getting a different bug: the AbortError was sometimes shown right after the click, before the file dialog was opened. And since the promise was rejected beforehand, the ensuing file open didn't resolve and stayed rejected. EDIT: tried on excalidraw.com in Brave and indeed I can reproduce there as well (took ~10 tries).

So Chromium is affected too, except in a different manner.

teramawi commented 3 years ago

I'm on Windows 10 64bit 20H2 (Build 19042.804) with Firefox 78.11.0esr (32-Bit) and I can reproduce the issue reliably.

I can not reproduce the issue using Google Chrome Version 91.0.4472.101 (Offizieller Build) (32-Bit) using the same platform.

Let me know if i can help you somehow pinpointing the problem.

dwelle commented 3 years ago

I'm getting the AbortError on FF even with single click (not sure if more or less often than double click). Leads me to believe the Chromium and FF bugs may be the same, except FF doesn't render the error in time before the dialog opens.

dwelle commented 3 years ago

How does the polyfill even work ever?

https://github.com/GoogleChromeLabs/browser-fs-access/blob/ad4399f278d9e1d1f4187a48cc0fb7aaa351240b/src/legacy/file-open.mjs#L52-L62

var handler = () => console.log('click');
document.body.addEventListener('click', handler);
document.body.click();
document.body.removeEventListener('click', handler);

The above catches the programmatic click event because the listener is bound synchronously.

Maybe the browser-fs-access hack depends on the file dialog blocking the thread? But how is the ensuing change event fired before the click event.

Actually, from @teramawi's comment https://github.com/GoogleChromeLabs/browser-fs-access/issues/53#issuecomment-863858428 it seems the click event doesn't come to play, and it's the pointermove event. Though it's a comparable problem.


EDIT: ok, checking the source, we're not listening on click, which is why the click() does not trigger the rejection.

dwelle commented 3 years ago

I can confirm the Brave bug relates to pointermove as well. And it's 100% reproducible by slide-clicking (moving your mouse a bit when clicking) on the button:

brave_fs_access_bug

seanaye commented 3 years ago

I get this issue 100% of the time on Brave. Even when not moving the mouse at all and triggering the event with tab+enter, although unlike the screencap above it errors only after I select a file and click open.

Brave: Version 1.25.73 Chromium: 91.0.4472.106 (Official Build) (arm64) MacOS: 11.1

RomLAURENT commented 3 years ago

Hi,

Same issue here with FF.

Here is my custom legacy setup to circumvent this matter :

export const setupLegacyCleanupAndRejection = rejectionHandler => {
    const timeoutHandle = setTimeout(() => {
        window.addEventListener("pointermove", rejectionHandler);
        window.addEventListener("pointerdown", rejectionHandler);
        window.addEventListener("keydown", rejectionHandler);
    }, 500);

    return rej => {
        clearTimeout(timeoutHandle);
        window.removeEventListener("pointermove", rejectionHandler);
        window.removeEventListener("pointerdown", rejectionHandler);
        window.removeEventListener("keydown", rejectionHandler);
        if (rej) {
            rej(new DOMException("The user aborted a request.", "AbortError"));
        }
    };
};

Basically I'm just delaying the start of listening for "abort" events.

tomayac commented 3 years ago

I have now removed the rejection handling for legacy browsers entirely since it caused more harm than good. People who need the throwing behavior can configure their own handler, as per the option introduced in #45.