Ticket #4125 (closed Bug: fixed)

Opened 5 years ago

Last modified 4 years ago

Remove Format removes SCAYT spans

Reported by: arczi 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

4125.patch (1.4 KB) - added by alfonsoml 5 years ago.
Proposed patch
4125_2.patch (738 bytes) - added by SpellChecker.net 4 years ago.
4125_3.patch (1.1 KB) - added by SpellChecker.net 4 years ago.
4125_4.patch (3.2 KB) - added by garry.yao 4 years ago.
4125_5.patch (3.9 KB) - added by garry.yao 4 years ago.
4125_6.patch (3.8 KB) - added by garry.yao 4 years ago.
4125_7.patch (4.1 KB) - added by garry.yao 4 years ago.

Change History

Changed 5 years ago by alfonsoml

Proposed patch

comment:1 Changed 5 years ago by alfonsoml

  • Keywords Review? added
  • Status changed from new to assigned
  • Owner set to alfonsoml

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

comment:2 Changed 4 years ago by SpellChecker.net

  • Status changed from assigned to closed
  • Resolution set to fixed

patch is submitted in #5145

comment:3 Changed 4 years ago by fredck

  • Status changed from closed to reopened
  • Resolution fixed deleted

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 4 years ago by alfonsoml

  • Keywords Review? removed

Changed 4 years ago by SpellChecker.net

comment:5 Changed 4 years ago by SpellChecker.net

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

comment:6 Changed 4 years ago by alfonsoml

  • Milestone changed from CKEditor 3.x to CKEditor 3.3

#5467 has been marked as dup

comment:7 Changed 4 years ago by SpellChecker.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 4 years ago by alfonsoml

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 4 years ago by SpellChecker.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 4 years ago by alfonsoml

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 4 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 4 years ago by SpellChecker.net

comment:12 Changed 4 years ago by SpellChecker.net

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

Changed 4 years ago by garry.yao

comment:13 Changed 4 years ago by garry.yao

  • Keywords Confirmed Review? added; Discussion removed
  • Version set to 3.0

Introducing filters for removeformat plugin.

comment:14 Changed 4 years ago by garry.yao

  • Status changed from reopened to new
  • Owner changed from alfonsoml to garry.yao

comment:15 Changed 4 years ago by alfonsoml

  • 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 4 years ago by garry.yao

comment:16 Changed 4 years ago by garry.yao

  • Keywords Review? added; Review- removed
  • Status changed from new to assigned

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 4 years ago by alfonsoml

  • 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 4 years ago by garry.yao

comment:18 Changed 4 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 4 years ago by alfonsoml

  • 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 4 years ago by garry.yao

comment:20 Changed 4 years ago by garry.yao

  • Keywords Review? added; Review- removed

Ok, thanks for the advise.

comment:21 Changed 4 years ago by alfonsoml

  • Keywords Review+ added; Review? removed

comment:22 Changed 4 years ago by garry.yao

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [5460].

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