Opened 14 years ago

Closed 13 years ago

#5931 closed Bug (fixed)

Unable to change font-size

Reported by: Karen Ananiev Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.4.2
Component: Core : Styles Version: 3.3.1
Keywords: HasPatch Cc: comp615@…

Description

  1. Open an example and select all text.
  2. Set font-size to 20.
  3. Select any word by double click.
  4. Set font-size to 14.
  5. Set font-size to 20 -> font-size will not be changed.

Attachments (6)

5931.patch (902 bytes) - added by Charlie 14 years ago.
maybe?
5931_2.patch (2.0 KB) - added by Tobiasz Cudnik 14 years ago.
5931_3.patch (3.5 KB) - added by Tobiasz Cudnik 14 years ago.
5931_4.patch (763 bytes) - added by Garry Yao 14 years ago.
5931_5.patch (2.8 KB) - added by Garry Yao 13 years ago.
5931_6.patch (3.2 KB) - added by Garry Yao 13 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 14 years ago by Karen Ananiev

Version: 3.3.1

comment:2 Changed 14 years ago by Tobiasz Cudnik

Keywords: Confirmed added

comment:3 Changed 14 years ago by Charlie

The problem seems to be more generally a problem with in-line styles. When an embedded inline style is applied, I.E. <style 1>text <style 2>text</style 2> text </style 1>

The first attempt to change the inner style only serves to change it to the following: <style 1>text </style 1><style 2>text</style 2><style 1> text </style 1>

Then a second attempt to apply style one (instead of two) to the inner span yields the appropriate: <style 1>text text text </style 1>

The two possible solutions are: 1) Initally create the split span tags instead of creating embedded spans with conflicting style properties 2) Have it recognize that instead of just splitting the outer style, it needs to re-look at the style we want to apply and potentially re-merge everything.

This is all from just playing around, hopefully it'll give someone ideas. I'll try to delve deeper and see if I can't figure out more

Changed 14 years ago by Charlie

Attachment: 5931.patch added

maybe?

comment:4 Changed 14 years ago by Charlie

Cc: comp615@… added
Keywords: HasPatch Review? added

Ok, have a look at the patch. The problem was that when the element we were altering was compared to the parent, CKEditor realized that this entire style element didn't need to be there (because of the parents) and stopped trying to 'add' the style.

The problem with this however, is that the style element already existed. So when it stopped, the element simply stayed as it was. I changed this to force it to add the blank element, thus replacing the old element, then remove the blank element.

There might be a more elequent way to do this as Iäm not familiar with all the commands available to me, so please let me know if you think this works or how it could be better!

comment:5 Changed 14 years ago by Tobiasz Cudnik

Milestone: CKEditor 3.4

Thank you for the patch, we will review it ASAP.

comment:6 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review? removed

The review keywords are reserved to the core team. HasPatch must be used in these cases.

comment:7 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 3.5

comment:8 Changed 14 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5931_2.patch added

comment:9 Changed 14 years ago by Tobiasz Cudnik

Status: assignedreview

Patch proposed before didn't resolve the issue in case of deeper nesting (step 4 repeated more than once).

comment:10 Changed 14 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

With this patch I'm getting this code:

<p>
	<span style="font-size: 20px;">This is <span style="font-size: 20px;">some</span> <strong>sample text</strong>. You are using <a href="http://ckeditor.com/">CKEditor</a>.</span></p>

Notice that there is a nested redundant span. In this case the inner span should be removed.

Changed 14 years ago by Tobiasz Cudnik

Attachment: 5931_3.patch added

comment:11 Changed 14 years ago by Tobiasz Cudnik

Status: review_failedreview

Added garbage collection of selected style and fixed styles and attributes inheritance direction.

Changed 14 years ago by Garry Yao

Attachment: 5931_4.patch added

comment:12 Changed 14 years ago by Garry Yao

Status: reviewreview_failed

For me we're just missing one condition block to cover the case described here.

comment:13 Changed 13 years ago by Tobiasz Cudnik

Status: review_failedreview

5931_4.patch fails for following TC:

  • Use content below
    <p>
    	<span style="font-size: 20px;">lorem ipsum <span style="font-size: 26px;">lorem FOO lorem</span> lorem ipsum</span></p>
    
    
  • Change font size of "FOO" to 20px

Result: No change.

I'm opting for another review of 5931_3.patch which passes this TC.

Changed 13 years ago by Garry Yao

Attachment: 5931_5.patch added

comment:14 Changed 13 years ago by Garry Yao

Status: reviewreview_failed

How about combining these two patches which looks good for me.

comment:15 Changed 13 years ago by Tobiasz Cudnik

Status: review_failedreview

I think it's a successful merge which reuses existing mechanisms and reduces the code amount, while achieving the same goal.

comment:16 Changed 13 years ago by Tobiasz Cudnik

Status: reviewreview_passed

R+ for 5931_5.patch.

comment:17 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [5951].

comment:18 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

Basic style application is broken by the change, revert with [5952].

Changed 13 years ago by Garry Yao

Attachment: 5931_6.patch added

comment:19 Changed 13 years ago by Garry Yao

Owner: changed from Tobiasz Cudnik to Garry Yao
Status: reopenedreview

comment:20 Changed 13 years ago by Tobiasz Cudnik

Status: reviewreview_passed

comment:21 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed now with [5961].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy