Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#10260 closed Bug (fixed)

ACF blocks tabSpaces

Reported by: Jakub Ś Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.1.1
Component: General Version: 4.1
Keywords: Cc:

Description

Seems Allowed Content Filter blocks http://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-tabSpaces. Tab key causes focus on next element instead of inserting spaces into content area.

The below will not work without allowedContent: true

var editor = CKEDITOR.replace( 'editor1', {
	tabSpaces : 4,
	//allowedContent: true				
} );	 

Change History (15)

comment:1 Changed 7 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 7 years ago by Mike Burgh

Following

comment:3 Changed 7 years ago by Piotrek Koszuliński

Duplicate: #10271

comment:4 Changed 7 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.1.1

comment:5 Changed 7 years ago by Piotrek Koszuliński

ACF does not pass <span data-cke-marker="1"> element which editable#insertHtml uses (https://github.com/ckeditor/ckeditor-dev/blob/master/core/editable.js#L1127) to surround spaces when processing data.

I found that ACF also does not pass bookmarks (<span data-cke-bookmark="1" style="display:none;">&nbsp;</span>) and who knows how many other elements that we may need to sometimes insert to the editor.

So a basic solution would be to extend ACF with this kind of rule (this should be placed in filter constructor, next to default rule for p/br/div):

this.allow( {
	span: {
		match: function( el ) {
			return Object.keys( el.attributes ).some( function( attr ) {
				return attr.indexOf( 'data-cke-' ) > -1;
			} );
		},
		styles: 'display'
	}
}, 'default', 1 );

(Note: we don't need to specify data-cke-* attributes, because they all are passed by default)

However:

  • We will allow these spans in data outputted from editor (on the other hand we already allow data-cke-* attributes on all elements). So... if you agree that we shouldn't allow them for both - output and input, then this rule has to be implemented internally, inside filter. Note that CKEDITOR.filter#applyTo has 2nd argument which tells in which direction data is processed. The correct place for this rule will be around these lines - https://github.com/ckeditor/ckeditor-dev/blob/master/core/filter.js#L281-L288

This approach has another advantage - we will not expose in filter.allowedContent rule which is fully internal.

  • We haven't caught this issue before, because tests for editable#insertXXX are done with disabled ACF. I think that this case proved that we should enable ACF in those tests by setting minimum config.allowedContent that they need.

comment:6 Changed 7 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:7 Changed 7 years ago by Olek Nowodziński

Status: assignedreview

I'm for keeping stuff like this internally and as simple as possible. Thus I created branch t/10260 (+tests) with a fix for the issue inside of CKEDITOR.filter#applyTo.

comment:8 Changed 7 years ago by Piotrek Koszuliński

Fixes also #10302.

comment:9 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:10 Changed 7 years ago by Piotrek Koszuliński

Status: review_passedreview_failed

I rebased and pushed both branches. Additionally - I pushed more tests and it turned out that t/10260@dev does not pass one of test cases. Filter should not accept CKE's stuff in output.

comment:11 Changed 7 years ago by Olek Nowodziński

Status: review_failedreview

Updated both t/10260@dev and t/10260@tests.

I restricted the "holy cow mode" for data-cke-* spans to dataProcessor.toHtml() only as we don't want them on output. I also fixed some new test cases since dataProcessor.toDataFormat() avoids filtering, so switching tests to basic filter.applyTo() was necessary.

comment:12 Changed 7 years ago by Piotrek Koszuliński

Status: reviewreview_passed

comment:13 Changed 7 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Commits merged into master: git:d84ac37780390b6b, 79acdab8adf1360 (tests).

comment:14 Changed 7 years ago by Piotrek Koszuliński

Please, verify also if this patch fixes #10302 too.

comment:15 Changed 7 years ago by Olek Nowodziński

This patch also fixes #10302.

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