Opened 9 years ago
Last modified 9 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 9 years ago by
| Owner: | set to Tomasz Jakut |
|---|---|
| Status: | new → assigned |
comment:2 Changed 9 years ago by
| Status: | assigned → review |
|---|
comment:3 Changed 9 years ago by
| Status: | review → review_failed |
|---|
As for initial implementation, it looks good, but needs some more polishing:
- The docs for the styleChange event are missing (remember also about @since tag).
- 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).
- No manual tests.
- 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 9 years ago by
| Status: | review_failed → review |
|---|
- Docs added.
actionchanged to string (apply/remove').- Manual test added.
- 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.

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