Problem/Motivation

Discovered whilst working on #3227033: Remove Quick Edit from core

core/modules/ckeditor5/tests/src/Functional/CKEditor5QuickEditLibraryTest.php tests the integration of CKEditor5 with Quick Edit.
Since Quick Edit is on it's way out of core (and live long and prosper in Contrib) we need to move this file and namespace to the land of quickedit

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3291018-5-suggestions-do-not-test.patch2.54 KBwim leers

Issue fork drupal-3291018

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:

About issue forks

Comments

Spokje created an issue. See original summary.

Spokje opened merge request !2390

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Priority: Normal » Major
Status: Active » Needs review

Since this is a blocker for (at least) #3227033: Remove Quick Edit from core, which has Priority Major, setting this one to that prio as well.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Related issues: +#3194048: Enable Quick Edit support, +#3196689: Remove legacy Quick Edit JS style changes

Zero remarks 🚀

Ugh … this is moving a test that asserts that the work-around CSS for making CKEditor 5 work correctly in Quick Edit gets loaded when necessary. That work-around was introduced in #3194048: Enable Quick Edit support because #3196689: Remove legacy Quick Edit JS style changes is not yet fixed.

But if we're moving the test for the work-around out of ckeditor5.module, I think we should also move the work-around out of ckeditor5.module and into quickedit.module. Otherwise, the CKE5 module is providing the fix and the Quick Edit module is doing the test. That makes no sense, right?

So, I think that we need to:

  1. move the drupal.ckeditor5.quickedit-temporary-work-around library + CSS from ckeditor5.libraries.yml to quickedit.libraries.yml, and remove all references to it from the CKEditor 5 module.
  2. The above didn't yet ensure it gets loaded when necessary. Theoretically it'd be fine to load always since it's such a tiny amount of CSS that is also specifically scoped to CKEditor 5 selectors. Better: just load it whenever quickedit.inPlaceEditor.formattedTextis loaded: merge the files of drupal.ckeditor5.quickedit-temporary-work-around into quickedit.inPlaceEditor.formattedText.
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module

… and I missed in #4 that there's also core/modules/ckeditor5/css/quickedit.css. That too should move into the Quick Edit contrib module now. And could also always be loaded.

Ah but … we have a similar problem for the CKEditor module: core/modules/ckeditor/css/ckeditor.css contains similar styles.

Splitting that off into a separate issue: #3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3291018-5-suggestions-do-not-test.patch2.54 KB

To help get you going on #4, I was thinking of something like this.

(Don't want to touch the MR so I can still RTBC 🤓)

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Two remarks:

1) I couldn't get the original patch by @Wim Leers to pick up the "work-around CSS" all the time, so I resorted to the "if CKEditor5 module enabled" method, previously used the other way around ("if Quick Edit module enabled").
2) This might be a prime candidate to skip the Stable and Stable9 override CSS, since it's a temporary(?) workaround and having to use Stable/Stable9 means the original deprecation message can't be used any more.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

🚢

  • catch committed 1c5c378 on 10.0.x 
    Issue #3291018 by Spokje, Wim Leers: Move \Drupal\Tests\ckeditor5\...
  • catch committed dc4e2a8 on 9.5.x
    Issue #3291018 by Spokje, Wim Leers: Move \Drupal\Tests\ckeditor5\...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x and 9.5.x, thanks!

Spokje closed merge request !2390

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Fixed » Patch (to be ported)
spokje’s picture

spokje
Primary language Dutch
commented
Title: Move \Drupal\Tests\ckeditor5\Functional\CKEditor5QuickEditLibraryTest to the quickedit namespace/directory » [PP-2] Move \Drupal\Tests\ckeditor5\Functional\CKEditor5QuickEditLibraryTest to the quickedit namespace/directory
Assigned: spokje » Unassigned
Status: Patch (to be ported) » Postponed

The plain diff from MR!2390 should apply cleanly on 9.4.x _after_ the commit of #3267258: Remove Quick Edit support from editor.module and #3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module in that specific order.

PP-2 for those two.

No deprecation errors to adjust in this one, but we need to update the CR when this gets committed.

spokje’s picture

spokje
Primary language Dutch
commented
Title: [PP-2] Move \Drupal\Tests\ckeditor5\Functional\CKEditor5QuickEditLibraryTest to the quickedit namespace/directory » Move \Drupal\Tests\ckeditor5\Functional\CKEditor5QuickEditLibraryTest to the quickedit namespace/directory
Version: 9.5.x-dev » 9.4.x-dev
Status: Postponed » Fixed

Not backporting this any more.

Status: Fixed » Closed (fixed)

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