Opened 9 years ago
Closed 9 years ago
#13828 closed Bug (fixed)
Widget classes should be added to wrapper rather than widget element
Reported by: | Marek Lewandowski | Owned by: | Szymon Kupś |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.6.0 |
Component: | General | Version: | |
Keywords: | Cc: |
Description (last modified by )
As @Reinmar writes:
Currently widget classes are applied to widget elements. This makes it impossible to achieve various results - e.g. http://stackoverflow.com/questions/29704019/how-to-floatleft-a-container-with-an-image-having-a-relative-width
We wanted to keep classes on elements because we thought that it allows to create the same stylesheets for the content inside editor and outside. Unfortunately, due to the wrappers and CSS's limited capabilities it makes it impossible to achieve some pretty standard results.
Note, that it will not be that uncommon to move classes to the wrapper because e.g. image2 already moves alignment classes there. It's a pity that we didn't learn from that fact earlier ;/.
So all the methods like addClass should add to the wrapper rather than to the widget's element.
This is part of #13200 issue.
Change History (7)
comment:1 Changed 9 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 9 years ago by
Owner: | set to Szymon Kupś |
---|---|
Status: | confirmed → assigned |
comment:4 Changed 9 years ago by
comment:5 Changed 9 years ago by
Initial Idea
Actually, during research @s.kups pointed out that if we'll move *all* widget elem classes to the wrapper, we'll have have a problem that classes from template's main element will be moved to wrapper no matter what. That might confuse developers that don't know too much details about widget system.
Ideas to overcome this limitation involved whitelisting allowed classes for given widget type, but it would make implementation so much more complicated.
New Idea
Instead we're thinking about class replication to a widget wrapper. The implementation is deadly simple, it's backward compatible and it allows to easily differentiate widget's wrapper from the main element.
We might use cke-widget-wrapper-
as a prefix.
Let's progress with this second approach.
comment:6 Changed 9 years ago by
Status: | assigned → review |
---|
Pushed branch:t/13828 with the new idea proposed in comment:5.
It looks like changes in addClass and removeClass methods are enough to accomplish this behaviour. I've introduced read-only, static property CKEDITOR.plugins.wrapper.WRAPPER_CLASS_PREFIX
and updated tests.
comment:7 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Good job! The only one remark is related to docs:
- Prefer
@property
over@type
, just like it's used in CKEDITOR.NODE_ELEMENT, CKEDITOR.POSITION_AFTER_START etc.
Merged to master with git:336a28c.
It is important to mention that once this is fixed we need to ensure that
image2
plugin works correctly.