Follow-up to #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5
Problem/Motivation
CKEditor dialog are not alterable, the patch in #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5 has a fix
Proposed resolution
Use parts of the patch in #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5
Remaining tasks
- Split the patch.
- Get it committed
User interface changes
None
API changes
None
Related Issues
#2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff-13-16.txt | 1.45 KB | Jelle_S |
#16 | i2579979-16.patch | 6.55 KB | Jelle_S |
Comments
Comment #2
attiks CreditAttribution: attiks at Attiks commentedPatch containing the changes to ckeditor
Comment #3
attiks CreditAttribution: attiks at Attiks commentedIf this gets committed, please credit Jelle_S as well
Comment #4
webchickAdding the nod_ summoning tag.
Comment #5
attiks CreditAttribution: attiks at Attiks commentedDiscussed with Jelle_S, we will change the allowedContent as well so it uses the object notation
Comment #6
Jelle_SComment #7
Jelle_SNew patch with the allowedContent converted to
CKEDITOR.style
.Comment #10
Jelle_SComment #11
attiks CreditAttribution: attiks at Attiks commentedThis is working and much easier for conyrib to change it, marked as RTBC and assigned to nod_ for final go
Comment #12
nod_missing
new CKEDITOR.style()
?Other than that, ok with me.
Comment #13
Jelle_SI think this should fix the remarks in #12.
Comment #14
nod_Would be better to rename the second iteration variable, that would solve the var eslint errors.
Comment #15
nod_(duplicate)
Comment #16
Jelle_SNew patch, got no eslint errors here.
EDIT: also removed a
.trim()
since the.split()
regexp already takes care of that.Comment #17
nod_good to go.
Comment #18
Wim LeersThis is plain wrong. You can totally alter the image form. Because it's a form like any other.
What is missing, is the ability for any additional (custom) attributes that those form alterations may add, to persist on the images. Which means this is JS land, and we cannot write test coverage for this (removing the
Needs tests
tag because of that).I very much support this, but it should be manually tested. Given that @nod_ RTBC'd it, I'm assuming he did the necessary testing.
Comment #19
Wim LeersComment #20
attiks CreditAttribution: attiks at Attiks commented#18 Sorry for the wrong wording
Comment #21
Wim Leers#20: np np :) I feel very strongly about unambiguously wording things, I know many people don't feel that strongly about that.
Many thanks for doing this!
Comment #22
webchickThis seems like a very sensible "simplest thing that can possibly work" in 8.0.0 so we can make this whole are shine more in 8.1.x. Great work.
Committed and pushed to 8.0.x. Thanks!
Comment #24
DuaelFrThat fix had side effects: it unlikely fixed #2578957: (regression) "Open in new window" checkbox does not work anymore
Comment #25
Wim LeersThis caused a nasty regression: #2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI. Seems like @nod_ did not do the necessary manual testing :(
Comment #26
Jelle_SFYI: Just uploaded a patch over at #2585173: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI. Should fix the issue.
Comment #27
BerdirI'm getting strange JS errors after this:
Uncaught TypeError: Cannot set property 'float' of undefined
Looks like styles is not initialized, but I have no idea what's different in my case that it only happens for me?
This for example happens when switching from an text format without drupalimage to one that has it enabled and completely breaks wysiwyg then.
The following fixes it for me, I can open a follow-up tomorrow, but I don't know enough JS to know if the problem is really somewhere else?
Comment #28
attiks CreditAttribution: attiks at Attiks commentedFixed in #2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats
Comment #30
Wim LeersAnother regression: #2598070: [regression] CKEditor Link button does not show if HTML filtering is enabled.