Opened 6 years ago

Closed 6 years ago

#5931 closed Bug (fixed)

Unable to change font-size

Reported by: karena 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 comp615 6 years ago.
maybe?
5931_2.patch (2.0 KB) - added by tobiasz.cudnik 6 years ago.
5931_3.patch (3.5 KB) - added by tobiasz.cudnik 6 years ago.
5931_4.patch (763 bytes) - added by garry.yao 6 years ago.
5931_5.patch (2.8 KB) - added by garry.yao 6 years ago.
5931_6.patch (3.2 KB) - added by garry.yao 6 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by karena

  • Version set to 3.3.1

comment:2 Changed 6 years ago by tobiasz.cudnik

  • Keywords Confirmed added

comment:3 Changed 6 years ago by comp615

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 6 years ago by comp615

maybe?

comment:4 Changed 6 years ago by comp615

  • 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 6 years ago by tobiasz.cudnik

  • Milestone set to CKEditor 3.4

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

comment:6 Changed 6 years ago by fredck

  • Keywords Review? removed

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

comment:7 Changed 6 years ago by fredck

  • Milestone changed from CKEditor 3.4 to CKEditor 3.5

comment:8 Changed 6 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from confirmed to assigned

Changed 6 years ago by tobiasz.cudnik

comment:9 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

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

comment:10 Changed 6 years ago by Saare

  • Status changed from review to review_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 6 years ago by tobiasz.cudnik

comment:11 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

Changed 6 years ago by garry.yao

comment:12 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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

comment:13 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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 6 years ago by garry.yao

comment:14 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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

comment:15 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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

comment:16 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

R+ for 5931_5.patch.

comment:17 Changed 6 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [5951].

comment:18 Changed 6 years ago by garry.yao

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Changed 6 years ago by garry.yao

comment:19 Changed 6 years ago by garry.yao

  • Owner changed from tobiasz.cudnik to garry.yao
  • Status changed from reopened to review

comment:20 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

comment:21 Changed 6 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed now with [5961].

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