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

  1. 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

  2. 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

Issue fork drupal-3231337

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

Wim Leers’s picture

Title: Remove manual dataDowncast from DrupalMediaEditing » [PP-1] Remove manual dataDowncast from DrupalMediaEditing
Related issues: +#3206522: Add FunctionalJavascript test coverage for media library

#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.

Wim Leers’s picture

Issue summary: View changes
Status: Active » Postponed
Wim Leers’s picture

Title: [PP-1] Remove manual dataDowncast from DrupalMediaEditing » [drupalMedia] Remove manual dataDowncast from DrupalMediaEditing
Project: CKEditor 5 » Drupal core
Version: 1.0.x-dev » 9.3.x-dev
Component: Code » ckeditor5.module
Status: Postponed » Active
Issue tags: +JavaScript

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lauriii’s picture

Priority: Normal » Major
Issue tags: +stable blocker

This 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.

Wim Leers’s picture

Title: [drupalMedia] Remove manual dataDowncast from DrupalMediaEditing » [PP-1] [drupalMedia] Remove manual dataDowncast from DrupalMediaEditing
Status: Active » Postponed
Issue tags: +Needs issue summary update
Related issues: +#3227822: [GHS] Ensure GHS works with our custom plugins, to allow adding additional attributes

#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.

Wim Leers’s picture

Title: [PP-1] [drupalMedia] Remove manual dataDowncast from DrupalMediaEditing » [drupalMedia] Remove manual dataDowncast from DrupalMediaEditing
Status: Postponed » Active

Wim Leers’s picture

I 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:

  1. still do a shallow clone of the element
  2. set a very-long-attribute-name-that-signals-that-we-are-stringifying-for-internal-reasons attribute on it
  3. detect the presence of this element in our editingDowncast logic, to f.e. not do the link DOM manipulation
lauriii’s picture

I'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 want data-caption to be part of the preview.

Wim Leers’s picture

#11: my proposal in #10 was attempting to get rid of the side effects.

Wim Leers’s picture

Status: Active » Reviewed & tested by the community

bnjmnm made their first commit to this issue’s fork.

bnjmnm’s picture

bnjmnm’s picture

bnjmnm’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

  • bnjmnm committed 1ccdb3b on 10.0.x
    Issue #3231337 by lauriii, Wim Leers: [drupalMedia] Remove manual...

  • bnjmnm committed 270047b on 9.4.x
    Issue #3231337 by lauriii, Wim Leers: [drupalMedia] Remove manual...

  • bnjmnm committed 13d1108 on 9.3.x
    Issue #3231337 by lauriii, Wim Leers: [drupalMedia] Remove manual...
bnjmnm’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.