Opened 3 years ago

Closed 3 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 Marek Lewandowski)

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 3 years ago by Marek Lewandowski

Status: newconfirmed

comment:2 Changed 3 years ago by Marek Lewandowski

Description: modified (diff)

comment:3 Changed 3 years ago by Szymon Kupś

Owner: set to Szymon Kupś
Status: confirmedassigned

comment:4 Changed 3 years ago by Marek Lewandowski

It is important to mention that once this is fixed we need to ensure that image2 plugin works correctly.

comment:5 Changed 3 years ago by Marek Lewandowski

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.

Last edited 3 years ago by Marek Lewandowski (previous) (diff)

comment:6 Changed 3 years ago by Szymon Kupś

Status: assignedreview

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 3 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

Good job! The only one remark is related to docs:

Merged to master with ​git:336a28c.

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