Ticket #4589 (closed Bug: expired)

Opened 5 years ago

Last modified 5 years ago

Output style attribute "override" by regular expression is ignored

Reported by: pomu0325 Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.2
Component: Core : Styles Version: 3.0
Keywords: Cc:

Description

For example, following override setting unintendedly matches all <font> element whatever attribute it has.

config.fontSize_style =
{
	element		: 'span',
	attributes	: { 'class' : '#(size)' },
	overrides	: [ { element : 'font', attributes : { 'class' : /^foo/ } } ]
};

'CKEDITOR.tools.clone()' seems not to clone RegExp object properly at line.89 of styles/plugin.js

styleDefinition = CKEDITOR.tools.clone( styleDefinition );

After this line, regular expression is turned into , therefore it matches anything.

Attachments

4589.patch (422 bytes) - added by pomu0325 5 years ago.
4589_2.patch (784 bytes) - added by garry.yao 5 years ago.
4589_3.patch (540 bytes) - added by garry.yao 5 years ago.

Change History

comment:1 Changed 5 years ago by pomu0325

It seems to be a side-effect of [3704], 'clone' method now doesn't clone RegExp properly.

reproduce

  • Add above settings to config.js
  • Open any CKEditor sample.
  • Paste following html in source mode.
    <font class="foo"><font class="bar">test</font></font>
    
  • Select text and choose font-size dropdown in WYSIWYG mode

expected result

Only the <font> element which matches the regular expression should be overrided.

<span class="16px"><font class="bar">test</font></span>

actual result

Although 'class="bar"' doesn't match the regular expression, it is also overrided.

<span class="16px">test</span>

This problem is simply avoided by just cloning RegExp object. Is there any reason RegExp should not be cloned??

Changed 5 years ago by pomu0325

comment:2 Changed 5 years ago by wwalc

  • Keywords HasPatch added

comment:3 Changed 5 years ago by fredck

  • Milestone set to CKEditor 3.2

The patch should be good for it.

Changed 5 years ago by garry.yao

comment:4 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned

Beside the RegExp fix there, the array clone could be further simplified - Array is simply first class Object in JavaScript.

comment:5 Changed 5 years ago by garry.yao

  • Keywords Review? added; HasPatch removed

comment:6 follow-up: ↓ 7 Changed 5 years ago by alfonsoml

  • Keywords Review- added; Review? removed

The RegExp part has been merged in [4774] from the 3.1.x branch, so that part isn't needed.

I've done a quick test and it seems that the behavior remains the same if the code for the Array object is removed. Maybe it was added due to problems with some browser? If no one finds problems with that I guess that it could be removed.

Changed 5 years ago by garry.yao

comment:7 in reply to: ↑ 6 Changed 5 years ago by garry.yao

  • Keywords Review- removed
  • Status changed from assigned to closed
  • Resolution set to expired

Replying to alfonsoml:

The RegExp part has been merged in [4774] from the 3.1.x branch, so that part isn't needed.

The problem described at this ticket was fixed already by [4774].

...Maybe it was added due to problems with some browser?

It's not targeting a specific browser, just to make the array clone codes simpler.

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