Opened 8 years ago

Closed 8 years ago

#5617 closed Bug (fixed)

Filter does not allow two 'text' filters

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.3
Component: Core : Output Data Version: 3.0
Keywords: Confirmed Review+ Cc:

Description (last modified by Garry Yao)

By adding another 'text' type filter to htmldataprocessor breaks the editor output, e.g.

htmlFilter.addRules(
	{
		text : function( text )
		{
			return text;
		}
	});

Attachments (3)

5617.patch (652 bytes) - added by Garry Yao 8 years ago.
5617_2.patch (1.6 KB) - added by Garry Yao 8 years ago.
5617_3.patch (1.7 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Garry Yao

Attachment: 5617.patch added

comment:1 Changed 8 years ago by Garry Yao

Description: modified (diff)
Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The new logic may not always be good. For example, if we have more than one filter and the "currentEntry" is changed by the first filter only, it will not be returned by the callItems function. It will happen only if the last filter changes it.

Other than that, the filter may also change the node type, so subsequent filters to be called will not be good for that node type. Even element or attribute names may be changed, invalidating the next filters. So, to avoid such situation, we should limit this full loop only on text or comment nodes, and only if the filter doesn't change the node type. In other cases, the current behavior must be maintained.

Changed 8 years ago by Garry Yao

Attachment: 5617_2.patch added

comment:3 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Ticket Test added:
run OR view source.

comment:4 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • It's ok to append the "type" property to the fragment class, to make the code clearer, avoiding the addition of line 235.
  • There is a problem though... a filter may return nothing, which means no change to the current entry. But, at line 272, you're updating the currentEntry with the undefined returned value, which will invalidate the filter in the subsequent calls. That line should be executed only if ret is not undefined.

Changed 8 years ago by Garry Yao

Attachment: 5617_3.patch added

comment:5 in reply to:  4 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to fredck:

  • It's ok to append the "type" property to the fragment class, to make the code clearer, avoiding the addition of line 235.

We've already have several places that rely on !node.type to detect a fragment node, which will get broken by the suggested change.

  • There is a problem though... a filter may return nothing, which means no change to the current entry. But, at line 272, you're updating the currentEntry with the undefined returned value, which will invalidate the filter in the subsequent calls. That line should be executed only if ret is not undefined.

Possibly, the new patch take this into consideration.

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:7 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [5477].

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