Fix #363 -- Move labels to native file input #364
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #364 +/- ##
=======================================
Coverage 97.72% 97.72%
=======================================
Files 8 8
Lines 264 264
=======================================
Hits 258 258
Misses 6 6
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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #363 by associating labels with the native file input element instead of the custom s3-file element. This ensures that clicking labels properly triggers the file picker dialog.
Key changes:
- Modified the s3-file custom element to generate unique IDs for internal file inputs and reassociate labels to point to them
- Added a
labelsgetter to the S3FileInput class that retrieves associated labels via ElementInternals API or DOM query - Changed the HTML output from self-closing
<s3-file>tags to properly closed<s3-file></s3-file>tags to support containing the native input element
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| s3file/static/s3file/js/s3file.js | Adds label reassociation logic in connectedCallback, generates unique IDs for file inputs, and implements a labels getter to find associated label elements |
| s3file/forms.py | Updates handle_starttag to output closing tags for s3-file elements instead of self-closing tags |
| tests/tests/s3file.test.js | Adds three new tests to verify label association with native file input works correctly, handles missing labels gracefully, and confirms label clicks trigger the file input |
| tests/test_forms.py | Updates test assertions to expect properly closed </s3-file> tags instead of self-closing tags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test("connectedCallback handles input without labels", () => { | ||
| const form = document.createElement("form") | ||
| document.body.appendChild(form) | ||
| const input = new s3file.S3FileInput() | ||
| // Should not throw error when there are no labels | ||
| form.appendChild(input) | ||
| assert(input._fileInput !== null) | ||
| assert(input._fileInput.type === "file") | ||
| }) |
There was a problem hiding this comment.
There is no test coverage for the scenario where multiple s3-file elements exist without ID attributes. This is important to test because the file input ID generation (line 43 of s3file.js) uses a fallback value that could result in duplicate IDs. Consider adding a test that creates multiple s3-file elements without IDs and verifies that each gets a unique file input ID.
| return ( | ||
| this._internals?.labels || document.querySelectorAll(`label[for="${this.id}"]`) | ||
| ) |
There was a problem hiding this comment.
The fallback query selector will create invalid selectors when this.id is undefined or an empty string. This could result in label[for="undefined"] or label[for=""], which may match unintended labels or throw errors. Consider adding a guard to return an empty array or NodeList when this.id is falsy.
| return ( | |
| this._internals?.labels || document.querySelectorAll(`label[for="${this.id}"]`) | |
| ) | |
| if (this._internals?.labels) { | |
| return this._internals.labels | |
| } | |
| if (!this.id) { | |
| // When there is no id, there can be no associated <label for="..."> elements. | |
| // Return an empty collection instead of building an invalid or unintended selector. | |
| return [] | |
| } | |
| return document.querySelectorAll(`label[for="${this.id}"]`) |
| // Create a hidden file input for the file picker functionality | ||
| this._fileInput = document.createElement("input") | ||
| this._fileInput.type = "file" | ||
| this._fileInput.id = `${this.id || "s3file"}-input` |
There was a problem hiding this comment.
When multiple s3-file elements exist without an ID attribute, they will all generate the same file input ID "s3file-input", violating HTML's requirement for unique IDs. Consider appending a unique identifier (e.g., timestamp or counter) to ensure each generated ID is unique.
| test("connectedCallback handles input without labels", () => { | ||
| const form = document.createElement("form") | ||
| document.body.appendChild(form) | ||
| const input = new s3file.S3FileInput() | ||
| // Should not throw error when there are no labels | ||
| form.appendChild(input) | ||
| assert(input._fileInput !== null) | ||
| assert(input._fileInput.type === "file") | ||
| }) |
There was a problem hiding this comment.
The test doesn't verify the behavior of the labels getter when the custom element has no ID attribute. Given the fallback query selector in the labels getter uses this.id, it would be valuable to add an assertion that checks input.labels returns an empty collection when the element has no ID, ensuring the getter handles this edge case properly.
|
@codingjoe I tried it and its still un responsive when i click on the label. |
@yardensachs do you have the rendered HTML for me? |
|
Yes, I a also using django-unfold as a django theme. File: |
clicking on the row doesnt do anything, but clicking on "Logo" does trigger it, now that I see it. I wonder why it does the "-input" thing |
|
@codingjoe for us an input field was not visible anymore from version 6.0 I tried version 6.0.2 from this branch and the button is still gone and this is the html. <form action="/toolbox/booking-option/import/"
method="post"
enctype="multipart/form-data">
<input type="hidden" name="csrfmiddlewaretoken" value="[REDACTED-CSRF-TOKEN]">
<fieldset class="module aligned">
<div class="form-row">
<p>
<label for="id_csv_file" class="file-label">
CSV-Datei:
</label>
<s3-file
name="csv_file"
data-fields-x-amz-signature="[REDACTED-AWS-SIGNATURE]"
data-fields-x-amz-algorithm="AWS4-HMAC-SHA256"
data-fields-x-amz-date="20260106T094546Z"
data-fields-x-amz-credential="[REDACTED-AWS-CREDENTIAL]"
data-fields-policy="[REDACTED-BASE64-POLICY]"
data-fields-key="tmp/s3file/[REDACTED-ID]/${filename}"
data-url="/__s3_mock__/"
data-s3f-signature="[REDACTED-S3-SIGNATURE]"
accept="text/csv"
required
id="id_csv_file"
placeholder=" ">
</s3-file>
</p>
</div>
</fieldset>
<div class="submit-row">
<input type="submit" value="importieren" class="default">
</div>
</form>Does this help with finding the issue or is this something else altogether? |
|
It really pains me, but I think it's best to revert to the old implementation. It wasn't as elegant, but without Apple's support, it loses its elegance to more trouble than reward. I will revert the changes and release a new major version. |
@yardensachs would you care to review if this patch resolves your issue?