Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Title says it all.
Based on feedback from Piotrek Koszuliński (CKEditor developer): they expressed concern that it does get called very, very often and that that our listener may lead to performance problems.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | ckeditor_debounce_onchange-2064063-6.patch | 1.53 KB | Wim Leers |
| #6 | interdiff.txt | 584 bytes | Wim Leers |
| #5 | ckeditor_debounce_onchange-2064063-5.patch | 1.45 KB | Wim Leers |
| #5 | interdiff.txt | 1.68 KB | Wim Leers |
| #4 | ckeditor_debounce_onchange-2064063-4.patch | 2.42 KB | Wim Leers |
Comments
Comment #1
nod_tag
Comment #2
Reinmar CreditAttribution: Reinmar commentedMy concern was that in case of 1:1 relation between CKEditor's
#changeevent and Drupal's onChange, it may happen that e.g. content is being sent to server too often. However, I don't know how you use onChange, so I may be wrong. If a debouncer is meant to be used inside onChange's listener, then everything will be fine.My second concern is that the
editor.getData()is called on every #change. Getting data isn't a lightweight process - there's a parsing, filtering, processing, writing involved, so it's pretty heavy. It won't kill browser of course, but it may be unnecessary. Especially if callback's argument isn't then used.I also noticed that if multiple listeners are added using
onChangetheeditor.getData()will be called multiple times (once per every listener) on every#change. This may in fact overheat browser in case of dozens of listeners.Comment #3
Wim LeersGreat feedback, and great catch wrt the multiple listeners!
Comment #4
Wim LeersPlease review — not sure if this is the best or most elegant way to support multiple callbacks.
Comment #5
Wim Leersnod_ said in chat that:
editor.getData()is called only once for all callbacks (if there are multiple callbacks) is not yet justified; if it turns out to be a performance problem in the future, we can still change this. (After all, that wouldn't be an API change.)debounce(function changeHandler() {can cause problems — I added it to make stack traces less anonymous, which I know he likes, but apparently this is problematic too. ("named closures do weird things sometimes", he said.) Easy change.Patch rerolled to address that.
Comment #6
Wim Leersnod_ noticed a small pre-existing problem that should be fixed as part of this patch:
ckeditor.jsuses jQuery, but that dependency is not declared in the library definition.Comment #7
nod_Straightforward, that works, thanks :)
Comment #7.0
nod_Updated issue summary.
Comment #8
Wim Leers6: ckeditor_debounce_onchange-2064063-6.patch queued for re-testing.
Comment #10
Wim Leers6: ckeditor_debounce_onchange-2064063-6.patch queued for re-testing.
Comment #12
Wim Leers6: ckeditor_debounce_onchange-2064063-6.patch queued for re-testing.
Comment #14
Wim Leers6: ckeditor_debounce_onchange-2064063-6.patch queued for re-testing.
Comment #15
Wim LeersOn the 4th try no more false negatives — yay! Back to RTBC. (I just wanted to confirm that the patch still applied.)
Comment #16
Dries CreditAttribution: Dries commentedThanks. Committed to 8.x.