Opened 5 years ago

Closed 5 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 5 years ago by Olek Nowodziński

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

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

Description: modified (diff)

comment:3 Changed 5 years ago by Wim Leers

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

Following with great interest!

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

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

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

Owner: set to Olek Nowodziński
Status: newreview

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 5 years ago by Olek Nowodziński

Milestone: CKEditor 4.3.2

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

Milestone: CKEditor 4.3.2CKEditor 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 5 years ago by Olek Nowodziński

Status: reviewassigned

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

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

Milestone: CKEditor 4.3.3CKEditor 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 5 years ago by Olek Nowodziński

Description: modified (diff)

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

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

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

Status: assignedreview

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

Status: reviewreview_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 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:14 Changed 5 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 5 years ago by Wim Leers

So:

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

Right?

comment:16 in reply to:  15 Changed 5 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 5 years ago by Olek Nowodziński

Status: review_failedreview

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 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

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

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

Resolution: fixed
Status: review_passedclosed

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

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