-
Notifications
You must be signed in to change notification settings - Fork 2.1k
@uppy/dashboard: fix form appending for shadow dom #6031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: edf81e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. But we probably don't need the new dev environment, I don't think we'll use it. A test would be great but you mentioned that's difficult. It also doesn't work in WSL? If not, then we can leave it for now
|
@Murderlon I couldn't get it to work under WSL either, sorry :( |
|
Can you remove the dev setup and resolve the CI errors? |
|
@Murderlon As requested, I removed the "dashboard_shadow.html" file and addressed the lint issues. Thank you for your consideration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| * Use the "rootNode" of whereever Uppy is rendered, falling back | ||
| * to `window.document` if domRef isn't initialized for some reason | ||
| */ | ||
| const rootNode = domRef.current?.getRootNode() ?? (document as Node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Guarding getRootNode: handle absent method gracefully
domRef.current?.getRootNode() only guards domRef.current being nullish, not the presence of getRootNode. In environments where Node.getRootNode is undefined (older/legacy browsers or certain polyfilled contexts), this will throw TypeError: getRootNode is not a function. Use domRef.current?.getRootNode?.() (or a safe fallback like ownerDocument) to avoid a runtime crash.
Fixes #6028
I've also added
private/dev/dashboard_shadow.htmlto more easily test ShadowDOM-specific bugs.I looked into writing a test case under
packages/@uppy/dashboard/src/index.browser.test.tsbut couldn't get it to work locally on Windows.Note
Ensure FileCard’s hidden form is appended/cleaned up in the correct root (Document body or ShadowRoot) using a ref-derived root node, and add a patch changeset.
formto the correct root usinggetRootNode()(handles Light DOM/iframes viaDocument.body, and Shadow DOM viaShadowRoot).domRefto root element to detect rendering context; attachrefand update effect accordingly.formfrom its actualparentNode.Fix form appending for shadow dom.Written by Cursor Bugbot for commit edf81e8. This will update automatically on new commits. Configure here.