Item5858: WysiwygPlugin needs to be more careful about what it changes

A recent bug Bugs:Item5828 reported a situation where the WysiwigPlugin destroys topics irrecoverably. There is discussion in that topic on whether this problem could be avoided by another plugin being written differently. While that probably is correct, that bug revealed some fundamental issues with WysiwygPlugin that could lead to other problems:

1. The WysiwygPlugin indiscriminantly changes text from HTML to TML in the afterEditHandler, whether that text has been produced by it earlier or not. I believe that the plugin should mark (e.g., by begin/end symbols) the text it converts into HTML and only convert back to TML that portion of the text. Other text could have been added to the topic by other plugins and must not be converted.
2. The WysiwygPlugin cannot be applied in the normal Plugin ordering. It must be the last plugin that runs before a textarea is edited, and the first plugin that cleans up after editing the text area. Using plugin order to resolve interactions between plugins involving the WysiwygPlugin may not work as it might leave the before or after edit in the wrong order.

I believe resolving (1) is critical. WysiwygPlugin must be more defensive (careful) on where it applies.

-- TWiki:Main/ThomasWeigert - 31 Jul 2008

No; it has no idea whether the text comes from Textarea, from Texas, or from Texel. It does what an afterEditHandler is supposed to do; postprocesses text that is passed to the save script. The solution is to ensure that the edited text is passed to the save script, and move your prepend/append to the right place in that process.

-- TWiki:Main.CrawfordCurrie - 31 Jul 2008

You cannot expect other processes to code for the WysiwygPlugin. Sending a string into save assuming it is TML is perfectly fine for a plugin, as that is the official TWiki langauge. That the WysiwygPlugin has secretely exchanged the TML for HTML and that it then messes with it later without paying attention to what it does is the WysiwygPlugin's problem. It has no business changing text wantonly.

CC, you can fix this easily. Instead of just sticking your magic secret word in the topic, stick begin and end markers around it and change only the text within that markers.

-- TWiki:Main.ThomasWeigert - 31 Jul 2008

I am downgrading this to Normal. I am now 24 hours from releasing 4.2.1 and this is not going to be fixed or even attempted fixed in 4.2.1.

This bug item open a whole principle discussion on how Wysiwyg should work. Not an urgent type bug. Not even for 4.2.2

-- TWiki:Main.KennethLavrsen - 31 Jul 2008

Ken, I disagree. This bug destroys topic text in a way that it has to be reconstructed painstakenly. Anything that destroys a user's content is a serious bug and needs to be handled urgently.

The discussion above may be about how WysiwygPlugin should work, but the essence is serious:

If you edit a topic that uses SectionalEditPlugin or any other plugin that creates topic text during edit or otherwise before the WysiwygPlugin operates on the portion of the text it had first translated into HTML, that topic will be destroyed. The topic will loose critical newlines and protected characters will be translated into their actual printed form.

That is a serious issue we cannot hoist on our users. Release in a day is not an excuse, I think. One can always postpone the release.

-- ThomasWeigert - 01 Aug 2008

Maybe Thomas is right that the release should postponed for a few weeks as if Bugs.Item5819 is correct 4.2.1 would fail on some systems where 4.2.0 didn't. That doesn't feel right.

Maybe Kenneth is right too, that we don't need sectional editing working right now, as it didn't work for much to long.

I guess the users definitely want wysiwyg and sectional editing working properly together and unicode support done right. But I fear they will have to wait for 5.x. Let's do it step by step. Urgent bugs first. Maybe TTC can/should decide what's urgent. -- Just my 2c.

-- TWiki:Main.FranzJosefGigler - 01 Aug 2008

Thomas, I understand where you are coming from, but I think you need to look at the broader picture. The plugins architecture is designed such that afterEditHandlers are invoked in a specific place in the flow to perform transformations of the edited text. You have zeroed in on the WysiwygPlugin but in fact the same pattern can re-emerge with any other plugin that post-processes edited content.

For example, consider how you would handle a (fictional) EncodingPlugin that converted the encoding of the topic text for the duration of the edit session, and then converted it back to the site encoding in the afterEditHandler. If this plugin were used alongside EditContrib, then again you have the situation that you have stuck together text in one encoding with text in another encoding, and are now expecting the EncodingPlugin to sort out the mess. Note that similar interaction issues would exist between the EncodingPlugin and the WysiwygPlugin, and in this case it could not be resolved by begin/end markers so that is IMHO not a useful solution.

This is not a problem specific to WysiwygPlugin; it is a more general problem with the plugins architecture that happens to be highlighted by your chosen implementation of the EditContrib. We should be aiming to sort out these kinds of situations by refining the mechanisms that TWiki already provides for solving these sorts of problems (such as the plugins order) and not tacking on one-off workarounds.

In terms of resolving your issue with the EditContrib, I have already suggested to you how the existing plugins handler architecture might be used to resolve your issue, by removing the bin/savesection script and implementing the functionality in the afterEditHandler.

I think there is a strong case, highlighted by this issue, for refactoring the mechanisms used to control the plugins pipeline (as defined by the {PluginsOrder}). This pipeline is fundamentally flawed in that it only provides a single ordering, such that beforeEditHandler invocation of plugins A, B and C is ABC, but afterEditHandler invocation is also ABC. I see the requirement for such a refactoring as important, and something that needs to be addressed really soon, but I do not consider it a release blocker. I certainly do not think there is a case for hacking the WysiwygPlugin to address this one-off specific case for the EditContrib.

I have raised a feature request in Codev where we can continue this discussion.

-- CrawfordCurrie - 01 Aug 2008

CC, I agree with everything you say, except the claim that the WysiwygPlugin is innocent.

There is one thing, I think that the WysiwygPlugin does wrong, and that really causes all the problems. It does not mark the text that it converts into HTML and then indiscriminantly changes text to TML. It should only change the text it first marked.

This is not a one-off but I think sound defensive programming. You should not assume things that you cannot control. The WysiwygPlugin assumes that the whole topic text (if there is the magic word) has been translated into HTML and needs to be translated back. That assumption cannot be guaranteed. More importantly, it does not need to make this assumption. It can easily assure that it only translates back to TML content marked as previously translated into HTML.

It appears to be an easy fix (maybe I am wrong), so I do not understand the resistance here....

-- ThomasWeigert - 01 Aug 2008

Well, as I have tried to explain, IMHO the WysiwygPlugin is behaving correctly, and I see no need to change it. The only reason the WysiwygPlugin is presented with anything other than 100% HTML in this case is because another extension - the EditContrib - is puliing a fast one (the savesection script). Also:

1. Text for conversion already is marked, by the secret ID. I am extremely reluctant to add more "secret markers" to the text - the risk of overlap with genuine content is already too high.
2. Adding such IDs in the text increases the dependence between the 'before' and 'after' of the editing process. An interactive edit isn't the only way text can arrive at the save script. Increasingly REST-type functions are using save.
3. Even if I felt it was the right thing to do, I don't make such modifications without adding unit tests. Easy fixes have in the past been a plague on the core and bundled plugins.
4. Anything that risks introducing bugs into any core component (like WysiwygPlugin) the day before a release has to have an incredibly strong justification.

You might be right that it would be an enhancement to support subset conversion in topic text. Right now I can't think of an application for it, but that doesn't mean there isn't one. But I'll reflect your words back to you w.r.t modifying the EditContrib to use an afterEditHandler or beforeMergeHandler viz. It appears to be an easy fix (maybe I am wrong), so I do not understand the resistance here....

-- CrawfordCurrie - 01 Aug 2008

As I said, I think you are correct about the comments re EditContrib and am changing that one. However, I think you are also completely wrong about your comments regarding the innocence of WysiwygPlugin. I think it is bad practice to make the assumption that this plugin has complete control over the topic. It does not. I therefore still think this is a problem, albeit I have rewritten the EditContrib as discussed.

-- ThomasWeigert - 01 Aug 2008

Anyway, I have rewritten SectionalEditPlugin to leverage afterEditHandler, but there is still a problem, reported as Item5861. The above messed up text is the result of this problem.

-- ThomasWeigert - 01 Aug 2008

I have restored back to rev 9 as rev 10 was goofed up

-- KennethLavrsen - 01 Aug 2008

Rev 10 shows an example of how this plugin messed up the text...

-- ThomasWeigert - 01 Aug 2008

Rev 10 shows how develop.twiki.org messed up things - it more the correct description.

Bugs web is currently running on the TSA code which is very unstable. You cannot count on what you see here on bugs.

-- TWiki:Main.KennethLavrsen - 01 Aug 2008

Changed priority to normal as release is coming up.

-- ThomasWeigert - 01 Aug 2008

Reading this being in 2010 I think the many other bug reports covers what is needed and much has been addressed since. This reports is too generic to ever get closed. So I no action it now.

-- KennethLavrsen - 07 Jan 2010