Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#21 | 3260032-20.patch | 2.94 KB | longwave |
#18 | 3260032-18.png | 37.31 KB | longwave |
#17 | 3260032-17.patch | 1.77 KB | bnjmnm |
#17 | Screen Shot 2022-03-02 at 8.07.07 AM.png | 535.94 KB | bnjmnm |
#12 | qu-ie11.png | 854.92 KB | bnjmnm |
Comments
Comment #2
larowlanI 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.
Comment #3
Wim Leers#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!
Comment #4
Wim LeersIt'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.But we still need to fix this in Drupal 9.4 for sure.
Bumping to
because it affects every page.Comment #5
Wim LeersIt seems pretty clear that this must be stable-blocking.
Comment #6
Wim Leers#3265230: Refactor ie11.filter.warnings.es6.js to simpler structure & other improvements landed :)
Comment #7
longwaveCan we add it to
drupal.editor
ordrupal.filter
instead?Comment #8
longwaveIt 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.Comment #9
Wim LeersOhhhh, 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, becausewill AFAICT result in this code:
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!Comment #11
longwaveSo 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.
This code in the
text_format
pre render callback ensures thatdrupal.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.
Comment #12
bnjmnmWhen #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.
Comment #13
longwaveThanks 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.
Comment #14
bnjmnmGood 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?
Comment #15
lauriii#3266310: IE11 user warning has ungraceful failures was committed.
Comment #16
Wim LeersYay, now we can finally do this one 🤓
Comment #17
bnjmnmSince 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 ofckeditor5/drupal.ckeditor5
.Quick Edit message again works:
Comment #18
longwaveGreat - 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:
Comment #20
tim.plunkettQuickedit is asserting that ie11.user.warnings.js is loaded :\
Comment #21
longwaveNow the JS is loaded on demand, we can't assert its existence until Quick Edit has been triggered.
Comment #22
tim.plunkettOh, great! Thanks @longwave
Comment #23
Wim LeersWow, epic improvement here! 😄 Hopefully this is the last time IE makes our lives more difficult as web developers… 🤞
Comment #26
lauriiiCommitted 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.
Comment #28
bnjmnmCherry-picked to 9.3.x