Opened 8 years ago
Last modified 8 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 8 years ago by
Owner: | set to Tomasz Jakut |
---|---|
Status: | new → assigned |
comment:2 Changed 8 years ago by
Status: | assigned → review |
---|
comment:3 Changed 8 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 8 years ago by
Status: | review_failed → review |
---|
- Docs added.
action
changed 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.