The Drupal 7 version of the Edit module already implements this. We can't do a 1:1 port though, because of D8 Edit having a vastly different architecture thanks to Plugins.

See http://drupalcode.org/project/edit.git/commitdiff/785db79121d3d550795d7a... for the D7 diff.

CommentFileSizeAuthor
#7 edit_wpo_lazy_load_editors-1943776-7.patch19.96 KBwim leers
#7 interdiff.txt9.16 KBwim leers
#3 edit_wpo_lazy_load_editors-1943776-3.patch11.57 KBwim leers
#3 interdiff.txt745 byteswim leers
#1 edit_wpo_lazy_load_editors-1943776-1.patch11.56 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Create.js PropertyEditor widgets should be loaded lazily » In-place editors (Create.js PropertyEditor widgets) should be loaded lazily
Assigned: Unassigned » nod_
Status: Postponed » Needs review
Issue tags: +WPO, +sprint
StatusFileSize
new edit_wpo_lazy_load_editors-1943776-1.patch11.56 KB

Lazy loading of in-place editors (Create.js PropertyEditor widgets)!

Details:

  • removed setting the "defer" attribute, since that currently breaks aggregation as per #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI). Having the "defer" attribute is better, but not if that means there can't be any JS aggregation.
  • EditorSelectorInterface modified
  • new EditMetadata command, the edit/metadata route now no longer returns a JsonResponse but an AjaxResponse, because it is impossible to have the JS itself trigger the loading of libraries. Hence I had to once again use the filthy Drupal.ajax hack, and this time I even have to create a temporary element, because Edit module itself does not have any elements in the DOM. This again stresses the importance of #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation.
  • removes Editor module's Create.js widget's dependency on Edit module's "direct" (plain contentEditable) widget (less JS loaded when the "direct" widget is not used on a page)

Consequences on an out-of-the-box Drupal 8 Standard installation with 1 node: the frontpage won't ever cause CKEditor to be loaded, on the node/1 page it will only be loaded after the DOMContentLoaded event. Less bytes, faster time to first paint & interaction.

Assigning to nod_ for review, whom rightfully complained about the current state of Drupal 8 front-end performance, to which this brings a very big positive impact.

nod_’s picture

nod_
Primary language French
Location Lille
commented

in EditorSelector.php

  public function getEditorAttachments(array $editor_ids) {
    $attachments = array();
    $editors = array_unique($editors);

    // Editor plugins' attachments.
    foreach ($editor_ids as $editor_id) {
      $editor = $this->editorManager->createInstance($editor_id);
      $attachments[] = $editor->getAttachments();
    }

$editors is not defined. In the D7 use case, there were lots of duplicates in $editors_ids, making the array_unique necessary. Haven't checked here, I'm pretty sure we'll end up needing it at some point.

Beside that, all good.

Great, great thing to have here. Was biggest front-end perf issue of D8 (next is a one-liner patch for the toolbar)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt745 bytes
new edit_wpo_lazy_load_editors-1943776-3.patch11.57 KB

Hah, stupid :)

nod_’s picture

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

WimLeers++

Work as intended. bumping priority since it removes something like 100ms on desktop to all pages for load time.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: nod_ » wim leers

Thanks for the review, nod_! :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Can we not add a test to ensure that ckeditor is not in the initial page load?

+++ b/core/modules/edit/lib/Drupal/edit/EditorSelector.phpundefined
@@ -90,24 +90,20 @@ public function getEditor($formatter_type, FieldInstance $instance, array $items
+   * Implements \Drupal\edit\EditorSelectorInterface::getEditorAttachments().

@inheritdoc

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new interdiff.txt9.16 KB
new edit_wpo_lazy_load_editors-1943776-7.patch19.96 KB

Addressed both. You're right about those tests; I should've added those a long time ago. I went a little bit farther though: I also make sure that the lazy-loading itself actually works :)

nod_’s picture

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

yay tests!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Performance, +Needs followup

Awesome! The expanded test coverage looks excellent!

I didn't have any complaints at all, but one thing effulgentsia noted was this todo:

+   * @todo Convert back to JsonResponse once we have the ability to load
+   *   libraries from the client side.

...was potentially confusing. The code is actually doing the right thing by returning AjaxResponse and as far as both of us, as well as nod_ could tell, there was no issue for what this is talking about.

nod_ provided some more information via IRC:

nod_: webchick: it's a pretty far-fetched todo. The patch makes edit/metadata return ajax commands so that we can load js files and stuff. Wim talked about being able to return library info and have an API in drupal to load all the files associated with it
nod_: webchick: but this API doesn't exist and there is no issue to talk about it as far as I know
so from {libraries: [ [ 'ckeditor', 'ckeditor'], ['system', 'createjs'] ] } have something that can load all the files declared in the hook_library_info() about them, without sending the file list in that response (and having the library to files mapping somewhere in drupalSettings)
webchick: nod_: Ok, so science fiction then. :D
nod_: webchick: for now, pretty much :)

...so I think let's get a follow-up filed for that, but for now I've removed this @todo so it doesn't confuse anyone.

Committed and pushed to 8.x. Thanks! :D Performance++

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

Yes, converting back to JsonResponse is a nice-to-have. AJAX commands are just such a majestical PITA to use when the code is driven by the client-side (which is the case for in-place editing) rather than the server-side. This is the travesty that we have to endure currently:

+        var id = 'edit-load-metadata';
+        // Create a temporary element to be able to use Drupal.ajax.
+        var $el = jQuery('<div id="' + id + '" class="element-hidden"></div>').appendTo('body');
+        // Create a Drupal.ajax instance to load the metadata.
+        Drupal.ajax[id] = new Drupal.ajax(id, $el, {
           url: drupalSettings.edit.metadataURL,
+          event: 'edit-internal.edit',
+          submit: {}, // The POST value.
+          progress: { type : null } // No progress indicator.
         });
+        // Implement a scoped editMetaData AJAX command: calls the callback.
+        Drupal.ajax[id].commands.editMetadata = function(ajax, response, status) {
+            //  do something with the response
+        };
+        // This will ensure our scoped editMetadata AJAX command gets called.
+        $el.trigger('edit-internal.edit');

Follow-up created: #1980744: Turn edit/metadata into a JsonResponse again, to allow contrib to implement client-side caching of metadata.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs followup

.

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