Problem/Motivation

I'm finding this code to be problematic:

function ckeditor5_library_info_alter(&$libraries, $extension) {
[...]
  if ($extension === 'system') {
    $libraries['base']['dependencies'][] = 'ckeditor5/ie11.user.warnings';
  }
[...]
}

It's effectively adding this library and its dependencies (including jQuery UI) to every page, even for anonymous users.

On my site, it was causing a flash of unstyled content for every anonymous page load.

Steps to reproduce

1.Install CKEditor5
2. Visit any page as an anonymous user, see a large list of JavaScript files added to the page due to the "ckeditor5/ie11.user.warnings" dependencies.

Proposed resolution

Only add this library when needed.

Remaining tasks

Write a patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

larowlan’s picture

I wonder if there's an option to make this configurable.
Folks starting a project now are unlikely to even need IE11 support given it's EOL before the middle of the year.

Wim Leers’s picture

Title: CKEditor5 adds ie11.user.warnings library to every page » CKEditor5 adds ie11.user.warnings library to every page, triggering a FOUC even for anonymous users
Issue tags: +JavaScript, +front-end performance
Related issues: +#3232550: Improve messaging about Internet Explorer 11

#2: That's exactly what's being discussed in #3232550: Improve messaging about Internet Explorer 11. Please chime in there!

@samuel.mortenson Woah good catch!

Wim Leers’s picture

  1. It's possible that #3265230: Refactor ie11.filter.warnings.es6.js to simpler structure & other improvements will fix this. Nope, it just changes the JS, but the JS will still be loaded.
  2. It's certain that #3261585: Remove IE11 warning for CKEditor 5 in Drupal 10, since Drupal 10 does not support IE anyway will fix this in Drupal 10: it won't exist anymore 🤓

But we still need to fix this in Drupal 9.4 for sure.

Bumping to Major because it affects every page.

Wim Leers’s picture

Issue tags: +stable blocker

It seems pretty clear that this must be stable-blocking.

Wim Leers’s picture

longwave’s picture

Can we add it to drupal.editor or drupal.filter instead?

longwave’s picture

Status: Active » Needs review
FileSize
1.7 KB

It already relies on drupal.editor so let's try attaching it to that instead, so it is only loaded if there is an editor on the page. There is some test coverage for this feature, which is hopefully enough.

Wim Leers’s picture

Title: CKEditor5 adds ie11.user.warnings library to every page, triggering a FOUC even for anonymous users » CKEditor 5 adds ie11.user.warnings library to every page, triggering a FOUC even for anonymous users
Issue tags: +Needs manual testing
Related issues: +#3206686: IE11 warning for CKE5 in Drupal 9

Ohhhh, good idea! 🤩

Then again … I wonder if we're missing something here, because the surrounding code in ckeditor5_library_info_alter() suggests that this is something @bnjmnm would've thought about… so I did some digging.

This code was added in #3206686: IE11 warning for CKE5 in Drupal 9, specifically in https://git.drupalcode.org/project/ckeditor5/-/merge_requests/20/diffs?c....

I'm relieved to report that I did spot this in my review: https://git.drupalcode.org/project/ckeditor5/-/merge_requests/20#note_20543 — but my review was limited to 🤯 — yes, just the emoji 🤣 This was the early days of stabilizing the CKEditor 5 module, so this was not important back then.

So … I think that this probably indeed was a legitimate oversight, and that the solution that @longwave proposed here is fine.

If it comes back green, this is an obvious improvement. This needs to be tested manually in IE11 though, because

+++ b/core/modules/ckeditor5/ckeditor5.libraries.yml
@@ -76,7 +76,6 @@ ie11.user.warnings:
-    - editor/drupal.editor

will AFAICT result in this code:

(function (Drupal, Modernizr) {
  var isIE11 = Modernizr.mq('(-ms-high-contrast: active), (-ms-high-contrast: none)');

  if (isIE11) {
    var quickEditLabelObserver = null;
    Drupal.editors.ckeditor5 = {
…

failing, but only on IE11. Because on any other browser it returns early. Only on IE11 does it try to interact with Drupal.editors.ckeditor5, but that will not yet be defined.

I think therefore the correct solution is to alter the list of JS assets loaded by the editor/drupal.editor asset library… and that is probably why @bnjmnm went with this specific approach!

Status: Needs review » Needs work

The last submitted patch, 8: 3260032-8.patch, failed testing. View results

longwave’s picture

So the Quick Edit test fails because the editor is not present on the page at all to start with, and is only loaded in over Ajax; this is the scenario described in the comment. I tried adding the JS in a pre render callback, which solves the loading order, but ultimately this runs into the same issue.

    // Attach Text Editor module's (this module) library.
    $element['#attached']['library'][] = 'editor/drupal.editor';

    // Attach attachments for all available editors.
    $element['#attached'] = BubbleableMetadata::mergeAttachments($element['#attached'], $this->pluginManager->getAttachments($format_ids));

This code in the text_format pre render callback ensures that drupal.editor is loaded along with any editor attachments, so we could just load it directly as an attachment; but if we can't guarantee this will work when CKEditor 5 is attempted to be loaded over Ajax, I'm not sure to make this work in the Quick Edit case without preloading it on every page.

I guess this needs manual testing with IE11 and Quick Edit to see what happens.

bnjmnm’s picture

FileSize
854.92 KB
693.54 KB
224.74 KB
854.92 KB

When #3206686: IE11 warning for CKE5 in Drupal 9 landed, CKEditor 5 would fail gracefully in both ajax loaded instances (like Quick Edit) and via the initial request (such as a node form). In both cases, CKEditor 5 would not be available, but the field would be a textarea that accepted edits.

In manual testing I discovered that this isn't quite as I recall. There are some issues in HEAD
and some introduced here.

Problem introduced with patch #8:
In HEAD, Quick Edit CkEditor 5 uses are still failing gracefully, and the field is an editable text area. With #8, we now get an angry IE11

If we step past the IE error, the field then appears editable, but it is not.

Issue discovered here occurring in HEAD and #8: (this wasn't happening when #3206686: IE11 warning for CKE5 in Drupal 9 landed, but it's not clear when it started).

When IE11 tries to get CKEditor 5 that loads with page, it's no longer graceful

It's still possible to click through the errors and get to a working textarea with message

I point this out although it's present in HEAD as it may help identify how we need to approach this overall.

longwave’s picture

Thanks for the manual testing. I suspected #8 wouldn't work directly as suggested in the comment and then reasoned in #11, but still I'm not sure what the correct solution is.

I suggest that the error discovered in HEAD is first fixed in a separate issue, and then we come back to this one.

bnjmnm’s picture

Good idea @longwave. I added #3266310: IE11 user warning has ungraceful failures. Even if the fix in the MR isn't the way to go, that should get us close?

lauriii’s picture

Wim Leers’s picture

Yay, now we can finally do this one 🤓

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
535.94 KB
1.77 KB

Since the killswitch approach was deemed acceptable in #3260032: CKEditor 5 adds ie11.user.warnings library to every page, triggering a FOUC even for anonymous users, we don't need to go to the same lengths to prevent IE11 from loading
(site-crashing) incompatible JS. That means the excessive "load everywhere" hack isn't needed, and ckeditor5/ie11.user.warnings can just be added as a dependency of ckeditor5/drupal.ckeditor5.

Quick Edit message again works:

longwave’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
37.31 KB

Great - this makes it all much cleaner!

Confirmed that the warning shows up correctly in IE11 with this patch applied, even when a CKEditor 5 is loaded late over Ajax:

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 3260032-17.patch, failed testing. View results

tim.plunkett’s picture

Quickedit is asserting that ie11.user.warnings.js is loaded :\

longwave’s picture

Status: Needs work » Needs review
FileSize
2.94 KB

Now the JS is loaded on demand, we can't assert its existence until Quick Edit has been triggered.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Oh, great! Thanks @longwave

Wim Leers’s picture

Wow, epic improvement here! 😄 Hopefully this is the last time IE makes our lives more difficult as web developers… 🤞

  • lauriii committed 77657fb on 10.0.x
    Issue #3260032 by longwave, bnjmnm, Wim Leers, samuel.mortenson:...

  • lauriii committed 81717f7 on 9.4.x
    Issue #3260032 by longwave, bnjmnm, Wim Leers, samuel.mortenson:...
lauriii’s picture

Version: 9.4.x-dev » 9.3.x-dev

Committed 77657fb and pushed to 10.0.x. Also cherry-picked to 9.4.x. Thanks!

Leaving open for a 9.3.x backport once the freeze is over.

  • bnjmnm committed d7aa8c4 on 9.3.x authored by lauriii
    Issue #3260032 by longwave, bnjmnm, Wim Leers, samuel.mortenson:...
bnjmnm’s picture

Status: Reviewed & tested by the community » Fixed

Cherry-picked to 9.3.x

Status: Fixed » Closed (fixed)

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