Problem/Motivation

https://github.com/ckeditor/ckeditor5/releases/tag/v35.0.1

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#22 interdiff-cke-19-22.txt756 bytesxjm
#22 cke-3301495-22-10x.patch3.41 MBxjm
#19 interdiff-10x.txt819 byteslauriii
#19 3301495-19-10x.patch3.41 MBlauriii
#19 3301495-19-95x.patch3.41 MBlauriii
#12 3301495-8-10x.patch3.41 MBlauriii
#12 3301495-8-95x.patch3.41 MBlauriii
#8 interdiff.txt2.55 KBlauriii
#8 3301495-8-93x.patch3.41 MBlauriii
#8 3301495-8-94x.patch3.41 MBlauriii
#4 interdiff.txt3.38 KBlauriii
#4 3301495-4-93x.patch3.41 MBlauriii
#4 3301495-4-94x.patch3.41 MBlauriii
#2 drupalImage-diff.txt736 byteslauriii
#2 3301495-2-93x.patch3.4 MBlauriii
#2 3301495-2-94x.patch3.4 MBlauriii

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Needs review
StatusFileSize
new 3301495-2-94x.patch3.4 MB
new 3301495-2-93x.patch3.4 MB
new drupalImage-diff.txt736 bytes
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

Everything looks good, only missing the types updates

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3301495-4-94x.patch3.41 MB
new 3301495-4-93x.patch3.41 MB
new interdiff.txt3.38 KB

Addressed #3.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

all good :)

The last submitted patch, 2: 3301495-2-94x.patch, failed testing. View results

The last submitted patch, 2: 3301495-2-93x.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new 3301495-8-94x.patch3.41 MB
new 3301495-8-93x.patch3.41 MB
new interdiff.txt2.55 KB

Addressing test failures.

The failure in \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::testFullHtml was caused by changes in attributes order.

The failure in \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testEditableCaption was likely due to accessibility improvements in CKEditor 5.

nod_’s picture

nod_
Primary language French
Location Lille
commented

That works for me, hopefully tests are green

The last submitted patch, 4: 3301495-4-94x.patch, failed testing. View results

The last submitted patch, 4: 3301495-4-93x.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new 3301495-8-95x.patch3.41 MB
new 3301495-8-10x.patch3.41 MB

  • xjm committed 689828d on 9.4.x 
    Issue #3301495 by lauriii, nod_: Update CKEditor 5 to 35.0.1

nod_ credited xjm.

nod_’s picture

nod_
Primary language French
Location Lille
commented

RTBC+1 of new patches

  • xjm committed 8f565e0 on 9.3.x 
    Issue #3301495 by lauriii, nod_: Update CKEditor 5 to 35.0.1
xjm’s picture

xjm
she/her
Primary language English
commented

I reviewed the 9.4.x and 9.3.x patches by following the handbook instructions for updating CKEditor 5 and applying the interdiff from #8. Confirmed I got the same patches. Those are committed now to the respective branches. I'll wait for tests to finish for the other three branches. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 3301495-8-10x.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3301495-19-95x.patch3.41 MB
new 3301495-19-10x.patch3.41 MB
new interdiff-10x.txt819 bytes

Applied interdiff from #8 and skipped test from #3274937: Get CKEditor 5 to work in (modal) dialogs because of a regression in 10.x.

xjm’s picture

xjm
she/her
Primary language English
commented

Fixing saved credits; crosspost ate @nod_'s credit or such.

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#3301631: Regression with CKEditor 35.0.1 and modal dialogs

Filed #3301631: Regression with CKEditor 35.0.1 and modal dialogs to un-skip and fix the test.

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new cke-3301495-22-10x.patch3.41 MB
new interdiff-cke-19-22.txt756 bytes

Attached just adds an inline @todo comment referencing the followup.

Status: Needs review » Needs work

The last submitted patch, 22: cke-3301495-22-10x.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review

10.0.x fail is a known random; requeuing.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I reviewed #19 as per #17 and everything is good. I agreed with @lauriii on skipping the test so long as we have the followup issue as a CKE5 stable blocker.

If someone else verifies and +1s the RTBC, I can commit this.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs review

The todo looks correct, pointing to the existing issue.

I then tried to verify the latest patch, same as in #17 and was not able to do so. My patch was 2 MB larger. I finally realized it was because jquery ui assets were being added to the patch. After a discussing this with xjm in Slack, she tracked the problem down to the recent commit for Terser. That resulted in that commit being reverted, #3296481-22: Update terser and terser-webpack-plugin to the latest versions,

Once that was reverted I successfully built a patch matching the patch #22. That was all on 10.0.x so I can confirm that is fine.

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

I have repeated the process for 9.5.x and 10.1.x. I got the same patch for 9.5.x as in #19 and the same as for 10.1.x in #22. So, I can confirm the patches for 9.5.x, 10.0.x and 10.1.x.

Curious, I don't recall changing the status when making the previous comment.

  • xjm committed b68ad46 on 10.1.x 
    Issue #3301495 by lauriii, xjm, nod_, quietone: Update CKEditor 5 to 35....

  • xjm committed a79231f on 10.0.x 
    Issue #3301495 by lauriii, xjm, nod_, quietone: Update CKEditor 5 to 35....

  • xjm committed 33edc28 on 9.5.x 
    Issue #3301495 by lauriii, xjm, nod_, quietone: Update CKEditor 5 to 35....
xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

OK, those are committed now to 10.1.x, 10.0.x, and 9.5.x. Phew! We have #3301631: Regression with CKEditor 35.0.1 and modal dialogs to fix the modal dialog test. Thanks!

Status: Fixed » Closed (fixed)

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: » #3277438: Update to CKEditor 5 v34.1.0