Support copy-paste in rich-text fields and fine-tune editing window#3446
Conversation
|
|
||
|
|
||
| class PageForm(JanewayTranslationModelForm): | ||
| class PageForm(BleachableForm, JanewayTranslationModelForm): |
There was a problem hiding this comment.
The order of the inheritance would be correct in most cases, but this is an exception:
Since the MRO in python3 relies on C3 Linearization, this order will cause the methods of django.forms.Form to run before django.forms.ModelForm. Since this is a ModelForm, not only is this order wrong, we don't want to bring the execution path of django.forms.Form over at all.
There are two solutions:
- Base the
BleachableFormon the common ancestor offorms.Formandforms.ModelFormwhich isdjango.forms.BaseForm. You can then construct your forms asclass Foo(BleachableForm, ModelForm)andclass Bar(BleachableForm, forms.Form) - Use a Mixin that self contains the behaviour of bleach, and then create two differently branching form classes:
classs BleachableFormMixin(forms.BaseForm):
def save(self, *args, **kwargs):
[..]
class BleachableForm(BleachFormMixin, forms.Form): pass
class BleachableModelForm(BleachFormMixin, forms.ModelForm): pass
There was a problem hiding this comment.
Thanks for catching this and explaining the solution--commit incoming
| ) | ||
| is_markdown = models.BooleanField(default=True) | ||
| edited = models.DateTimeField(auto_now=timezone.now) | ||
| support_copy_paste = models.BooleanField( |
There was a problem hiding this comment.
Can we add this to a BleachModelMixin instead? Not only makes the implementation DRY, it also self-contains the logic into a model that self-documents the existance of the support_copy_paste attribute.
| name='support_copy_paste', | ||
| field=models.BooleanField(default=True, help_text='Turn this on if copy-pasting content into rich-text fields from a word processor or using the toolbar to format text. Turn it off only if you are editing HTML and CSS using the code view.'), | ||
| ), | ||
| migrations.RunPython(preserve_old_formatting, reverse_code=migrations.RunPython.noop), |
There was a problem hiding this comment.
One neat way of doing this without the custom logic is to set default=False on the AddField instruction above
followed by an AlterField instruction here that sets default=True.
your implementation is valid too though
| ] | ||
|
|
||
| # Which HTML attributes are allowed | ||
| BLEACH_ALLOWED_ATTRIBUTES = ['href', 'title', 'src', 'target'] |
There was a problem hiding this comment.
I think we need to support class as well to make use of custom classes provided by each theme's CSS
There was a problem hiding this comment.
I excluded class in order to remove cruft from Microsoft Word when bleaching. There is no way to do that unless we disallow class. As for site managers using custom styling, we've provided the "Support copy paste" boolean for them to turn off bleaching on a case-by-case basis. Is that OK with you?
| abstract = BleachField( | ||
| blank=True, | ||
| null=True, | ||
| help_text=_('Please avoid pasting content from word processors as they can add ' | ||
| 'unwanted styling to the abstract. You can retype the abstract ' | ||
| 'here or copy and paste it into notepad/a plain text editor before ' | ||
| 'pasting here.') | ||
| help_text=_('Copying and pasting from word processors is supported.'), |
There was a problem hiding this comment.
Putting this comment here since I can't point at untouched lines: recently it surfaced that we need to bleach the article title field, since while not a rich-text field, we still render it as safe. This turns the article title into a vector for JS injection
There was a problem hiding this comment.
OK, I've changed the title field to a BleachField
|
We discussed user error prevention for this feature this morning, and here is some additional work we are going to do to CMS pages to make sure press managers can undo saves that implemented unwanted changes. User StoryAs a tech support person, I might implement custom CSS or advanced HTML for a press website, but I might be the only one who knows that code is saved in the CMS. Then, as a press manager who does not know about HTML and CSS, I may not know that the bleach boolean could wipe out the custom styling my tech support person put in. I might edit a page by pasting in a sentence or two, and then check “Support copy paste” because I copy-pasted. This would result in the previous custom styling work being wiped out. We should give users a way to undo the save that caused the unwanted result. Solution: page versions for CMS
|
|
This also looks good: https://django-simple-history.readthedocs.io/en/latest/ Thanks for suggesting it @mauromsl. |
For future reference, I've just tried implementing UPDATE: I got around the translation issue, sort of, but the resulting migrations are quite messy and redundant. You can see what I mean on this work-in-progress branch: 1481-bleach...1481-bleach-django-simple-history Which way should we implement this for the 1.5.1 release? |
|
By using django-admin-history, we give users a pathway to revert unwanted changes. A new button appears beside the Save button on the forms for Page, NewsItem, and Repository, since those are the three that are now optionally bleachable. Staff members can follow direct links to the Django admin page for a version, where they have the option to revert it: Editors are not given admin links but are instead prompted to ask staff for help reverting a past version: |
joemull
force-pushed
the
1481-bleach
branch
from
542c96d to
f1bab09
Compare
Signed-off-by: Mauro MSL <[email protected]>
joemull
force-pushed
the
1481-bleach
branch
from
f1bab09 to
99ce999
Compare
3 participants
Implementation notes
I opted to use
django-bleachselectively on a range of rich-text fields. I created a default set of allowed tags and attributes in Janeway global settings, prioritising semantic markup.I implemented it in several different ways, sometimes in the model, sometimes in the form, and sometimes with an option to turn it off:
supports_copy_pastefield that controls whether the content is bleached. Existing pages are set not to be bleached via a custom migration function, so that current styling data isn't lost. New pages are bleached by default but users can still turn it off.I also took this opportunity to fine-tune the Summernote toolbar (so it matches the allowed tags) and add syntax highlighting and soft line wrapping for the codemirror window.
Drawbacks
I could not make this work for all rich-text fields with the time available, because some of them are governed by JQTE and we would need to standardize these to Summernote before fixing them. These include the custom HTML block homepage element, most notably.
Alternatives considered
The
django-bleachproject depends onbleach, which depends onhtml5lib, which has recently been deprecated. A possible replacement is under construction inammonia, its Python bindingnh3, and Mark Walker's Django helperdjango-nh3. The Django helper is too new to use immediately but it is written by the same author asdjango-bleach, so moving to it in a year or two might be a good idea.Closes #1481
Closes #2282