Opened 6 years ago

Closed 6 years ago

#6737 closed New Feature (fixed)

Format Combo doesn't respect config.format_xx definitions

Reported by: sebduggan Owned by: fredck
Priority: Normal Milestone: CKEditor 3.6
Component: UI : Toolbar Version: 3.4.2
Keywords: Cc:

Description

The format combo box doesn't display its options in the correct style as attributed in the config.format_xx options.

For example, if I set the following config option:

CKEDITOR.config.format_h1 = { element : 'h2'};

...then when I choose "Heading 1" from the combo, the selected text is formatted as an <h2> element. However, the format combo box still shows "Heading 1" with the <h1> CSS applied to it.

The fix for this is simple. In /_source/plugins/format/plugin.js, line 48 should be changed from:

this.add( tag, '<' + tag + '>' + label + '</' + tag + '>', label );

to:

this.add( tag, '<' + styles[tag].element + '>' + label + '</' + styles[tag].element + '>', label );

(You may like to do this differently according to house coding conventions).

Attachments (2)

6737.patch (16.9 KB) - added by garry.yao 6 years ago.
6737_2.patch (1.2 KB) - added by fredck 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Saare

  • Keywords HasPatch added
  • Status changed from new to confirmed

We should also support attributes and styles defined that way.

comment:2 Changed 6 years ago by wwalc

  • Milestone set to CKEditor 3.6
  • Type changed from Bug to New Feature

This change may have a surprising results for some users, so targeting to 3.6.

Changed 6 years ago by garry.yao

comment:3 Changed 6 years ago by garry.yao

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

Patch unifies the styles definition of format combo with editor's standard styles definition (via styles set).

comment:4 Changed 6 years ago by garry.yao

  • Keywords HasPatch removed

Note that though back-compat on configuration (old format_tags) is not maintained by the propose patch, old editor wouldn't be broken because of upgrading.

comment:5 Changed 6 years ago by fredck

  • Status changed from review to review_failed

I'm sorry to say so, but we don't need to make such a strong change to our code to achieve what the reported looks for here. We don't need another Style combo here, not even additional js files to get downloaded. The format combo has a precise and delimited scope. The solution is in fact as simple as proposed by the reporter.

KISS!

comment:6 Changed 6 years ago by fredck

  • Owner changed from garry.yao to fredck
  • Status changed from review_failed to assigned

Changed 6 years ago by fredck

comment:7 Changed 6 years ago by fredck

  • Status changed from assigned to review

comment:8 Changed 6 years ago by Saare

  • Status changed from review to review_passed

comment:9 Changed 6 years ago by fredck

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

Fixed with [6683].

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