Opened 8 years ago
Closed 8 years ago
#27015 closed Cleanup/optimization (fixed)
Hidden widget shouldn't have maxlength/minlength attributes
| Reported by: | Claude Paroz | Owned by: | nobody |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
This was reported against django-contrib-comments: https://github.com/django/django-contrib-comments/pull/94
The widget_attrs() method of forms.CharField is currently transmitted its min_length/max_length attributes to the related widget. It should not do it for hidden widgets as the specs don't list those attributes as permitted attributes.
https://www.w3.org/TR/html-markup/input.hidden.html
Change History (9)
comment:1 by , 8 years ago
| Easy pickings: | unset |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
follow-up: 3 comment:2 by , 8 years ago
If we simply test for the widget type in CharField.widget_attrs(), I think this will do it (without touching any purposefully defined widget attributes).
comment:3 by , 8 years ago
Replying to claudep:
If we simply test for the widget type in
CharField.widget_attrs(), I think this will do it (without touching any purposefully defined widget attributes).
Or rely on Widget.is_hidden?
comment:5 by , 8 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
comment:6 by , 8 years ago
| Needs documentation: | set |
|---|---|
| Patch needs improvement: | set |
| Triage Stage: | Ready for checkin → Accepted |
Technically, minvalue/maxvalue is only incompatible with <input type="hidden">, not all widgets that might have is_hidden = True.
I know it's a bit far-fetched, and the current code is probably good enough, but I think we should still mention this in the release notes, just on the off-chance someone has some kind of custom widget that is hidden but relies on minvalue/maxvalue being passed.
comment:7 by , 8 years ago
I'm skeptical as Widget.is_hidden isn't documented as something that's meant to be overridden. If you have an example usage, I'll be interested to see it.
comment:8 by , 8 years ago
| Needs documentation: | unset |
|---|---|
| Patch needs improvement: | unset |
| Triage Stage: | Accepted → Ready for checkin |
I don't have a concrete example and don't care enough about this to stand in the way of the current patch.
As far as I can tell Widget.is_hidden isn't documented (only BoundField.is_hidden is) but that's never really stopped anyone in the past from using things :)
That seems like a reasonable feature request.
I think the widget should still output those attribute if they've been specified by the user (not everyone cares about valid HTML and they might actually want to use those attribute with javascript for example).
This might make the implementation a bit tricky (which is why I removed the "easy pickings" flag).