neutronX / django-markdownx

Comprehensive Markdown plugin built for Django
https://neutronx.github.io/django-markdownx/
Other
863 stars 153 forks source link

Check for RESIZABILITY_ATTRIBUTE presence before calling match() #90

Closed nanocell closed 6 years ago

nanocell commented 7 years ago

This fixes an issue where, in the absence of the editor element's resizability attribute, a .match() call would be called on an undefined value. This fix simply checks for the presence of of the attribute with properties.editor.hasAttribute(RESIZABILITY_ATTRIBUTE) prior to executing match().

The above error occurs when MARKDOWNX_EDITOR_RESIZABLE is set to False and any update is made to the editor content.

xenatisch commented 7 years ago

Since there is not issues posted on this and just so we're clear; are you saying that when False, the attribute is not set on the HTML element, or are you saying that the JavaScript does not recognise it?

nanocell commented 7 years ago

My apologies for being unclear. When the MARKDOWNX_EDITOR_RESIZABLE setting is set to False, the attribute (data-markdownx-editor-resizable) is not set on the element, as expected, (since it shouldn't automatically resize) but this results in the following JavaScript error:

Uncaught TypeError: Cannot read property 'match' of null
    at _initialize (markdownx.js:377)
    at new MarkdownX (markdownx.js:509)
    at markdownx.js:576
    at Array.map (<anonymous>)
    at markdownx.js:572
    at markdownx.js:534
    at Array.map (<anonymous>)
    at HTMLDocument.ready (markdownx.js:534)

The PR simply adds an additional check to ensure that the attribute (properties.editor.getAttribute(RESIZABILITY_ATTRIBUTE)) is non-null before attempting to call match().

xenatisch commented 7 years ago

This is very bizarre. Implementation of that attribute is independent of its value (see the code for fields). Additionally, the attribute has a default value (see the code for settings).

How is that possible for the resulting element (i.e. Django field) not to have the attribute? Unless of course we're talking about a custom field. Is that where you noticed the problem? If so, you're making an important point here that would essentially be applicable to all other attributes too.

To address that, however, I think we should perhaps attempt to look at it from a rather different perspective. Whilst your solution is absolutely valid (and I will merge it for the next release), I think the best way to approach this is to set the attribute using JavaScript when it is absent. That is as opposed to checking for its existence every time we call the function. This is primarily a performance consideration to optimise the response... but I'll do this at some point in the future as it is not an urgent issue.

nanocell commented 7 years ago

So, I did a little more digging and you're right. The django-markdown codebase does push those attributes through to the widget, irrespective of their values. The snag lies in Django (we're using 1.11) in one of the widget template files:

# templates/django/forms/widgets/attrs.html

{% for name, value in widget.attrs.items %}{% if value is not False %} {{ name }}{% if value is not True %}="{{ value }}"{% endif %}{% endif %}{% endfor %}

Note that Django will completely omit an attribute if its value is False. So, I suppose the alternative to doing 'hasAttribute()' checks would be to coerce the MARKDOWNX_EDITOR_RESIZABLE variable to string before sending through to Django's Textarea widget.

Having said that, the hasAttribute() check already gets called in the intialization function so it only gets called once per editor element (not on every update). So, performance-wise it should be fine?

Also, I'm totally open to implementing the fix differently and opening a new PR if you prefer. It's your call ;)

adi- commented 6 years ago

I guess new PR will be good.