New toolbar is not backwards compat.

[Sa'ar Zac Elias]

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']

[Frederico Caldeira Knabben]

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' ] ]

[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.

[Frederico Caldeira Knabben]

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

[Frederico Caldeira Knabben]

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

[Garry Yao]

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.

[Garry Yao]

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;

[Sa'ar Zac Elias]

• 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.

[Frederico Caldeira Knabben]

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

[Satya Minnekanti]

i am adding myself to the cc field for this ticket

[Garry Yao]

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.

[Frederico Caldeira Knabben]

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.

[Frederico Caldeira Knabben]

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.

[Garry Yao]

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.

[Frederico Caldeira Knabben]

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.

[Garry Yao]

Fixed with [6810].