Opened 3 years ago

Last modified 3 years ago

#16769 review New Feature

Event fired when style is changed

Reported by: Tomasz Jakut Owned by: Tomasz Jakut
Priority: Normal Milestone:
Component: General Version:
Keywords: Cc:

Description

Currently applying new style or removing existing style in the editor (e.g. via changing element's style using "Styles" dropdown menu) does not fire any event. Because of that there is no sensible way to detect such changes in the editor.

Using change event for this purpose could be a workaround, but generates too much noise. Dedicated event should be a better solution.

Change History (4)

comment:1 Changed 3 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: newassigned

comment:2 Changed 3 years ago by Tomasz Jakut

Status: assignedreview

Simple and naive implementation landed on branch:t/16769.

comment:3 Changed 3 years ago by kkrzton

Status: reviewreview_failed

As for initial implementation, it looks good, but needs some more polishing:

  1. The docs for the styleChange event are missing (remember also about @since tag).
  2. The action attribute in styleChange event has 0|1 value which might be unclear. It will be more readable with some "const" or string values (apply/remove or something similar).
  3. No manual tests.
  4. As removeFromRange/applyToRange works different for inline/block/object (core/style.js#L287 and core/style.js#263) elements, it will be good to provide at least one unit test for each element type (so 3 for apply and 3 for remove).

One thing which I am not sure about is that if the event is fired in a proper place. The applyStyleOnSelection utilizes 6 different methods underneath with a quite complicated flow, which may or may not result in changing styles (readonly elements etc.), so it will be fired basically after an attempt to change the style (which in most cases results in style change ofc). IMHO, we may proceed with such approach as long as it is clearly described in docs/specs.

We should also keep in mind that CKEDITOR.style class is used not only to change styles (in terms of CSS / style attribute), but it is also used to change attributes or element types (which may not have any visual impact) so it is also important to clearly state that (ofc if one is familiar with CKEDITOR.style class it might be obvious.)

comment:4 Changed 3 years ago by Tomasz Jakut

Status: review_failedreview
  1. Docs added.
  2. action changed to string (apply/remove').
  3. Manual test added.
  4. Unit test updated.

The event is still fired for every attempt to style anything and IMO it's more predictable (user click "Bold" button and expects that something happens).

Every weirdness of CKEDITOR.style is put into docs (or at least I hope so) ;)

Pushed branch:t/16769.

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