ckeditor5.dialog.fix.js throws "Uncaught TypeError: event.target.classList is undefined" in Firefox in Drupal 10 with the editor in a modal

Created a patch for the issue mentioned, please review.
Thank You.

Fixed CCF, please review.

Fixed CCF, please review.
Attaching Interdiff.

I have tested patch #7. I forked the issue and created a new branch to apply the patch to and test it on. I installed the add_content_modal contrib module, used devel_generate to create some articles, configured the add_content_modal so i can edit the article content type in a modal.

See screenshot for the error message in the console and the ckeditor field loaded in a modal window. The error is 'Uncaught TypeError: event.data is undefined'.

The issue in #2 was created by me, so I can confirm I was running into this issue twice now.

Any ideas how it can come to this situation?

Hi @Anybody I think that if this is fixed the fix should work whether or not the admin_menu is enabled.

MR !3951 works for me repeating the test I describe at #11. No error messages in browser console when I switch focus between the ckeditor fields and another browser window.

@Anybody perhaps you could test MR !3951 with the admin menu enabled..

Should only be marked fixed by a committer when the fix is merged.

But this was previously tagged for tests which still need to happen.

@smustgrave Well, that may be correct. I understood that if i mark fixed it gets merged if there is no intervention for 2 weeks. This is not really a major issue. It generates an error in console but does not affect any functionality. CKeditor5 is 'experimental' in any case, is it not? Anyway let's hope others will test it. The term 'tests' is confusing. I mean this 'fix' just passed the automated tests. So can you define what you mean by 'tests'? If you mean the kind of 'tests' that are defined in the 'steps to reproduct' and that I define at #11, then great if numerous contributers test it. The more the better!

In which browser is this happening?

For me it was Firefox

@Wim Leers, I reproduced the error in Firefox 112.0.2. Also I tested my MR !3951 in the same and the error disappears. I switched focus between all of the fields in the node (inside a modal with ckeditor5 being used on the body field) and no errors triggered. The code change required is JQuery. I studied the JQuery documentation regarding the methods involved but the fix I got by googling how to fix 'Uncaught TypError undefined' error.

Firefox twice.

Can you also try in another browser? That'd help to rule out a browser-specific bug as the cause.

The screenshot shows a different? error triggered in Safari Version 16.2.

#24

Merge request !3951 tested in Chrome version 112.0

Merge request !3951 tested in Safari 16.2

Merge request !3951 tested in Firefox 113.0

If you can use DDEV-LOCAL, here are most of the command line steps I used after cloning the repository, forking the issue and checking out the branch:

DDEV steps to reproduce

ddev config --project-type=drupal10
ddev start
ddev composer install
ddev composer require drush/drush
ddev drush site:install --account-name=admin --account-pass=admin -y
ddev launch
ddev composer require 'drupal/add_content_modal:^1.0'
ddev drush en add_content_modal
ddev composer require 'drupal/devel:^5.0'
ddev drush generate-content 20
ddev drush cr

You will have to go to disable aggregation of css and js, clear all caches
Configure the add_content_modal: enable it for at least one content type

Then when editing one of the generated basic pages or articles, it should open in a modal (make sure you have configured this in add_content_modal settings. Then make sure that e.g. the body field of the content type is configured to use ckeditor5. Then move cursor focus between the various fields of the node edit form in the modal. Then click on another browser tab and return. Inspect using browser tools, selecting the 'console' tab. Make sure error are displayed in the console. Repeat in different browsers.

Did not see tests in the MR

@smustgrave

This is one example core test that seems to cover everything I could possibly create a test for:

core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php

It not only creates a text field for a node and activates ckeditor5 for that field, it inserts text into the text field using every possible button in the toolbar of the ckeditor and saves the result..

And it is one of 20 'FunctionalJavascript' tests. I am assuming that all these tests are run when anyone runs automated testing? Are you expecting in addition to all such tests me to write a custom one? If so, what do i need to test that is not being tested already? Or are these core tests not triggered when choosing 'Add test / re-test'?

If they are triggered then for overall functionality of the ckeditor5 module then these tests seem comprehensive. The error I am trying to fix though triggering an error does not seem to break any functionality so by in addition, manually viewing the browser's console, i would expect the combined test coverage to be as complete as it ever could be?

Probably would leave up to submaintainer and committer if they would merge without test coverage but it is possible to test for console errors. There may be a FunctionalJavascript function for this or it could be nightwatch. Would have to double check that.

@smustgrave Yes I need to re-read the original description of this issue. I can install the add_content_modal:^1.0 module, make it a test dependency. I will also have a look at nightwatch- see if it can supply any testing needs.

Also posted in the slack channel for wimleers to take a look.

@smustgrave, thank you for posting in Slack. There are indeed assertions in Nightwatch that test browser tools console output.

Could you also try to reproduce this error in Chrome please? 🙏

Drupal's automated JavaScript tests use Chrome. If we can reproduce it in Chrome, an automated test is possible. Otherwise, it's impossible.

Great work here so far BTW! 👏😊

@Wim Leers The error does not appear in Chrome. I see that the nightwatch.conf.js file in core only sets up a chrome environment. But not currently Firefox or Safari. To test the error using Nightwatch I would have to add an environment for these other browsers.

@smustgrave Is that something that is permissible? Or is the core nightwatch config restricted to Chrome only? If Chrome only then it seems as @Wim Leers says creatoin of an automated test for this issue is not possible.

Alright, it's not possible to test then 👍

The fix seems reasonable.

Very much looking forward to the fix in 10.1.x. Will this be part of the next minor release (possibly 10.1.1?) Current broken state of WYSIWYG editor functionality in 10.1.0 is a real issue for editors.

Changing the priority to major would be sufficient in this case, I guess, due to the broken key functionalities, like linking content (through Linkit) but won't change anything here. So I'll leave it as is and just say THANK YOU ALL for fixing this :)

As the MR patch doesn't apply against 10.1.0 I guess this will needs rerolls against 10.1.x and 11.x?
Should we do that in separate MR's or how to proceed here best @Wim Leers?

Or will this be first committed against 10.0.x now?

The MR diff applies against 11.x for me, however I think this needs an inline comment.

Hi @catch I have added comments.

 // Fixes a console error message generated in Firefox only
 // @see https://www.drupal.org/project/drupal/issues/3351600

I can edit them if they are not clear enough.

I think we need the MR to be against 11.x, but I can't change that. Comment was added. @catch for help? ;)

Uploading the static patch from MR!3951, if anyone needs it for composer and confirming it still applies and works with Drupal 10.1.1 and Drupal 11.

So re-confirming RTBC from #39 once this is rebased / retargeted.

Running into the same issue with 9.5

#45 still works in Drupal 10.1.4.
I will not change the Status, as I don't know if it has been rebased / retargeted.

New merge request against 11.x, back to RTBC :)

I don't think this needs a frontend framework manager review. This is a straight forward bug fix targeting a specific browser that we don't have as part of our testing infrastructure.

Committed 8b0a91f and pushed to 11.x. Also cherry-picked to 10.2.x and 10.1.x. Thanks!

It seems that the patch #46 does not writes to the proper place. This one should work well.

The above patch did not apply to 9.5.11 installation.

Attached a new version without code changes