Remove Format removes SCAYT spans

[Artur Formella]

Steps to reproduce:

• Turn on SCAYT

• Insert random text (i.e. ghkfghc fc rftguc ftyiv gyi ftyi tyf )

• Select this text

• Click Remove Format button

Result:

SCAYT underline was removed

[Alfonso Martínez de Lizarrondo]

Proposed patch

[Alfonso Martínez de Lizarrondo]

The patch will refresh the state after execution of the remove format command.

[WebSpellChecker.net]

patch is submitted in #5145

[Frederico Caldeira Knabben]

It's ok to indicate that this one is supposed to be fixed by another tickets, but it should be closed only after confirming the fix one the relative ticket gets closed.

[WebSpellChecker.net]

4125_2.patch has only related to current ticket changes.

[Alfonso Martínez de Lizarrondo]

#5467 has been marked as dup

[WebSpellChecker.net]

We have noted that "Remove Format" does not remove links. Does it make a sense to add some filtering ability for this method like in elementspath to ignore some specific markup, e.g. SCAYT markup?

[Alfonso Martínez de Lizarrondo]

What's wrong with the first patch? I don't even understand why you proposed to use the beforeCommandExec event with a timeout.

[WebSpellChecker.net]

Refresh method should be called only when other plugins have finished their changes in markup. Calling setTimeout with a minimum delay, made the interpreter schedule the script block to be executed once the current call stack is empty so the execution of the scayt method will run after editor finish execution of it's own code.

Call refresh method solves the problem, but it has a side effect that scayt markup may blink and also makes work of browser little bit slower by invoking scayt code, so the better way from our perspective is to move markup processing to the “remove format” plugin.

One solution we see is to register “scayt markup” as a service one to avoid “remove-format” plugin removing “scayt markup”. Such decision was implemented in “element path” plugin with registering custom filters. Is it possible to implement custom filters for “remove-format”?

Another solution we see is to add “_cke_realelement” attribute to all scayt’s nodes. It seems, it will solve the problem automatically, but we expect that it will affect scayt core performance, thus browser responsiveness. Nonetheless we need an API from CKEditor, which will give us a correct attribute name, which can be used to mark scayt nodes as a service CKEditor markup.

[Alfonso Martínez de Lizarrondo]

So instead of using the beforeCommandExec it will be safer to use afterCommandExec, right?

We are talking about the "remove format" command, so it's expected that everything might blink as soon as the styles and format goes away. This is a command that people will use scarcely so I wouldn't worry about the performance.

Adding a "_cke_realelement" attribute won't work for the removeFormat (it's checked only for img) and it could have unexpected results in other parts of the code.

I would like to hear other opinions about adding custom markup or special filters for the remove format command (please, note that the situation for the "element path" is different because in that case the nodes will remain and the only solution is to skip them. Here we can do a normal clean up and then do the spellcheck again or introduce a mechanism to skip them.

[Garry Yao]

Please call for discussion by putting on keyword 'Discussion'.

This is a command that people will use scarcely so I wouldn't worry about the performance.

True, if we're talking of SCAYT only, we don't need the remove format filters.

I would like to hear other opinions about adding custom markup or special filters for the remove format command.

I would vote for the filtering mechanism as it could be anticipated that we'll have further use cases later, e.g. the selection marker proposed at #4666; Any inline element with contentEditable="false" later.

[WebSpellChecker.net]

4125.patch is a draft, which makes removeFormat plugin to ignore scayt markup likewise any fake elements.

[Garry Yao]

Introducing filters for removeformat plugin.

[Alfonso Martínez de Lizarrondo]

In the filter function there's no need to store the result variable, it can be changed to

if ( filters[ i ]( element ) == false ) 
    return false;
}
return true;

Also, there are no comments explaining how and why it works this way and the availability of editor._.removeFormat.filters

[Garry Yao]

Let's introduce it as an editor method for easy documentation. (It's extremely hard for JSDocToolkit to understand our private fields, aka '-' )

[Alfonso Martínez de Lizarrondo]

The removeFormatFilters var is no longer needed in the SCAYT plugin, and there's still not a single comment in the code added, at the very least the filter function should be explained a little.

What about this description for the addRemoveFormatFilter docs?:

/**
 * Adds a function that will be called during the execution of the 
 * <b>removeFormat</b> command.
 * If it returns false, that element won't be removed.
 * @since 3.3 
 * @param {Function} func The function to be called, it will be passed 
 *		a {CKEDITOR.dom.element} element to be test.
 * @example 
 *  // Don't remove empty span 
 *  editor.addRemoveFormatFilter( function( element ) 
 *              { 
 *                      return !( element.is( 'span' ) && CKEDITOR.tools.isEmpty( element.getAttributes() ) ); 
 *              }); 
 */ 

[Garry Yao]

The documentation is already presented when defining 'addRemoveFormatFilter ', isn't that enough?

[Alfonso Martínez de Lizarrondo]

I think that a simple comment at line 119 wouldn't hurt anyone and can simplify understanding and remembering what that code does 6 months from now.

The description for the addRemoveFormatFilter is wrong because it states that the default for that function is an array, but that's the default value for the internal property. I tried to readjust the description with my proposal to match what that function does and make it simple for the people to understand how to use it.

[Garry Yao]

Ok, thanks for the advise.

[Garry Yao]

Fixed with [5460].