Problem/Motivation

https://github.com/ckeditor/ckeditor5/releases/tag/v37.0.0
https://github.com/ckeditor/ckeditor5/releases/tag/v37.0.1
https://github.com/ckeditor/ckeditor5/releases/tag/v37.1.0

👇 Highlights:

  • Fully converted to TypeScript — does not affect Drupal core at all, but means a better DX for plugin developers 👍 This is something they've been working on for a very long time, and it will enable them to iterate faster in the future, with more confidence that no BC breaks are introduced! 🚀
  • #3339763: [upstream] CKEditor 5 mangles table structure should be fixed — at least partially (see #3339763-11: [upstream] CKEditor 5 mangles table structure).
  • 37.1.0 made a change to <span lang="fr">Bonjour</span> — that'd have been italic until this release. This may require us adding .ck-content span[lang] { font-style: italic; } to core/modules/ckeditor5/css/language.css … or not. Because we already have this in place:
    .ck-content [lang] {
  outline: 1px dotted gray;
}

    Arguably this is actually a net improvement, and avoids user confusion — that's also why this was done in the first place: https://github.com/ckeditor/ckeditor5/issues/12672.

    That needs a decision from a front-end framework manager.

  • Many bugfixes!

Steps to reproduce

Proposed resolution

  1. Update core/package.json
  2. cd core
  3. yarn install
  4. yarn build
  5. yarn build:ckeditor5-types

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CKEditor has been updated to 37.1.0

Issue fork drupal-3355358

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

Wim Leers created an issue. See original summary.

Wim Leers opened merge request !3856

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
Issue tags: -JavaScript +JavaScript
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Issue tags: -JavaScript +JavaScript, +CSS, +Needs manual testing, +Needs frontend framework manager review

Analyzed the release notes. AFAICT we need manual testing for:

  1. Table ("Table") plugin: #3339763: [upstream] CKEditor 5 mangles table structure — verify if it is fixed in its entirety or partially
  2. TextPartLanguage ("Language") plugin: 37.1.0 made a change to <span lang="fr">Bonjour</span> — that'd have been italic until this release. This may require us adding .ck-content span[lang] { font-style: italic; } to core/modules/ckeditor5/css/language.css … or not. Because we already have this in place:
    .ck-content [lang] {
  outline: 1px dotted gray;
}

    Arguably this is actually a net improvement, and avoids user confusion — that's also why this was done in the first place: https://github.com/ckeditor/ckeditor5/issues/12672.

    That needs a decision from a front-end framework manager. Tagging accordingly.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs frontend framework manager review

I think we want to refrain from making the text italic to benefit from the UX fix. It looks like we have an alternative affordance in place already so that's good 👍

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

I agree the outline is an improvement as it is visually distinguishing the [lang] use in a manner that won't be confused with intentionally italicized text.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -CSS

Excellent, thanks both of you! Then also untagging CSS.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Applied the MR locally and following #3339763: [upstream] CKEditor 5 mangles table structure

Pasted

<table>
  <tr>
    <th>Header</th>
    <th>Header</th>
    <th>Header</th>
  </tr>
  <tr>
    <th>Header</th>
    <td>Data</td>
    <td>Data</td>
  </tr>
</table>

and got

<table>
    <tbody>
        <tr>
            <th>
                Header
            </th>
            <th>
                Header
            </th>
            <th>
                Header
            </th>
        </tr>
        <tr>
            <th>
                Header
            </th>
            <th>
                Data
            </th>
            <th>
                Data
            </th>
        </tr>
    </tbody>
</table>

Gross

Applied the patch and tried again but this time I got

<table>
    <thead>
        <tr>
            <th>
                Header
            </th>
            <th>
                Header
            </th>
            <th>
                Header
            </th>
        </tr>
    </thead>
    <tbody>
        <tr>
            <th>
                Header
            </th>
            <td>
                Data
            </td>
            <td>
                Data
            </td>
        </tr>
    </tbody>
</table>

Which seems correct as the th's were wrapped in a thead vs tbody like before.

Spot checked some other plugins but didn't notice anything.

  • catch committed 8e474357 on 10.1.x 
    Issue #3355358 by Wim Leers, lauriii, bnjmnm, smustgrave: Update...
catch’s picture

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

Let's get this in before the alpha so it gets as much testing as possible.

Committed/pushed to 10.1.x, thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs manual testing

Thanks, @smustgrave & @catch!

Status: Fixed » Closed (fixed)

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