Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Apply {% localize off %} on <img> element #107

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

MacLake
Copy link
Contributor

@MacLake MacLake commented Dec 2, 2021

Description

At least {{ picture_size.size.0 }}px in line 32 of the template picture.html
can have a decimal comma in some cases which causes the image not to be
displayed in Firefox 95.0b8 and Chrome 96.0.4664.45.

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

At least {{ picture_size.size.0 }}px in line 32 of the template picture.html
can have a decimal comma in some cases which causes the image not to be
displayed in Firefox 95.0b8 and Chrome 96.0.4664.45.
@marksweb
Copy link
Member

marksweb commented Dec 2, 2021

It'd be good to have a test for this to prevent regression.

What you'd need to do in a test is create a template that tried to localize the value.

    def test_bleaching(self):
        """ Test that unsafe tags are sanitised """
        context = Context(
            {'some_unsafe_content': '<script>alert("Hello World!")</script>'},
        )
        template_to_render = Template(
            '{% load bleach_tags %}'
            '{{ some_unsafe_content|bleach }}'
        )
        rendered_template = template_to_render.render(context)
        self.assertInHTML(
            '&lt;script&gt;alert("Hello World!")&lt;/script&gt;',
            rendered_template
        )

I have a feeling you'll also need to define the format of the localization. I'm not so sure on how best to do that - would need some googling.

@MacLake
Copy link
Contributor Author

MacLake commented Dec 3, 2021

Well, we already have the standard template djangocms_picture/default/picture.html which is good enough for differentiating between decimal comma (leading to a non-showing picture) and the correct decimal point. What we need is a picture that causes the srcset to be created, I think it happens only for large pictures.
The thing that is harder for me to figure out is: Where should I set the language? Probably we should loop e. through ['en', 'de'] or whatever. There are many places with language definition, and it seems to be all English (en). Once I know that I only need to check response.content for decimal points (or comma if it fails).

The only possible danger for a regression is that there is a number or date in the <img> element that should be displayed in a localised format. The only human readable snippet is the alt text, which is not a number or date. The rest is numbers which must not be localised.

@marksweb
Copy link
Member

marksweb commented Dec 4, 2021

My reason for mentioning the template in the test was that it seems like this change forces localization to be turned off in the template, so you might want a test with a different template where the content was localized to prevent regression.

In terms of setting the language, you can use override_settings

class LanguageTestCase(TestCase):

    @override_settings(LANGUAGE_CODE='de')
    def test_language(self):
        ...

The test helpers are able to create images, just take a look at the model tests and then the helpers are here; https://github.com/django-cms/djangocms-picture/blob/master/tests/helpers.py

@MacLake
Copy link
Contributor Author

MacLake commented Dec 6, 2021

Okay, I will try, but I can only spend a limited amount of time on it.

I think it is more important to reproduce the error I encountered in the test, i. e. a decimal comma 800,0px instead of 800.0px and then get rid of it with {% localize off %} or template filter |unlocalize.

Anyway, some things to point out:

  • We really need to avoid localised numbers in any code that uses template variables. Integers seem to be safe, but floating point numbers definitely are not. So far I only had problems in JavaScript code, but apparently also in HTML attributes they may happen.

  • Language text translation and localised number formats are different things. I tried activate('de') and @override_settings(LANGUAGE_CODE='de') (what you proposed) in test_plugin_structure() just for trying to reproduce the decimal comma, but didn’t succeed so far (see https://github.com/MacLake/djangocms-picture/tree/test).

  • In my template change I only applied {% localize off %} on the <img> tag, not at the whole template. We could even reduce it to parts of it, or just use |unlocalize.

@marksweb marksweb merged commit 8a3836d into django-cms:master Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants