Fix Safari compatibility with autonomous custom element that replicates HTMLInputElement interface #342
Conversation
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
|
@copilot don't attach a file input to the web component, but fully replicate all hooks and functions needed in order for the component to register as an HTMLInputElement. |
…interface The custom element now: - Creates its own file picker UI with a button and file name display - Implements all HTMLInputElement properties (files, name, value, validity, etc.) - Uses ElementInternals for form participation when available - Provides validation methods (setCustomValidity, reportValidity, checkValidity) - Shows current file value for ClearableFileInput compatibility - All JavaScript and non-Selenium Python tests pass Note: Selenium tests need updating as the hidden input is now created dynamically by the custom element's JavaScript. Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
Implemented in commit 2e9a36c. The custom element now:
All JavaScript tests (10/10) and non-Selenium Python tests (19/19) pass. Note: The 6 Selenium tests need updating because they look for
Would you like me to update the Selenium tests as well? Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
codingjoe
left a comment
codingjoe
left a comment
There was a problem hiding this comment.
Please don't replicate the entire form behavior. The inner file input doesn't need to be hidden. Drop the custom buttons, use a regular file input, and simply attach the correct event handlers.
codingjoe
force-pushed
the
copilot/fix-safari-native-element-extension
branch
from
1d539a2 to
32b07ed
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes Safari compatibility issues by replacing customized built-in elements (which Safari doesn't support) with autonomous custom elements. The change migrates from extending HTMLInputElement with the is attribute to a standalone <s3-file> custom element that replicates the HTMLInputElement interface.
Key changes:
- Replaced
<input type="file" is="s3-file">with<s3-file>autonomous custom element - Implemented complete HTMLInputElement interface (properties:
files,name,value,type,form,validity,disabled,required; methods:setCustomValidity(),reportValidity(),checkValidity(),click()) - Added ElementInternals API support for native form participation with polyfilled fallback for older browsers
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
s3file/static/s3file/js/s3file.js |
Refactored from extending HTMLInputElement to HTMLElement; added hidden file input creation, full HTMLInputElement interface implementation, ElementInternals support, and attribute synchronization |
s3file/forms.py |
Added custom render method to transform Django's input element into <s3-file> custom element; removed is="s3-file" attribute from build_attrs |
tests/__tests__/s3file.test.js |
Updated tests to work with new autonomous custom element structure; removed test for type property that no longer exists on constructor; code formatting changes |
tests/test_forms.py |
Updated Selenium tests to find hidden input by type instead of name; removed assertions about is attribute; added test for s3-file element rendering |
package.json |
Removed lint:js script and standard dependency; added Prettier configuration; removed unused devDependencies |
.pre-commit-config.yaml |
Added Prettier pre-commit hook for JavaScript formatting |
.github/workflows/ci.yml |
Removed separate js-lint job; changed wget to curl for Chrome installation |
Comments suppressed due to low confidence (1)
s3file/static/s3file/js/s3file.js:230
- Similarly, the
uploadHandleraccessesthis.form.pendingRquestswithout checking ifthis.formexists, which would throw a TypeError if the element is not in a form. Add a null check at the start of the method or wrap the form access in a conditional.
uploadHandler() {
if (this.files.length && !this.upload) {
this.upload = this.uploadFiles()
this.form.pendingRquests = this.form.pendingRquests || []
this.form.pendingRquests.push(this.upload)
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def render(self, name, value, attrs=None, renderer=None): | ||
| """Render the widget as a custom element for Safari compatibility.""" | ||
| return mark_safe( # noqa: S308 | ||
| str(super().render(name, value, attrs=attrs, renderer=renderer)).replace( | ||
| f'<input type="{self.input_type}"', "<s3-file" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Using mark_safe with string replacement on potentially user-controlled HTML is risky. The parent's render() method produces HTML that could contain user-provided attribute values (like file names or custom attributes). The simple .replace() operation doesn't validate or sanitize the replacement, and if the parent render output contains unexpected patterns (e.g., multiple <input type="file" strings, or injected content that looks like the pattern), the replacement could produce malformed or exploitable HTML. While the immediate risk is low since Django's form rendering should be safe, consider using proper HTML parsing with a library like html.parser or BeautifulSoup for more robust transformation.
| def render(self, name, value, attrs=None, renderer=None): | ||
| """Render the widget as a custom element for Safari compatibility.""" | ||
| return mark_safe( # noqa: S308 | ||
| str(super().render(name, value, attrs=attrs, renderer=renderer)).replace( | ||
| f'<input type="{self.input_type}"', "<s3-file" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The render method uses a simple string replacement that only changes the opening tag from <input type="file" to <s3-file, but doesn't properly handle the closing tag. HTML file input elements are self-closing (<input ... />), but the custom element <s3-file> needs a proper closing tag (</s3-file>). This replacement will produce malformed HTML like <s3-file ... /> instead of <s3-file ...></s3-file>. Consider using proper HTML parsing and reconstruction, or at minimum, also replace the closing portion of the tag (e.g., /> with ></s3-file>).
| changeHandler() { | ||
| this.keys = [] | ||
| this.upload = null | ||
| try { | ||
| this.form.removeEventListener('submit', this.submitHandler.bind(this)) | ||
| this.form?.removeEventListener("submit", this.submitHandler.bind(this)) | ||
| } catch (error) { | ||
| console.debug(error) | ||
| } | ||
| this.form.addEventListener('submit', this.submitHandler.bind(this), { once: true }) | ||
| this.form?.addEventListener("submit", this.submitHandler.bind(this), { | ||
| once: true, | ||
| }) |
There was a problem hiding this comment.
The changeHandler method attempts to remove an event listener by calling this.submitHandler.bind(this), but this creates a new function reference that won't match the originally added listener. The bind() method creates a new function each time it's called, so the removeEventListener will fail silently and the old listener will remain attached, potentially causing duplicate event handlers and memory leaks. Consider storing the bound handler as a class property (e.g., this._boundSubmitHandler = this.submitHandler.bind(this) in the constructor) and using that reference for both adding and removing the listener.
| _syncAttributesToHiddenInput() { | ||
| if (!this._hiddenInput) return | ||
|
|
||
| const attrsToSync = ["accept", "required", "multiple"] | ||
| attrsToSync.forEach((attr) => { | ||
| if (this.hasAttribute(attr)) { | ||
| this._hiddenInput.setAttribute(attr, this.getAttribute(attr)) | ||
| } else { | ||
| this._hiddenInput.removeAttribute(attr) | ||
| } | ||
| }) | ||
|
|
||
| this._hiddenInput.disabled = this.hasAttribute("disabled") | ||
| } |
There was a problem hiding this comment.
The tests now assert that the hidden file input has an empty name (name=""), which is correct since the custom element should handle the form submission. However, this creates a potential issue: if the hidden input accidentally gets a name attribute through attribute synchronization, it could submit duplicate data. The _syncAttributesToHiddenInput method in the JavaScript doesn't sync the 'name' attribute (it only syncs 'accept', 'required', 'multiple'), which is correct. Consider adding an explicit check or comment in _syncAttributesToHiddenInput to ensure the name attribute is never copied to the hidden input to prevent accidental form data duplication.
| connectedCallback() { | ||
| // Create a hidden file input for the file picker functionality | ||
| this._hiddenInput = document.createElement("input") | ||
| this._hiddenInput.type = "file" | ||
|
|
||
| // Sync attributes to hidden input | ||
| this._syncAttributesToHiddenInput() | ||
|
|
||
| // Listen for file selection on hidden input | ||
| this._hiddenInput.addEventListener("change", () => { | ||
| this._files = this._hiddenInput.files | ||
| this.dispatchEvent(new Event("change", { bubbles: true })) | ||
| this.changeHandler() | ||
| }) | ||
|
|
||
| // Append elements | ||
| this.appendChild(this._hiddenInput) | ||
|
|
||
| // Setup form event listeners | ||
| this.form?.addEventListener("formdata", this.fromDataHandler.bind(this)) | ||
| this.form?.addEventListener("submit", this.submitHandler.bind(this), { | ||
| once: true, | ||
| }) | ||
| this.form?.addEventListener("upload", this.uploadHandler.bind(this)) | ||
| } |
There was a problem hiding this comment.
The custom element doesn't implement a disconnectedCallback to clean up event listeners added to the form element in connectedCallback. When the custom element is removed from the DOM, the form's event listeners for 'formdata', 'submit', and 'upload' will remain attached, causing memory leaks. Add a disconnectedCallback method that removes these listeners to prevent memory leaks when elements are removed from the DOM.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
==========================================
+ Coverage 98.19% 98.20% +0.01%
==========================================
Files 8 8
Lines 221 223 +2
==========================================
+ Hits 217 219 +2
Misses 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codingjoe
force-pushed
the
copilot/fix-safari-native-element-extension
branch
from
c34c592 to
8233407
Compare
codingjoe
force-pushed
the
copilot/fix-safari-native-element-extension
branch
from
f3c0eaa to
8b17c41
Compare
Problem
Version 6.0.0 introduced web components using customized built-in elements (extending native
HTMLInputElementwith theisattribute). However, Safari on both macOS and iOS does not support this approach, causing file uploads to S3 to fail silently in Safari browsers.See: https://caniuse.com/mdn-api_customelementregistry_builtin_element_support and https://bugs.webkit.org/show_bug.cgi?id=182671
Solution
Replaced customized built-in elements with Safari-compatible autonomous custom elements that fully replicate the HTMLInputElement interface without wrapping a file input element.
HTML Output Change
Before (Safari incompatible):
After (Safari compatible):
Key Changes
JavaScript (
s3file.js):class S3FileInput extends HTMLInputElementtoextends HTMLElement{ extends: 'input' }option fromcustomElements.define()files,name,value,type,form,validity,disabled,requiredpropertiessetCustomValidity(),reportValidity(),checkValidity(),click()<input type="file">internally (via JavaScript) only for triggering browser's file picker dialogobservedAttributesandattributeChangedCallbackPython (
forms.py):render()method to render standalone<s3-file>element (no nested input)Implementation Details
The custom element provides a complete file input replacement:
Browser Compatibility
Backward Compatibility
This change maintains API compatibility:
<s3-file>element or wait for dynamically created hidden inputTesting
Fixes #327
Original prompt
Fixes #327
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.