Problem/Motivation
DrupalMediaEditing plugin is manually downcasting the element for preview.
This is a stable blocker because this prevents arbitrary attributes from being sent to the server as part of the element that is used for rendering the preview. This could lead into inaccurate previews in the case that an arbitrary attributes are required for rendering the preview on the server side.
Proposed resolution
Improve this so that the manual step isn't needed, and the model is downcasted properly.
Remaining tasks
Delete
/** * MediaFilterController::preview requires the saved element. * Not previewing data-caption since it does not get updated by new changes. * @todo: is there a better way to get the rendered dataDowncast string * https://www.drupal.org/project/ckeditor5/issues/3231337? */ const renderElement = (modelElement) => { const attrs = modelElement.getAttributes(); let element = '<drupal-media'; Array.from(attrs).forEach((attr) => { if (attr[0] !== 'data-caption') { element += ` ${attr[0]}="${attr[1]}"`; } }); element += '></drupal-media>'; return element; };()done
Update
this._fetchPreview(this.previewURL, { text: renderElement(modelElement), uuid: modelElement.getAttribute('data-entity-uuid'), }).then(({ label, preview }) => {. (
done)
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3231337-16-d93x.patch | 73.56 KB | bnjmnm |
| #15 | 3231337-15-d94.patch | 73.76 KB | bnjmnm |
| #15 | 3231337-15-d10.patch | 73.61 KB | bnjmnm |
Issue fork drupal-3231337
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3231337-drupalmedia-remove-manual
changes, plain diff MR !1972
Comments
Comment #2
wim leers#3206522: Add FunctionalJavascript test coverage for media library should land first; that'll make this much simpler to achieve, since we'll have tests proving that everything still works correctly.
Comment #3
wim leersComment #4
wim leers#3206522: Add FunctionalJavascript test coverage for media library landed :)
Comment #6
lauriiiThis is a stable blocker because this prevents arbitrary attributes from being sent to the server as part of the
<drupal-media>element that is used for rendering the preview. This could lead into inaccurate previews in the case that an arbitrary attributes are required for rendering the preview on the server side.Comment #7
wim leers#6++ — that is way more clear than the description in the issue summary! :D
This should land after #3227822: [GHS] Ensure GHS works with our custom plugins, to allow adding additional attributes.
Comment #8
wim leers#3227822: [GHS] Ensure GHS works with our custom plugins, to allow adding additional attributes landed!
Comment #10
wim leersI wonder if a more elegant work-around for the undesirable side effects that using https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_controller_... has would be to:
very-long-attribute-name-that-signals-that-we-are-stringifying-for-internal-reasonsattribute on iteditingDowncastlogic, to f.e. not do the link DOM manipulationComment #11
lauriiiI'm not sure what is #10 trying to solve? We still need to remove the link attribute since we need to be able to render the
<drupal-media>element without the wrapping<a>. The shallow (or a deep) clone of the element already resolves the issues with side effects. I think we want to continue to use shallow clone since we don't wantdata-captionto be part of the preview.Comment #12
wim leers#11: my proposal in #10 was attempting to get rid of the side effects.
Comment #13
wim leersComment #15
bnjmnmComment #16
bnjmnmComment #17
bnjmnmComment #21
bnjmnmCommitted to 10.0.x / 9.4.x and backported to 9.3.x since CKEditor 5 is experimental and improving the media preview downcast is an unambiguously desirable change.