fix(desktop_drop): accept web drops whose items have no FileSystemEntry - #503
fix(desktop_drop): accept web drops whose items have no FileSystemEntry#503jamesaorson wants to merge 1 commit into
Conversation
The web ondrop handler called webkitGetAsEntry()! on every item in the drag. That API returns null for string items, and a drag routinely carries them alongside its files -- a drag from the browser's downloads menu sends text/uri-list and text/plain next to the file. The null assert threw synchronously inside List.generate, before the catchError was attached, so performOperation_web was never invoked: no DropDoneEvent reached the app, the file never uploaded, and the drop target's highlight stayed stuck until a rebuild. Null-guard the entry and fall back to getAsFile(), which returns the file for exactly the items the entry API cannot describe. Skip items that yield neither. Also notify with the resulting list even when it is empty. The event is what resets DropTarget's status, so swallowing it on a file-less drag (a dragged link, selected text) leaves the highlight stuck -- the same symptom, one step further along. Refs MixinNetwork#456, MixinNetwork#459 Signed-off-by: James Orson <jamesaorson@gmail.com>
|
Runtime verification is in — the PR body has been updated accordingly (it previously said this was unverified). A drag from the browser's downloads menu into an upload target now works on a build against this branch. Side by side, same drag, same machine (macOS):
I checked the served bundle in each case to be sure the difference was really this patch: Worth adding for @jtmcdole and #456: the trigger here was not a zip viewer but the browser's own downloads menu, which is a very ordinary thing for a user to drag from into an upload area. Both browsers put a real |
…pstream desktop_drop's web ondrop handler calls webkitGetAsEntry()! on every item in a drag. That returns null for string items, and a drag routinely carries them next to its files -- the browser's downloads menu sends text/uri-list and text/plain alongside the file. The null assert throws inside List.generate before catchError is attached, so performOperation_web never runs: no DropDoneEvent reaches the app, the file never uploads, and the drag highlight stays stuck until a rebuild. Point desktop_drop at our fork, which null-guards the entry, falls back to getAsFile(), and notifies with the resulting list even when empty so a file-less drag still clears the drop target instead of wedging it. The override tracks the fork's main; pubspec.lock pins the resolved commit. Remove it once a release carrying the fix reaches pub.dev. Upstream: MixinNetwork/flutter-plugins#503, refs #456 and #459. Signed-off-by: James Orson <jamesaorson@gmail.com>
Refs #1831 ## What Drag-and-drop upload was broken on web: the drop area highlighted, releasing never uploaded, and the highlight stayed stuck until a page refresh. The reporter hit it dragging from the browser's downloads menu. `desktop_drop`'s web handler calls `webkitGetAsEntry()!` on every item in the drag. That returns null for `kind == 'string'` items, and a drag routinely carries them alongside its files — the downloads menu sends `text/uri-list` and `text/plain` next to the file. The null assert throws synchronously inside `List.generate`, before the `.catchError` further down the chain is attached, so `performOperation_web` is never invoked. No `DropDoneEvent` reaches the app, the file never uploads, and `DropTarget`'s status never resets — hence the stuck highlight. The file was in the drag the whole time. This points `desktop_drop` at our fork, which null-guards the entry and falls back to `getAsFile()`. `getAsFile()` and `webkitGetAsEntry()` are not equivalent — the latter needs a filesystem-backed entry and returns null for exactly the items the former handles. That is also why the same drag works on other web upload targets and not on ours. ## Changes - `pubspec.yaml`: `dependency_overrides` pointing `desktop_drop` at `autobutler-org/flutter-plugins` `main`, with a comment explaining why and when to remove it - `pubspec.lock`: resolved to `29e5924` Fork commit: [autobutler-org/flutter-plugins@29e5924](autobutler-org/flutter-plugins@29e5924) Upstream PR: [MixinNetwork/flutter-plugins#503](MixinNetwork/flutter-plugins#503). The root cause is already filed upstream as [#456](MixinNetwork/flutter-plugins#456) from a different trigger (dragging out of a zip viewer, likewise no `FileSystemEntry`); the downloads-menu case is new evidence on the same defect. A stalled draft, [#459](MixinNetwork/flutter-plugins#459), fixes the null assert the same way — our version additionally notifies with the resulting list even when it is empty, so a drag carrying no files at all (a dragged link, selected text) clears the drop target instead of wedging it the same way. ## PR Type - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Docs / content - [ ] Chore / tooling - [ ] Tests ## Surface - [ ] Backend (Go) - [x] Frontend (Flutter) - [ ] API / swagger - [ ] CI / workflows - [ ] Docs / content only ## Testing - [x] Local testing recommended (UI changes, routing, behavior changes) - [ ] Local testing not needed (logic-only, docs, trivial change) - [ ] Includes new automated tests - [x] Manually tested by author Tested side by side on macOS: stock 0.8.4 on one port reproduces the failure, the patched build on another uploads normally. The served bundles were checked to confirm the difference was really the patch — `getAsFile` appears in the compiled output of the patched build and is absent from stock 0.8.4. `make check` passes. ## Bot Review Guidance The risk here is the dependency override, not application code. Worth checking that tracking a fork's `main` is acceptable, and that `pubspec.lock` pinning the resolved commit is considered sufficient for reproducibility. ## Notes **Deliberately `Refs`, not `Closes`.** This fixes the crash but not the whole issue. Three independent defects found during the investigation are untouched and #1831 should stay open for them: - 90 ms folder-hover debounce race (`file_browser_page.dart:763-787`, `:2199`) — a drop released between the page target and a folder target lands in neither; separately, after hovering any folder row the page-level highlight is dead for the rest of that drag - `FolderDropTarget` has no `enable` gate (`folder_drop_target.dart:35`) while the page target gates on `!_isUploading` (`:2171`), so folder rows highlight during a background upload and then early-return - `_isWebDragging` has no reset path other than an exit/done event — no timeout, no reset on rebuild **No automated coverage.** There are no widget tests for `FolderDropTarget`, `FolderDropWrapper`, or the page's drag state machine; the only existing test is pure-function coverage of `flattenDroppedItems`. Every defect above lives in the untested wiring. **Cleanup owed.** The override tracks a moving ref. `pubspec.lock` pins the resolved commit so builds stay reproducible, but `pub upgrade` will follow wherever that branch goes. Remove the override once a `desktop_drop` release carrying the fix reaches pub.dev — the comment in `pubspec.yaml` is the only thing carrying that reminder. Signed-off-by: James Orson <jamesaorson@gmail.com>
|
We are using it over here btw: https://github.com/autobutler-org/quark |
Fixes the web
ondrophandler crashing on any drag whose items are not all backed by aFileSystemEntry.The bug
desktop_drop_web.dartcallswebkitGetAsEntry()!on every item in the drag:webkitGetAsEntry()returns null forkind == 'string'items, and for file items with no filesystem entry behind them. The null assert throws synchronously insideList.generate, before the.catchErrorfurther down the chain is attached — so the error is not even caught by the handler that looks like it should catch it.performOperation_webis never invoked, noDropDoneEventreaches the app, and the drop target is left in a non-idle state: nothing is dropped, and the drag highlight stays stuck until the widget is rebuilt.How it is reached
This is #456 (dragging a file out of a zip viewer). We hit the same crash from a different direction: dragging from the browser's own downloads menu, which is a very ordinary thing for a user to do into an upload target.
Both Chrome and Firefox put a real, disk-backed
Fileon the dataTransfer, plus string items next to it. Firefox is explicit about it indownloads.js:One file item, two string items. The string items hit the null assert and take the whole drop down with them — including the perfectly good file sitting in the same drag. Ordinary HTML5 drop handlers accept these drags today, which is why the same drag works on other web upload targets but not on a
desktop_dropone.The fix
Null-guard the entry and fall back to
getAsFile(). The two APIs are not equivalent:getAsFile()returns the file for exactly the items the entry API cannot describe. Items that yield neither are skipped.webkitGetAsEntry()is still preferred when present, since it is what makes directory recursion possible.One deliberate difference from #459
#459 (draft, by @copilot-swe-agent) fixes the null assert the same way, and this PR keeps that approach. It also returns early when no usable items come out:
This PR instead invokes
performOperation_webwith the empty list. Skipping the call means a drag carrying no files at all (a dragged link, selected text) never produces aDropDoneEvent— and since that event is what resetsDropTarget's status, the drag highlight stays stuck exactly as it does in the crashing case.channel.dartalready handles an empty list correctly, emittingDropDoneEvent(files: []), so notifying unconditionally clears the drop target and lets the app decide there was nothing to take.Happy to close this in favor of #459 with the empty-list change folded in, if the maintainers would rather take that branch — the important half is the null-guard either way.
Testing
dart analyzeclean.Verified at runtime on macOS. A drag from the browser's downloads menu into an upload target built against this branch now uploads the file. The same drag against a stock 0.8.4 build reproduces the original failure — nothing uploads and the drag highlight stays stuck until the widget rebuilds. The two builds were run side by side on different ports, and the served bundle was checked to confirm the patched handler was the one actually loaded (
getAsFileappears in the compiled output of this branch and not in stock 0.8.4).The report that prompted this was an ordinary user hitting it in normal use, not a synthetic case.
The package's only existing test is
channel_linux_test.dart; there is no web test harness here, so this change has no automated coverage either. Glad to add one if you would like to point me at a preferred shape.I did not touch
CHANGELOG.mdor the version, on the assumption releases are maintainer-driven — say the word and I will add an entry.