Opened 8 years ago

Closed 7 years ago

#4125 closed Bug (fixed)

Remove Format removes SCAYT spans

Reported by: Artur Formella Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.3
Component: UI : Spell Checker Version: 3.0
Keywords: Confirmed Review+ Cc:

Description

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

Attachments (7)

4125.patch (1.4 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Proposed patch
4125_2.patch (738 bytes) - added by WebSpellChecker.net 7 years ago.
4125_3.patch (1.1 KB) - added by WebSpellChecker.net 7 years ago.
4125_4.patch (3.2 KB) - added by Garry Yao 7 years ago.
4125_5.patch (3.9 KB) - added by Garry Yao 7 years ago.
4125_6.patch (3.8 KB) - added by Garry Yao 7 years ago.
4125_7.patch (4.1 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (29)

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4125.patch added

Proposed patch

comment:1 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added
Owner: set to Alfonso Martínez de Lizarrondo
Status: newassigned

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

comment:2 Changed 7 years ago by WebSpellChecker.net

Resolution: fixed
Status: assignedclosed

patch is submitted in #5145

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: closedreopened

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.

comment:4 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? removed

Changed 7 years ago by WebSpellChecker.net

Attachment: 4125_2.patch added

comment:5 Changed 7 years ago by WebSpellChecker.net

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

comment:6 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Milestone: CKEditor 3.xCKEditor 3.3

#5467 has been marked as dup

comment:7 Changed 7 years ago by 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?

comment:8 Changed 7 years ago by 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.

comment:9 Changed 7 years ago by 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.

comment:10 Changed 7 years ago by 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.

comment:11 Changed 7 years ago by Garry Yao

Keywords: Discussion added

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.

Changed 7 years ago by WebSpellChecker.net

Attachment: 4125_3.patch added

comment:12 Changed 7 years ago by WebSpellChecker.net

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

Changed 7 years ago by Garry Yao

Attachment: 4125_4.patch added

comment:13 Changed 7 years ago by Garry Yao

Keywords: Confirmed Review? added; Discussion removed
Version: 3.0

Introducing filters for removeformat plugin.

comment:14 Changed 7 years ago by Garry Yao

Owner: changed from Alfonso Martínez de Lizarrondo to Garry Yao
Status: reopenednew

comment:15 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

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

Changed 7 years ago by Garry Yao

Attachment: 4125_5.patch added

comment:16 Changed 7 years ago by Garry Yao

Keywords: Review? added; Review- removed
Status: newassigned

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

comment:17 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

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() ) ); 
 *              }); 
 */ 

Changed 7 years ago by Garry Yao

Attachment: 4125_6.patch added

comment:18 Changed 7 years ago by Garry Yao

Keywords: Review? added; Review- removed

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

comment:19 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

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.

Changed 7 years ago by Garry Yao

Attachment: 4125_7.patch added

comment:20 Changed 7 years ago by Garry Yao

Keywords: Review? added; Review- removed

Ok, thanks for the advise.

comment:21 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review+ added; Review? removed

comment:22 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [5460].

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy