Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#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.

See https://github.com/django/django/blob/eff21d8e7a1cb297aedf1c702668b590a1b618f3/django/contrib/admin/templates/admin/widgets/clearable_file_input.html#L3

This first appeared in Django 5.0.

My suggestion would be to avoid rendering attrs into the checkbox.

Change History (17)

comment:1 by Natalia Bidart, 9 months ago

Resolution: needsinfo
Status: newclosed

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.

comment:2 by Johannes Maron, 9 months ago

Resolution: needsinfo
Status: closednew

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 Mariusz Felisiak, 9 months ago

Cc: Marcelo Galigniana David Smith added

comment:4 by Jörg Benesch, 9 months ago

I can second the report and will put the triage to accepted.

comment:5 by Jörg Benesch, 9 months ago

Triage Stage: UnreviewedAccepted

comment:6 by Natalia Bidart, 9 months ago

Resolution: needsinfo
Status: newclosed

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 ids):

<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 Mariusz Felisiak, 9 months ago

Triage Stage: AcceptedUnreviewed

comment:8 by Johannes Maron, 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

Last edited 9 months ago by Johannes Maron (previous) (diff)

comment:9 by Tim Graham, 9 months ago

Resolution: needsinfo
Severity: NormalRelease blocker
Status: closednew
Triage Stage: UnreviewedAccepted

comment:10 by Johannes Maron, 9 months ago

Owner: changed from nobody to Johannes Maron
Status: newassigned

I write a full patch tomorrow and check if we have other widgets that include the same problem.

Version 0, edited 9 months ago by Johannes Maron (next)

comment:11 by Johannes Maron, 9 months ago

Easy pickings: set
Has patch: set
UI/UX: set

comment:12 by Johannes Maron, 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 Tim Graham, 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 Johannes Maron, 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 Mariusz Felisiak, 9 months ago

Triage Stage: AcceptedReady for checkin

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

Resolution: fixed
Status: assignedclosed

In e6901955:

Fixed #35273 -- Fixed rendering AdminFileWidget's attributes.

Regression in 8a6c0203c4e92908c2b26ba54feba4ce7e76d081.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

In 8fd953f2:

[5.0.x] Fixed #35273 -- Fixed rendering AdminFileWidget's attributes.

Regression in 8a6c0203c4e92908c2b26ba54feba4ce7e76d081.

Backport of e69019555d683fd6a831f87cb09e3deb86e4e7c7 from main

Note: See TracTickets for help on using tickets.
Back to Top