Opened 11 years ago

Closed 11 years ago

#11300 closed Task (fixed)

Image2: polishing and refactoring of internals

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.0
Component: General Version: 4.3
Keywords: Drupal Cc: wim.leers@…

Description (last modified by Olek Nowodziński)

Some refactoring got to be done to make Image2 flexible and "environment-friendly". Those changes are relevant for third-party developers, e.g. for D8.

  1. The internal form of the widget got to be styleable: instead of inline styles, we need to introduce classes. With inline styles, the look of the widget is not configurable and possibly incompatible with custom stylesheets during editor's lifetime.
    1. The non-captioned image should have no class.
    2. The class of captioned image must be configurable via config.image2_captionedClass and applied to <figure class="...">.
    3. Class "caption" is obsolete so it will be removed.
    4. Alignment also must be controlled with classes.
      1. Classes must be configurable
    5. Tons of tests to be aligned to the new API.
  1. We should guarantee that additional attributes (e.g. data-*) are preserved, regardless of the internal state of the widget. Extra attributes must survive any kind of internal processing and re-emerge in editor's output (1:1).
    1. Quite likely it's already working because the image node in DOM is passed along with states of a widget (needs tests).
    2. Alternatively, attributes can be serialized in upcast, saved in widget's data and exploded while downcasting. But let's stick to (a) as an easiest "solution" first.

Change History (19)

comment:1 Changed 11 years ago by Olek Nowodziński

Description: modified (diff)
Summary: Image2 in D8 → Image2: polishing and refactoring of internals

comment:2 Changed 11 years ago by Olek Nowodziński

Description: modified (diff)

comment:3 Changed 11 years ago by Wim Leers

Cc: wim.leers@… added
Keywords: Drupal added

Following with great interest!

comment:4 Changed 11 years ago by Olek Nowodziński

Development goes on in ​t/11300 (and corresponding test branch).

comment:5 Changed 11 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: new → review

Changes in dev branch:

  • Internally, widget alignment is controlled with classes:
    • cke_image_left (replaces float:left)
    • cke_image_right (replaces float:right)
    • cke_image_center (replaces text-align:center and display:inline-block of the figure).
  • Captioned widget's wrapper has default class image. It is available both internally and in data.
    • It's possible to alter the class with CKEDITOR.config.image2_captionedClass.
  • Default caption of captioned widget can be translated (also altered).
    • Available via editor.lang.image2.placeholder.

Changes in test branch: aligned test to latest API, improved several tests.

comment:6 Changed 11 years ago by Olek Nowodziński

Milestone: → CKEditor 4.3.2

comment:7 Changed 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.2 → CKEditor 4.3.3

We are still discussing the details and we're not yet sure that the change is complete, so I'm postponing this issue, to avoid making wrong decisions.

comment:8 Changed 11 years ago by Olek Nowodziński

Status: review → assigned

Development is continued in ​t/11300b rebased on latest master.

comment:9 Changed 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.3 → CKEditor 4.4

I changed milestone to 4.4, because it's a change that breaks backward compatibility.

I rebased and force-pushed both branches.

comment:10 Changed 11 years ago by Olek Nowodziński

Description: modified (diff)

comment:11 Changed 11 years ago by Olek Nowodziński

Rebased ​branch:t/11300b (and tests) on latest major.

comment:12 Changed 11 years ago by Olek Nowodziński

Status: assigned → review

comment:13 Changed 11 years ago by Piotrek Koszuliński

Status: review → review_failed
  1. Default aligned image classes cannot contain cke_* prefix. They are meant to be reused everywhere, so should be generic like figure.image.
  2. The default value of config.image2_alignClasses is null like for indent classes or justify classes. So by default editor produces styles not classes.
  3. Regarding doc for config.image2_alignClasses:
    • Developers are not meant to use CKEDITOR.addCss for that. In case of framed editor they indeed should use config.contentsCss and/or contents.css. But in case of inline editor they should attach stylesheet normaly in the <head>.
    • We should mention what are the styles which CKEditor uses by default for alignment classes. Because when one sets image2_alignClasses he'll be lost now. How does he center an image? Is aligned captioned image styled like non-captioned?
  4. When I set image2_alignClasses image2 still produces inline styles. The whole point was about making aligned image styleable. How is one going to style it if there's no class?
Last edited 11 years ago by Piotrek Koszuliński (previous) (diff)

comment:14 Changed 11 years ago by Piotrek Koszuliński

It turned out that we had a communication problem. So to make sure that we understand each other, we're talking to add following feature to the state of t/11300b from comment:11.

We want to add a config.image2_alignClasses option which will change the:

  • inner format to use classes defined in this option instead of inline styles.
  • output format to use classes defined in this option instead of inline styles.

The goal is to make it possible to style aligned images differently (depending on the alignment) in both cases - inside editor and outside it. The first part was actually already doable, but one needed to use CKEditor's classes. Now it will be possible to use the same CSS selectors inside and outside editor.

The default value of image2_alignClasses should be null meaning that inline styles are used in inner and output format.

Remember about good documentation for this option. It should contain an information about styles that one has to add to contents.css to make image2 working - preferably in copy&paste-ready format.

comment:15 Changed 11 years ago by Wim Leers

So:

  • "inner format" = "downcasted format"
  • "output format" = "upcasted format"

Right?

comment:16 in reply to:  15 Changed 11 years ago by Olek Nowodziński

Replying to Wim Leers:

So:

  • "inner format" = "downcasted format"
  • "output format" = "upcasted format"

Right?

The "inner format" is the format used inside of the editor, after upcasting.

The "output format" is what you see when you call editor.getData() – a downcasted widget.

comment:17 Changed 11 years ago by Olek Nowodziński

Status: review_failed → review

Latest code pushed to ​branch:t/11300d (+tests).

  1. In this implementation config.image2_alignClasses is null by default (inline styles are produced).
  2. If image2_alignClasses are defined, editor drops support for inline styles and uses classes only.
  3. Introduced CKEDITOR.htmlParser.element.addClass (with tests).

comment:18 Changed 11 years ago by Piotrek Koszuliński

Status: review → review_passed

I force pushed rebased and a little bit enhanced branches. R+!

comment:19 Changed 11 years ago by Olek Nowodziński

Resolution: → fixed
Status: review_passed → closed

​git:1c0c524 landed in major (2d65984 tests).

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy