#35273 closed Bug (fixed)
AdminFileWidget renders two elements with the same ID
Reported by: | Johannes Maron | Owned by: | Johannes Maron |
---|---|---|---|
Component: | contrib.admin | Version: | 5.0 |
Severity: | Release blocker | Keywords: | |
Cc: | Johannes Maron, Marcelo Galigniana, David Smith | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
The AdminFileWidget
renders both the checkbox and file input with the same attributes, including the ID.
The template actually renders the ID attribute twice.
Both, two identical attributes on an element or two elements with the same ID are in violation with version of HTML.
This first appeared in Django 5.0.
My suggestion would be to avoid rendering attrs
into the checkbox.
Change History (17)
comment:1 by , 9 months ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:2 by , 9 months ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Hi Natalia,
Thanks, I appreciate you taking the time looking into this and welcoming me. Hehe, but I am not really new ;) I happened to contribute, among other things, the template-based form rendering and autocomplete fields. Thus, I would consider myself somewhat familiar with this part of Django.
The test you are pointing to is insufficient, since it doesn't include all arguments what would be passed to a widget by a form.
You can see that the output HTML doesn't include an ID for the input[type=file]
. Which it would obviously have in an actual form.
Funnily enough, the test actually outputs invalid HTML too. A label.for
attribute must point to an existing ID, which it doesn't.
I am happy to provide a test that will fail. The regression was introduced in Django 5 and only applies to the admin file input template.
Cheers,
Joe
comment:3 by , 9 months ago
Cc: | added |
---|
comment:5 by , 9 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 9 months ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
Thank you Johanness and Jörg!
Before accepting, we'd still need a sample project or test case showcasing the issue, as I can't reproduce the duplicated ID symptom (I'm using latest main
though). I created a project of my own and defined a model with a FileField, then configured its admin so the AdminFileWidget
is used. This is the resulting HTML which looks correct to me (at least it does not have duplicated id
s):
<p class="file-upload">Currently: <a href="/images/test_X66RiS7.py">images/test_X66RiS7.py</a> <span class="clearable-file-input"> <input type="checkbox" name="avatar-clear" id="avatar-clear_id"> <label for="avatar-clear_id">Clear</label></span><br> Change: <input type="file" name="avatar" id="id_avatar"></p>
comment:7 by , 9 months ago
Triage Stage: | Accepted → Unreviewed |
---|
comment:8 by , 9 months ago
Absolutely, here is a draft with a commit that exploits the error:
https://github.com/django/django/pull/17946
And that's the error:
https://github.com/django/django/actions/runs/8178301006/job/22361926712#step:6:25331
- <input id="test-clear_id" id="test_id" name="test-clear" type="checkbox"><label for="test-clear_id"> ? ------------- + <input id="test-clear_id" name="test-clear" type="checkbox"><label for="test-clear_id">
And yes, a bound field will pass the ID as an attribute to a widget here:
https://github.com/django/django/blob/c4df2a77761a1ae392eb5c4803b5712803d5239f/django/forms/boundfield.py#L96-L99
Hehe, and yes, good to be cautious. My last ticket was a dumpster fire. I read the docs this time, promise :P
I sincerely appreciate the swift support here <3
comment:9 by , 9 months ago
Resolution: | needsinfo |
---|---|
Severity: | Normal → Release blocker |
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Bisected to 8a6c0203c4e92908c2b26ba54feba4ce7e76d081.
comment:10 by , 9 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I write a full patch tomorrow and check if we have other widgets that include the same problem.
comment:11 by , 9 months ago
Easy pickings: | set |
---|---|
Has patch: | set |
UI/UX: | set |
comment:12 by , 9 months ago
OK, the patch is ready for review. BTW, the problem was only in the Django, not the Jinja2 template. I checked all the other widgets, none-other seemed to have the same issue.
comment:13 by , 9 months ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
The patch merely reverts part of 8a6c0203c4e92908c2b26ba54feba4ce7e76d081. and causes admin_widgets.tests.ImageFieldWidgetsSeleniumTests.test_clearablefileinput_widget_preserve_clear_checkbox
to fail.
(Incidentally, there is no need to set "easy pickings" to put this ticket on the radar for new contributors.)
comment:14 by , 9 months ago
Patch needs improvement: | unset |
---|
Thanks, Tim, for your diligence. I did read up on the other ticket and applied its behavior to all clearable file templates and added tests and another line to the release docs.
Please tell me, if there is anything else that needs to be addressed.
comment:15 by , 9 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hello Johanes, thank you for your interest in making Django better.
Could you please include a small example (or test case!) showcasing the described issue? Looking at the code and relevant test I do not see the duplicated
id
attribute, so I'm assuming there is some specifics on the code you are using that is triggering this behavior.Thank you! Feel free to re-open when you can provide more details.