Ticket #8979 (closed Bug: fixed)

Opened 3 years ago

Last modified 2 years ago

Incorrect font size shown for <font> tag

Reported by: tmonahan Owned by: fredck
Priority: Normal Milestone: CKEditor 3.6.4
Component: Core : Styles Version: 3.0
Keywords: IBM Cc: damo, satya

Description

To Reproduce:

  • Open any CKEditor sample and copy the following into the Source view:
<p>
	<font size="3">Sample Text</font></p>
  • Return to wysiwyg mode and click on the text in the editor.

Problem: The font size combo box displays the first font size in the list i.e. 8. It should not display any size.

This is confusing for the user because if they think the font size is 8 and wish to make it bigger e.g. 12, the font size actually gets smaller when size 12 styling is applied.

Attachments

8979.patch (2.3 KB) - added by garry.yao 2 years ago.
8979_2.patch (4.2 KB) - added by garry.yao 2 years ago.
8979_3.patch (1.7 KB) - added by fredck 2 years ago.

Change History

comment:1 Changed 3 years ago by j.swiderski

  • Status changed from new to confirmed
  • Version changed from 3.6.4 (SVN - trunk) to 3.0

Issue reproducible in all browsers from CKEditor 3.0

Changed 2 years ago by garry.yao

comment:2 Changed 2 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to review

Call for a conceptual review.

comment:3 Changed 2 years ago by fredck

  • Status changed from review to assigned

The root of the problem here seems to be the semantic meaning of the API.

The "checkElementRemovable" method is supposed to "check if the element is removable by the style". It's should not check if "the element matches the style".

Those are different needs. The first takes in consideration overrides and can be used by things like the bold button, which doesn't have variations and acts just like "add and remove".

The second instead does not look at overrides and can be used by features that should not match overrides, but should still suppress them when used (like font size).

So, what if we expand the API with the "checkElementMatch", which will be simply the result of splitting the current "checkElementRemovable" without adding new code?

This would give the very same results of your proposal, but would help us having a more semantically correct API, without adding strange features to it.

comment:4 Changed 2 years ago by garry.yao

  • Status changed from assigned to review
  • Component changed from General to Core : Styles
  • Milestone set to CKEditor 3.6.4

Changed 2 years ago by garry.yao

comment:5 Changed 2 years ago by garry.yao

Last edited 2 years ago by fredck (previous) (diff)

Changed 2 years ago by fredck

comment:6 Changed 2 years ago by fredck

  • Owner changed from garry.yao to fredck

The new patch simply changed the other of the methods on the styles plugin, to have a simpler diff for the change. Additionally, it reverts most of the unnecessary changes in the font plugin.

comment:7 Changed 2 years ago by garry.yao

  • Status changed from review to review_passed

comment:8 Changed 2 years ago by fredck

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

Fixed with [7502].

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