Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove maxlength attribute for html5 compliance #490

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

sterliakov
Copy link
Contributor

Pages that use PhonePrefixSelect do not validate with vnu validator because <select> tag is rendered with maxlength attribute which makes no sense for that tag. With this PR we just inspect context and pop maxlength attribute if it is present.

@francoisfreitag
Copy link
Collaborator

Please add a test that prevents the issue from commit back.

@amateja
Copy link
Collaborator

amateja commented Mar 11, 2022

Hi @sterliakov,

Your issue is caused by PhoneNumberPrefixWidget parent class - django.forms.widgets.MultiWidget. When MultiWidget.get_context is called it also calls subwidgets' get_context methods sharing own context containing maxlength attribute with them.

Instead modifying context of the widget I would recommend to strip maxlength key from attrs like this:

    def get_context(self, name, value, attrs):
        attrs = (attrs or {}).copy()
        attrs.pop('maxlength', None)
        return super().get_context(name, value or self.initial, attrs)

And in terms of testing we should simply check if maxlength keyword is not part of <select> html tag. We could do this in PhoneNumberPrefixWidgetTest class like this:

    def test_maxlength_not_in_select(self):
        widget = PhoneNumberPrefixWidget()
        html = widget.render(name="widget", value=None, attrs={"maxlength": 32})
        self.assertIn('<select name="widget_0">', html)

I've done my own tests and W3C validator was happy with the changes.

Regards,
Andrzej

@amateja amateja self-assigned this Mar 11, 2022
phonenumber_field/widgets.py Outdated Show resolved Hide resolved
phonenumber_field/widgets.py Show resolved Hide resolved
@sterliakov
Copy link
Contributor Author

Hello @amateja, I apologize for such a late response. I applied changes requested changes.

Please tell if I can be useful for this or other issues - I'm willing to help:)

@amateja amateja self-requested a review March 22, 2022 19:41
@amateja amateja merged commit 1df29ec into stefanfoulis:main Mar 23, 2022
@amateja
Copy link
Collaborator

amateja commented Mar 23, 2022

Thanks for your contribution.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants