Update CKEditor 5 to 35.4.0

Given that this is a minor version update, and that there's a possibility of BC breaks, including the one listed on their release page, I don't think we should try to squeeze this into 10.0.0, since we don't have an intervening RC remaining in which to discover problems. Also, even if we squeezed this into 10.0.0, we'd still be facing the same situation in another month or two when CKE's next minor release comes out.

But, we do need to decide wether to get this into 10.0.1, or only into 10.1.0. On the one hand, putting it into a Drupal patch release risks introducing a bit of disruption, if a site is using a contrib or custom CKE plugin affected by whatever minor BC breaks are in here. On the other hand, 10.0 is going to be security supported for a year, and if during that time CKE releases a security fix, then it will only be to their latest minor version, and if 10.0 is one or more minor CKE versions behind, then either Drupal's SA for it will need to bump the CKE version several minors up, or we'll need to create a custom backport.

This is a tricky one.

Updating in a patch release is also IMO more disruptive than doing it for 10.0.0, because sites should only be updating from 9.5 to 10.0.0 (i.e. with some testing involved), whereas you expect a patch release to be entirely non-disruptive compared to a new major.

If we decide not to update to the minor release before 10.1.x, we could get caught out by a security release and have to do it anyway, which would be the worst possible way to do it.

Test failure

I bet the failure is somehow related to \Behat\Mink\Driver\Selenium2Driver::setValue(), it's called just before the failure:

    // Update table cell content.
    $assert_session->waitForElement('css', '.ck-editor__nested-editable .ck-table-bogus-paragraph')->setValue($this->tableCellText);
    $table_cell = $page->find('css', 'figure.table > table > tbody > tr > td');
    $this->assertEquals($this->tableCellText, $table_cell->getText());

it's almost as if simulated typing does not trigger the cursor getting moved in CKEditor 5?

Risk

I remember that months ago we said we did NOT want to do a CKE5 update with the a11y bug fixed that @bnjmnm spotted just days before D10 release. That’s why did that extra release a few weeks ago: just for us. Now we're literally on the day before and still considering it? 😳

I just think it’s too risky to do any non-trivial change the day before 10.0.0. We've seen subtle tweaks before, that require subtle test changes, and it looks like this test failure shows that is happening again.

I just now committed #3326874: Update to jQuery 3.6.2. I don't expect that to change anything in this issue, but just in case, I'm going to re-queue the tests. In case that causes the prior jobs' output to go away, here's the failure message from the earlier runs that #6 is referring to:

Drupal\Tests\ckeditor5\FunctionalJavascript\TableTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-47.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.27 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\ckeditor5\FunctionalJavascript\TableTest
.F                                                                  2 / 2 (100%)

Time: 00:24.309, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\TableTest::testTableCaptionUi
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'table cell'
+'llec elbat'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/TableTest.php:178
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

I remember that months ago we said we did NOT want to do a CKE5 update with the a11y bug fixed that @bnjmnm spotted just days before D10 release. That’s why did that extra release a few weeks ago: just for us. Now we're literally on the day before and still considering it? 😳

I for one didn't anticipate they'd be doing minor releases every month with no security support for the previous minor at this point. Ideally we'd only have to worry about patch updates between our own minor releases but that's not what's happening.

This should address the failing test. I was not able to reproduce this outside of tests because AFAIK whenever the cell content is being changed, it should be focused. I didn't have a chance to research the root cause for this.

@catch: Well, release schedule and major/minor/patch release cadence aside, @xjm just had indicated she absolutely did not want to update CKEditor 5 days before shipping Drupal 10, and I think we can all agree with that intent. That being said, I also understand your POV in #4: it'll be more disruptive to update later.

We've indeed always known that CKEditor 5's more aggressive semver schedule/approach would cause interesting situations, but so far it's been true that they have not had major disruptions even across their major version updates (#3231364: Add CKEditor 5 module to Drupal core started with CKEditor 5 31.0.0!), just like they had said. We can't expect them to change their entire process. But … it'd still be good to work with them to try to smoothen things out as much as possible. I'll re-start that conversation now that the pressure of getting CKEditor 5 ready for Drupal 10 is off 😊

Review

Reproduced the #2 failures locally without applying the patch, but by just following the steps to update CKEditor 5. This means I've independently reproduced the problem. 👍

This also produced an identical diff 👍

Which brings me to the last point: @lauriii's solution to make tests pass. First: THANK YOU @lauriii! Second: woah, why does this change even work? It's functionally equivalent, it just breaks it up in a few more steps. Theoretically the only difference here is that you're retrieving the table cell contents before setting a value, and perhaps that somehow triggers a different behavior in CKEditor 5 itself? 😳

I've talked to @lauriii and he's about to start a git bisect of the upstream CKEditor 5 changes to determine exactly which upstream change has triggered this difference in test behavior on our end. To be fair, 3 updates ago, we've had to make a change to exactly the same test logic — see #3318867-7: Update CKEditor 5 to 35.3.0. There too it was something that no human could ever have reproduced. The same appears to be the case here. And the update before that had a similar problem with Selenium simulating user input: see #3313946-11: Update CKEditor 5 to 35.2.1.

Conclusion: this is ready to go. Chances are vanishingly small that @lauriii will find a fundamental objection in the upstream changes; it's far more likely that it's a weakness somewhere in the Behat → Mink → Selenium → webdriver → chromedriver → Google Chrome chain, where each layer makes some choices on how to simulate end user behavior. 🚢

P.S.: IMHO part of the reason we're encountering more obscure challenges in tests when updating CKEditor 5 is simply the fact that we have more tests.

Also, I just realized, another reason that @xjm wanted this to land earlier than this week is because of that last CKE5 a11y critical. That has landed weeks ago (#3283802: Update CKEditor 5 to 35.3.2 to fix voice control/IME on some platforms). Which means this is not solving the last critical at the very last moment (which in hindsight is probably the key thing @xjm wanted to avoid), and there's now less risk in this update 👍

I don't remember where I said what Wim is quoting to me saying, but I think I was hoping to get a release for us sooner, rather than saying we should not do the update once it was available. Normally we would disallow these updates so close to the release, but @catch and @longwave and I discussed it and agreed that "before" is better than "after" this case (WRT 10.0.0): because it's CKEditor, so future security releases are likely, and especially if the dictation issue isn't fixed yet without this update. Edit: This is if the test-only patch passes for @longwave.

For followup, quoting myself from Commiter Slack:

We should have a conversation with CKSource about this. Back in like 2019 and even last year they gave the impression they expected development to slow by now, but it's still pretty aggressive and... thing-breaking.

Obviously we can't expect them to provide security coverage for SIX different minor versions between our releases, but maybe we can negotiate coordination for vulns that affect a version core is using. Similar to what we do with Symfony.

And if the backport is hard, see if Ben and Lauri and Wim can help with the upstream to mitigate the burden, the way larowlan and alexpott do for Symfony again. (Sorry that I am volunteering you for things; it's just an idea of a compromise that could work for both projects without leaving us either insecure or broken.)

Since we managed to get this green thanks to @lauriii, I think we should indeed get it in (with a release note in case there's anything helpful we can say about the internal-ish breakages from the update.

Something ate my tags.

I've committed/pushed this to 10.1.x.

Leaving RTBC for #12 to hopefully come back green. Added a stub release note that doesn't say anything useful yet.

The upstream issue that is causing this is https://github.com/ckeditor/ckeditor5/pull/12898. It looks like it could be happening because of race conditions with the events triggered by the WebDriver. This line of code seems particularly prone to race conditions. The events that are being triggered are the same before and after. The exception is that there are some mouse specific events being triggered after. It just seems that the timing of the events is different because they are being triggered slightly slower when the clicking is taking place.

#12 is green which also makes sense regarding #17 too. Cherry-picked the 10.1 commit to 10.0.x, and committed the 9.5 patch to 9.5.