Opened 6 years ago

Closed 5 years ago

#7492 closed Bug (fixed)

Overrides of Styles don't work correctly for classes

Reported by: Alfonso Martínez de Lizarrondo Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone: CKEditor 3.6.3
Component: Core : Styles Version:
Keywords: Cc:

Description

Load the XHTML output sample.[br] Select a word an apply for example the Comic font to it. Now select Times New Roman

Expected results:[br] A single span that makes the text appear in Times New Roman.

Actual results:[br] Two nested spans with and the text is shown in Comic.

This is the way that the font styles are defined for that sample:

font_style :
{
		element		: 'span',
		attributes		: { 'class' : '#(family)' },
		overrides	: [ { element : 'span', attributes : { 'class' : /^Font(?:Comic|Courier|Times)$/ } } ]
},

As the "element" is the same one that it's used in the overrides definition it seems to be cause of the problems.

The patch fixes this problem, but there might be better ways to fix it if it was planned in a different way (and there might be other places to review)

Attachments (4)

7492.patch (1003 bytes) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Proposed patch
7492_tests.patch (1.7 KB) - added by Frederico Caldeira Knabben 6 years ago.
Tests for the tests branch.
7492_2.patch (3.1 KB) - added by Alfonso Martínez de Lizarrondo 5 years ago.
New patch
7492_3.patch (3.1 KB) - added by Alfonso Martínez de Lizarrondo 5 years ago.
Revised patch

Download all attachments as: .zip

Change History (15)

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

Status: newreview

Changed 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: 7492.patch added

Proposed patch

Changed 6 years ago by Frederico Caldeira Knabben

Attachment: 7492_tests.patch added

Tests for the tests branch.

comment:2 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.1
Status: reviewreview_passed

I'm attaching a test for this case.

While we need to work on an overall review on the styles system, the proposed patch seems to properly fix this case. It looks logic "enough".

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

Resolution: fixed
Status: review_passedclosed

Fixed with [6983]

comment:4 Changed 6 years ago by Wiktor Walc

Milestone: CKEditor 3.6.1
Resolution: fixed
Status: closedreopened

Reverted with [7061] due to #8078.

comment:5 Changed 6 years ago by Garry Yao

The overrides definitions in the xhtml_output sample page is reverted with [7062].

Changed 5 years ago by Alfonso Martínez de Lizarrondo

Attachment: 7492_2.patch added

New patch

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

Status: reopenedreview

The new patch restores the xhtml sample and fixes #8078 by calling removeOverrides with a new parameter (so it doesn't affect the rest of the code) when the element is a block type so it isn't removed.

This way we avoid nesting spans as the first patches did and we remove the element only when it's an inline and we don't have left over spans without attributes.

comment:7 Changed 5 years ago by Frederico Caldeira Knabben

comment:8 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

#8078 is still regressed with the new patch (its test also fails).

Changed 5 years ago by Alfonso Martínez de Lizarrondo

Attachment: 7492_3.patch added

Revised patch

comment:9 Changed 5 years ago by Alfonso Martínez de Lizarrondo

Status: review_failedreview

Sorry, don't know what I tested before creating the patch but it was obviously wrong.

comment:10 Changed 5 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.3
Status: reviewreview_passed

comment:11 Changed 5 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: review_passedclosed

Fixed with [7407]

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