Opened 6 years ago

Closed 5 years ago

#8979 closed Bug (fixed)

Incorrect font size shown for <font> tag

Reported by: Teresa Monahan Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.6.4
Component: Core : Styles Version: 3.0
Keywords: IBM Cc: Damian, Satya Minnekanti

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 (3)

8979.patch (2.3 KB) - added by Garry Yao 5 years ago.
8979_2.patch (4.2 KB) - added by Garry Yao 5 years ago.
8979_3.patch (1.7 KB) - added by Frederico Caldeira Knabben 5 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Jakub Ś

Status: newconfirmed
Version: 3.6.4 (SVN - trunk)3.0

Issue reproducible in all browsers from CKEditor 3.0

Changed 5 years ago by Garry Yao

Attachment: 8979.patch added

comment:2 Changed 5 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

Call for a conceptual review.

comment:3 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewassigned

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 5 years ago by Garry Yao

Component: GeneralCore : Styles
Milestone: CKEditor 3.6.4
Status: assignedreview

Changed 5 years ago by Garry Yao

Attachment: 8979_2.patch added

comment:5 Changed 5 years ago by Garry Yao

Last edited 5 years ago by Frederico Caldeira Knabben (previous) (diff)

Changed 5 years ago by Frederico Caldeira Knabben

Attachment: 8979_3.patch added

comment:6 Changed 5 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben

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 5 years ago by Garry Yao

Status: reviewreview_passed

comment:8 Changed 5 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [7502].

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