Opened 11 years ago
Closed 11 years ago
#11297 closed New Feature (fixed)
Introduce styles applicable to widgets
Reported by: | Piotrek Koszuliński | Owned by: | Piotrek Koszuliński |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.4.0 |
Component: | Core : Styles | Version: | 4.3 |
Keywords: | Cc: | wim.leers@… |
Description (last modified by )
Problem
It's not possible to apply style to widgets using styles combo. Styles are either applied around widget or inside its nested editables.
Simple solution (outdated)
The simplest solution I can think of is to focus on classes only. A style with a special property denoting that it can be applied to widgets (or somehow generalised "subject"), could only set class(es). That class would be applied to the widget element (only) and then we or widgets authors should make sure that classes are preserved during downcasting.
Classes would of course make it possible to style all parts of widgets, not only widget element.
Why am I proposing only classes? Because styles and other attributes would make this much more complicated. First of all, they could not be applied on elements inside widget and e.g. img
is not a widget element in some cases, so how would that style apply a border to it? Second of all, there are cases in which it would not be easy to ensure that all inline styles and attributes are kept during downcasting. A "border" style for image2 for some people should behave differently whether image is captioned or not and that can be achieved only by classes.
Change History (13)
comment:1 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
Status: | new → confirmed |
---|
comment:5 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Milestone: | → CKEditor 4.4 |
comment:6 follow-up: 7 Changed 11 years ago by
Spec v1.
Requirements
The goal is to allow applying styles (CKEDITOR.style
) to widgets. Remarks:
- Editor should be able to check if style can be applied on certain widget.
- Editor should be able to check if style is already applied on certain widget.
- Editor should be able to apply style and remove style from widget.
- Only widgets themselves know how to apply style to them.
- Styles system cannot know about widgets system so there has to be an intermediate level between them.
- It's too complex for our widgets to handle inline styles, so by default we will only accept classes. However, if it's feasible, we should make it possible to apply all kind of styles.
- We need to remember about integration with ACF.
Architecture
- New
editor.addCustomStyleHandler
method. It will be used to define new style types. Currently we have inline, block and object styles, which are element styles and are defined by integer flagsCKEDITOR.STYLE_*
. The custom style types need to be strings in order to make it simple to make them unique (within editor).
@param {String} type Name of the new type. Must be unique within editor instance. @paream {Object} handler The object containing handlers. @param {Function} handler.checkApplicable The same as {@link CKEDITOR.style#checkApplicable}. @param {Function} handler.checkActive The same as {@link CKEDITOR.style#checkActive}. @param {Function} handler.apply Applies the style to contents of the editor. @param {CKEDITOR.editor} handler.apply.editor The editor instance in which style will be applied. @param {Function} handler.remove Removes the style from contents of the editor. @param {CKEDITOR.editor} handler.remove.editor The editor instance in which style will be removed. @param {Function} toFeature See the integration with ACF part.
- When handling style (e.g. checking if it can be applied to current selection) styles system checks:
- If the style is one of default types (its definition had the
element
property defined), the standard handlers are used. - If the style is one of custom types (its
type
is not one of default), then a corresponding style handler is retrieved and its methods are used.
- If the style is one of default types (its definition had the
- Widgets system will add a
widget
style type. How a style definition will look like then? We definitely need awidget
property there which will tell to which widget that style may be applied. Therefore a style definition will look like this:
{ name: 'Thick border', type: 'widget', widget: 'image2', ... the attributes ... }
- It's now up to widgets system how "the attributes" part should look like. Since we should rather not block a possibility to handle more than classes by some 3rd party widgets (or widgets implemented in the future), the attributes part should be identical to the default styles definitions. However, we'll handle only
attributes.class
for now.
CKEDITOR.plugins.widget
will have 6 new simple methods:applyStyle
,removeStyle
,checkStyleActive
,addClass
,removeClass
andhasClass
. First two will simply getattributes.class
from style definition and use 4th and 5th method, 3rd will check if style is applied by using 6th method. Thanks to the fact that all methods are overridable each widget can customize that behaviour. E.g. image2 will overrideapply/remove/hasClass
methods to apply classes not as by default to widget element, but only toimg
orfigure
tags (so not to centering wrapper). Other widgets can overrideapplyStyle
to handle more than classes.
- The trickiest part is an integration with the ACF. ACF is able to create allowed content rules from styles because they contain information about elements and their attributes. Custom styles will not provide such information and therefore need to inform ACF what they represent. We may use similar mechanism to `feature#toFeature`, so style handler will actually be a
CKEDITOR.feature
.
However, this is still not enough. Widgets system will define the
toFeature
method, but how does it know the prototypes of elements (name + additional stuff likematch
) to which styles can be applied (both things together create allowed content rule)? Since only widgets themselves know what elements are styleable, they have to contain such information instyleableElements {String}
property. Defining such element (as string - e.g.'img figure'
) will be the easiest way to make widget styleable and integrated with ACF. However, it may happen that widget overridesapplyStyle
so much that it's not possible to create allowed content rules from styleable elements and style attributes or that someone wants to define more precise element prototype (e.g. usematch
). Therefore we need another option - a method which based on style definition will return allowed content rule -styleToAllowedContentRule
(I'm open for better names :D).
Not clear yet
- How the custom styles will be displayed in styles dropdown? In which group? Widgets styles are a kind of object styles, but we cannot assume that all custom styles will be. We can solve this in two ways:
- custom style handler will define to which default group it belongs (I think that it may be a callback too, because then style handler will have full control in which context and when which style of this type is assigned to which group - of course usually it will always return the same value so to KISS it may be a const too),
- we'll have custom styles rendered in a separate, 4th group in styles dropdown.
I like the first option more.
comment:7 Changed 11 years ago by
Replying to Reinmar:
The proposal sounds really good. Well done!
Not clear yet
- How the custom styles will be displayed in styles dropdown? In which group? Widgets styles are a kind of object styles, but we cannot assume that all custom styles will be. We can solve this in two ways:
- custom style handler will define to which default group it belongs (I think that it may be a callback too, because then style handler will have full control in which context and when which style of this type is assigned to which group - of course usually it will always return the same value so to KISS it may be a const too),
- we'll have custom styles rendered in a separate, 4th group in styles dropdown.
I'm for option 1, with a constant values, defaulting to "Object" to make it totally optional.
comment:8 Changed 11 years ago by
I'm for option 1, with a constant values, defaulting to "Object" to make it totally optional.
Cool.
Spec v2.
Revisions: https://gist.github.com/Reinmar/15989ded9535a15f0fea/revisions
Requirements
The goal is to allow applying styles (CKEDITOR.style
) to widgets. Remarks:
- Editor should be able to check if style can be applied on certain widget.
- Editor should be able to check if style is already applied on certain widget.
- Editor should be able to apply style and remove style from widget.
- Only widgets themselves know how to apply style to them.
- Styles system cannot know about widgets system so there has to be an intermediate level between them.
- It's too complex for our widgets to handle inline styles, so by default we will only accept classes. However, if it's feasible, we should make it possible to apply all kind of styles.
- We need to remember about integration with ACF.
Architecture
- New
editor.addCustomStyleHandler
method. It will be used to define new style types. Currently we have inline, block and object styles, which are element styles and are defined by integer flagsCKEDITOR.STYLE_*
. The custom style types need to be strings in order to make it simple to make them unique (within editor).
@param {String} type Name of the new type. Must be unique within editor instance. @paream {Object} handler The object containing handlers. @param {Function} handler.checkApplicable The same as {@link CKEDITOR.style#checkApplicable}. @param {Function} handler.checkActive The same as {@link CKEDITOR.style#checkActive}. @param {Function} handler.apply Applies the style to contents of the editor. @param {CKEDITOR.editor} handler.apply.editor The editor instance in which style will be applied. @param {Function} handler.remove Removes the style from contents of the editor. @param {CKEDITOR.editor} handler.remove.editor The editor instance in which style will be removed. @param {Function} [toFeature] See the integration with ACF part. @param {Integer} [assignedTo=CKEDITOR.STYLE_OBJECT] The group (one of default ones) to which this styles of this type will be assigned (e.g. in styles dropdown). Other idea for a name: `belongsTo`.
- When handling style (e.g. checking if it can be applied to current selection) styles system checks:
- If the style is one of default types (its definition had the
element
property defined), the standard handlers are used. - If the style is one of custom types (its
type
is not one of default), then a corresponding style handler is retrieved and its methods are used.
- If the style is one of default types (its definition had the
- Widgets system will add a
widget
style type. How a style definition will look like then? We definitely need awidget
property there which will tell to which widget that style may be applied. Therefore a style definition will look like this:
{ name: 'Thick border', type: 'widget', widget: 'image2', ... the attributes ... }
- It's now up to widgets system how "the attributes" part should look like. Since we should rather not block a possibility to handle more than classes by some 3rd party widgets (or widgets implemented in the future), the attributes part should be identical to the default styles definitions. However, we'll handle only
attributes.class
for now.
CKEDITOR.plugins.widget
will have 6 new simple methods:applyStyle
,removeStyle
,checkStyleActive
,addClass
,removeClass
andhasClass
. First two will simply getattributes.class
from style definition and use 4th and 5th method, 3rd will check if style is applied by using 6th method. Thanks to the fact that all methods are overridable each widget can customize that behaviour. E.g. image2 will overrideapply/remove/hasClass
methods to apply classes not as by default to widget element, but only toimg
orfigure
tags (so not to centering wrapper). Other widgets can overrideapplyStyle
to handle more than classes.
- The trickiest part is an integration with the ACF. ACF is able to create allowed content rules from styles because they contain information about elements and their attributes. Custom styles will not provide such information and therefore need to inform ACF what they represent. We may use similar mechanism to `feature#toFeature`, so style handler will actually be a
CKEDITOR.feature
.
However, this is still not enough. Widgets system will define the
toFeature
method, but how does it know the prototypes of elements (name + additional stuff likematch
) to which styles can be applied (both things together create allowed content rule)? Since only widgets themselves know what elements are styleable, they have to contain such information instyleableElements {String}
property. Defining such element (as string - e.g.'img figure'
) will be the easiest way to make widget styleable and integrated with ACF. However, it may happen that widget overridesapplyStyle
so much that it's not possible to create allowed content rules from styleable elements and style attributes or that someone wants to define more precise element prototype (e.g. usematch
). Therefore we need another option - a method which based on style definition will return allowed content rule -styleToAllowedContentRule
(I'm open for better names :D).
comment:9 Changed 11 years ago by
Owner: | set to Piotrek Koszuliński |
---|---|
Status: | confirmed → assigned |
comment:10 Changed 11 years ago by
Cc: | wim.leers@… added |
---|
comment:11 Changed 11 years ago by
I find overriding widgetDef#add/has/getClass(es)
quite confusing so I propose some improvements in #11771
comment:12 Changed 11 years ago by
Yes, it could be simplified. At the beginning we had 3 methods operating on classes, what was already a lot, now we have four, so they indeed could use some common "get styleable element" logic. Not a blocker now though.
comment:13 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
The reviews were handled in parts by Fred, Olek and me (for Olek's parts). We reached collective R+ :).
Introduced on major with git:721055c on dev and 45140b7 on tests.
cc