Added possibility to pass custom widget with editor configuration through widget attribute on RichTextField and RichTextBlock

[HitDaCa]

This pull requests replaces the original pull request #2139. I had wrongly rebased to latest master before adding later changes which made code compare unusable (trying to remove rebase closed the original request).

Following the advice from @m1kola & @JoshBarr #2139 (comment), RichtTextFields and StreamField RichTextBlocks now accept a widget attribute. By default, if none is passed, the standard RichTextArea widget is used.

Following @JoshBarr comment about easily configuring various RichText widgets in future, I have also moved the immutable parts of the RichTextArea into it's own abstract base class. This allows easier creation of future RichTextWidget variations.

from wagtail.wagtailcore.fields import RichTextField
from wagtail.wagtailadmin.edit_handlers import FieldPanel, StreamFieldPanel

from wagtail.wagtailcore.blocks import RichTextBlock
from wagtail.wagtailcore.fields import StreamField

from wagtail.wagtailcore.fields import RichTextArea

simple_config = {
    'halloheadings': {
        'formatBlocks': ['p', 'h1']
    },
    'halloformat': {
        'formattings': {
            "bold": False,
            "italic": False,
        },
    },
    'hallowagtaildoclink': {},
    'hallolists': {
        "lists": {
            "ordered": False,
            "unordered": False
        }
    },
    'hallowagtaillink': {},
    'hallorequireparagraphs': {}
}

complex_config = {
    'halloheadings': {
        'formatBlocks': ['p', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6']
    },
    'halloformat': {
        'formattings': {
            "bold": True,
            "italic": True,
        },
    },
    'hallowagtaildoclink': {},
    'hallolists': {
        "lists": {
            "ordered": True,
            "unordered": True
        }
    },
    'hallowagtaillink': {},
    'hallorequireparagraphs': {}
}

class BookPage(Page):
    book_text = RichTextField(widget=RichTextArea(editor_config=simple_config))
    page_content= StreamField([
        ('excerpt', RichTextBlock(required=False, widget=RichTextArea(editor_config=simple_config))),
        ('paragraph', RichTextBlock(required=False, widget=RichTextArea(editor_config=complex_config)))
    ])    
    content_panels = Page.content_panels + [
        FieldPanel('book_text', classname="full"),
        StreamFieldPanel('page_content')
    ]

I have also updated the RichTextField documentation and RichTextBlock documentation to explain this new attribute addition.

I have added a test for the new attribute.

[m1kola]

I think would be better to set all field options to self.field_options (as already done with help_text and required).

If you decided to do so, field method can look similar to this:

    @cached_property
    def field(self):
        field_options = self.field_options.copy()

        if 'widget' not in field_options:
            from wagtail.wagtailcore.fields import RichTextArea
            field_options.update({
                'widget': RichTextArea
            })

        return forms.CharField(**field_options)

Another good solution is set all field options to properties in RichTextBlock, but this is not widely used in Wagtail: as I see only ChooserBlock done this way. If you decided to go this way, you can deal with widget like this done in Django. Refs:

• https://github.com/django/django/blob/1.9.1/django/forms/fields.py#L59
• https://github.com/django/django/blob/1.9.1/django/forms/fields.py#L103

[m1kola]

Second solution is for fields, but we can apply same idea for blocks.

[HitDaCa]

I agree with your first proposed approach, this pattern is used mostly throughout other specific block types and upholding this consistency makes most sense to me.

As to your proposed solution, I'd prefer using the standard get method of the dictionary. This better covers all cases of a non existing key as well as an existing key returning None. The naming conventions in the below code now also follows used patterns.

def field(self):
    field_kwargs = self.field_options.copy()
    # check if a custom widget has been set or use default
    if not field_kwargs.get('widget'):
        from wagtail.wagtailcore.fields import RichTextArea
        field_kwargs.update({'widget': RichTextArea})
    return forms.CharField(**field_kwargs)

I've update the branch with this solution. I'm happy for your feedback if you see room for improvements.

[m1kola]

Could you, please, check in tests also following:

• That RichTextBlock.field method returns CharField with default widget, if custom widget is not provided
• That RichTextBlock.field method returns CharField with custom widget, if custom widget is provided
• That RichTextArea.render_js_init returns expected result when editor_config is provided
• That RichTextArea.render_js_init returns expected result when editor_config is not provided
• That RichTextField.formfield returns Field with default widget, if custom widget is not provided
• That RichTextField.formfield returns Field with custom widget, if custom widget is provided

[m1kola]

Other than the above comments, this looks good! Thanks, @HitDaCa.

@JoshBarr, please, take a look at this.

[m1kola]

@HitDaCa, if you need any assistance with this PR, feel free to let me know.

[HitDaCa]

@m1kola, thanks for all the feedback! I'll read through each of your suggestions in detail tomorrow morning and give my thoughts on every point. Thanks.

[m1kola]

@HitDaCa, thank you for patience! Looks great for me!

@gasman, can you, please, set same milestone as in #2139 and required labels? This PR replaces #2139.

[ababic]

Guys, thanks to all of you for putting in the work on this. It looks like a really valuable addition, and i'm excited to use it!

[gasman]

This is really neat! Unfortunately I've encountered a snag - passing a widget property to RichTextBlock breaks the resulting migration, because RichTextArea (and widgets / form field objects in general) can't be serialized into the migration. (See http://docs.wagtail.io/en/v1.3.1/topics/streamfield.html#streamfield-definitions-within-migrations for some discussion of the issues with StreamField in migrations.) This could probably be fixed directly by implementing a deconstruct method on RichTextArea, but I'm not sure I like that approach - it makes RichTextArea a special case as the only widget type that will work, whereas the API here suggests that it should be possible to pass any widget. Rather, I'd take this as a hint from Django that we shouldn't really be embedding form-related objects into a model-level definition. (We've encountered this before, in the design of FieldBlock - it's the reason we have to subclass FieldBlock for each individual field type, rather than being able to write something like FieldBlock(field=forms.CharField(max_length=255)) into the StreamField definition.)

I'm not sure what the best remedy is here. We could of course go back to the original plan of passing the hallo_config dict directly to RichTextBlock, but now I'm wondering if that's bad design - it occurs to me that this might be the first place where we have a StreamField block parameter that exists purely to specify the behaviour of the editor, rather than to define the data format. Maybe this kind of configuration ought to be separated out of the model-level StreamField definition, just as Django separates model fields and form fields?

[gasman]

Having thought about this some more, I've reached the conclusion that, whatever the parameter to RichTextField / RichTextBlock might be, it should not describe a particular rich text widget's behaviour. This is for several reasons:

• The standard Django objects for this purpose (i.e. form fields / widgets) cannot be serialized for migrations.
• RichTextField and StreamField are ultimately model fields, describing a database-level data representation, and details about the form representation do not belong there. Just as Django's own APIs enforce a separation between model fields and form fields, we should find a better place to put those purely 'cosmetic' options, if we need them. (For RichTextField, the better place is probably the 'panels' definition; for RichTextBlock, we probably don't have any suitable place at the moment.)
• The ability to swap in different rich text backends (e.g. TinyMCE - see Refactored editor #1521) is a very commonly requested feature, and one we'll probably work on very soon. Any implementation that only caters for hallo.js is only going to be a stopgap solution at best.

I think the answer is for the parameter to define a list of "features" - things that this particular rich text field is allowed to contain, such as paragraphs, bold, italics, other inline markup such as <code>, page links, document links, images, embeds, ULs, OLs. Something like:

body = RichTextField(features=['p', 'b', 'i', 'h2', 'page-link', 'doc-link'])

(In practice we'd probably want a site-wide common set of features - or perhaps several standard 'palettes', for different use-cases - and a mechanism for adding/subtracting from that list on a per-field basis, like django.forms's fields/exclude or wagtail/rfcs#5.)

The key distinction here is that we're saying "here are some conditions that the data must conform to" (and can potentially enforce that at the whitelisting/validation stage), not "this is how rich text widget X should behave". It's up to each rich text backend to look at the feature list and configure itself accordingly (adding/removing toolbar buttons, or whatever) to present that feature set to the user as best it can. Different rich text backends will have different capabilities, of course, and that's fine - it's up to the developer to ensure that they've set up a rich text backend that actually supports the features they've specified. For example, if a field is defined as RichTextField(features=['p', 'table']), but they're using the hallo.js backend which doesn't support table editing, then it'll simply ignore the 'table' feature and editors won't be able to put tables into their pages.

There's a lot more thinking to be done to define what a 'feature' is here - it's not enough to just come up with a standard list of identifiers and expect each rich text backend to know what they mean, because that would result in massive duplication of code (e.g. each backend has to know that 'doc-link' involves launching /admin/documents/chooser/ as a modal and doing a particular thing with the JSON result) and wouldn't be extensible enough (in order to add a new link type or allowed HTML tag, you'd have to hack the individual rich text backends). Most likely, we'd identify several general classes of feature (e.g. "ones that wrap a span of text in an HTML tag", "ones that insert a link based on the result of a modal") and have a mapping of feature identifiers to full descriptions, e.g.:

'doc_link': {
    'feature-type': 'LinkFromModal',
    'modal-url': '/admin/documents/chooser',
    'icon': 'document'
}

As I say - needs more thinking...

[kunambi]

It's 4 months since last activity. Will the feature of having RichTextField-specific options eventually reach release?

TYIA

[thibaudcolas]

Hey @kunambi, Dave and Josh were working on this while employed with me at Springload and have both moved on to other jobs since then. @gasman has expressed reserves with the current approach, so there needs to be more work on the PR, but no one has intentions to do this work as far as I know.

I will close this PR, should someone want to take it from there they should make a new one and address the feedback above.

Finally, if you're interested in rich text configuration Springload has been working on a new text editor that solves this use case for us. It's not as well documented, or stable, as I'd like it to be but we do already use it on production projects. Check out https://github.com/springload/wagtaildraftail, and #2409.

[gasman]

Now implemented (with an editor-agnostic features parameter as originally proposed above: #2175 (comment)) in #3736.