Opened 8 years ago

Closed 8 years ago

#7746 closed Bug (fixed)

New toolbar is not backwards compat.

Reported by: Sa'ar Zac Elias Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6
Component: UI : Toolbar Version: 3.6
Keywords: IBM Cc: damian.chojna@…, satya_minnekanti@…

Description

The new way of handling toolbar is not completely backwards compat. Each of the following fails in a different way:

['Bold','Italic','Underline','FontSize']
['FontSize','Bold','Italic','Underline']

Attachments (7)

7746.patch (2.1 KB) - added by Frederico Caldeira Knabben 8 years ago.
7746_2.patch (2.3 KB) - added by Frederico Caldeira Knabben 8 years ago.
7746_3.patch (6.9 KB) - added by Garry Yao 8 years ago.
7746_4.patch (10.1 KB) - added by Garry Yao 8 years ago.
7746_4_Issue_Screenshot.png (25.8 KB) - added by Frederico Caldeira Knabben 8 years ago.
7746_5.patch (8.7 KB) - added by Garry Yao 8 years ago.
7746_6.patch (8.9 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: newassigned

I found this problem when developing the feature. We have problems now when mixing "groupable" with "non-groupable" toolbar items.

I'll check if there is any reasonable solution for this, but it may happen that we'll have this limitation, so people will be forced to split toolbars like this:

[ ['Bold','Italic','Underline' ], [ 'FontSize' ] ]
[ ['FontSize' ], [ 'Bold','Italic','Underline' ] ]

comment:2 Changed 8 years ago by Garry Yao

Just about to report it, where I got a problem when landing bbcode toolbar, I consider it quite an often happens, we'd better make it back-compat.

Changed 8 years ago by Frederico Caldeira Knabben

Attachment: 7746.patch added

comment:3 Changed 8 years ago by Frederico Caldeira Knabben

Component: GeneralUI : Toolbar
Status: assignedreview

The proposed patch "fixes" the toolbar in these cases, for skins that don't support it, to avoid mixing groupable and non-groupable items.

Changed 8 years ago by Frederico Caldeira Knabben

Attachment: 7746_2.patch added

comment:4 Changed 8 years ago by Frederico Caldeira Knabben

The first patch was not good on handling separators. The new one should be ok.

comment:5 Changed 8 years ago by Garry Yao

Status: reviewreview_failed

The problem with this patch is that it mangles user's original toolbar definition, which result in changed toolbar navigation that user expect, it's never possible to have a combo and one button to live in the same toolbar.

Changed 8 years ago by Garry Yao

Attachment: 7746_3.patch added

comment:6 Changed 8 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao
Status: review_failedreview

I have instead an idea of making item grouping decidable by skin as it's a highly UI related feature, so skins have their flavors:

  1. Kama prefers not to group rich combo;
  2. V2/office prefer not to group separator;

comment:7 Changed 8 years ago by Damian

Cc: damian.chojna@… added
Keywords: IBM added

comment:8 Changed 8 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • The editor does not load with the default toolbar.
  • The approach looks good to me, but one thing:
    [ ['Bold', 'Styles', '-', 'Underline' ] ]
    

With the new approach, the separator should be just removed in Kama, as it doesn't bring any benefit.

comment:9 Changed 8 years ago by Frederico Caldeira Knabben

I liked your proposal as well, Garry. Thinks are pretty broken though, so it needs more work.

comment:10 Changed 8 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

i am adding myself to the cc field for this ticket

Changed 8 years ago by Garry Yao

Attachment: 7746_4.patch added

comment:11 in reply to:  8 Changed 8 years ago by Garry Yao

Status: review_failedreview

Replying to Saare:

[ ['Bold', 'Styles', '-', 'Underline' ] ] }}} With the new approach, the separator should be just removed in Kama, as it doesn't bring any benefit.

I think we can live with it for now.

Changed 8 years ago by Frederico Caldeira Knabben

Attachment: 7746_4_Issue_Screenshot.png added

comment:12 Changed 8 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

The attached screenshot shows how the skins sample renders with the latest patch. This happened with 7746_3.patch as well (the "pretty broken" part of my message).

I'm just using the first toolbar definition exemplified by the reporter:

config.toolbar = [ ['Bold','Italic','Underline','FontSize'] ];

Additionally, there is no need to remove the DocProp from the default toolbar definition. Considering that it'll be automatically ignored, it would just make the life of those dealing with full page harder, making also our sample file more complex.

Changed 8 years ago by Garry Yao

Attachment: 7746_5.patch added

comment:13 Changed 8 years ago by Garry Yao

Status: review_failedreview

comment:14 Changed 8 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I would be mostly ok for a R+ here, except for one tiny issue. The office2003 skin has the combo a bit broken on IE+Quirks (and potentially IE6).

It looks like a lot of testing on all browsers and UI combinations (RTL, HC) need to be performed here on review.

Changed 8 years ago by Garry Yao

Attachment: 7746_6.patch added

comment:15 in reply to:  14 Changed 8 years ago by Garry Yao

Status: review_failedreview

Replying to fredck:

I would be mostly ok for a R+ here, except for one tiny issue. The office2003 skin has the combo a bit broken on IE+Quirks (and potentially IE6).

Not this ticket to blame, it's a regression of [6748].

It looks like a lot of testing on all browsers and UI combinations (RTL, HC) need to be performed here on review.

Not that much, only the rich combo to be checked.

comment:16 Changed 8 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

The "_height:20px" fix is still not good. It's ok for IE+Quirks (all versions), but it breaks IE6 in standards mode, where the original 22px are required. Better to have a dedicated clause just for IE+Quirks.

I'll not block the commit just because of this. Please proceed with this small fix directly when committing.

comment:17 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6810].

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