Problem/Motivation

This is a very large, already minified file, that gets aggregated with different sets of files on different pages (e.g. if different node types have different ckeditor configurations or other CSS/JS added to the form.

Excluding it from aggregations should reduce bytes transferred in performance tests by quite a bit.

Spinning this out from #3615654: Add aggregate_target to core and ckeditor5 libraries to do on its own so it's easier to see the impact and because it should be backportable to a patch release if we want to.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3615749-nr-bot_3pxq6o8i.txt98 bytesneeds-review-queue-bot

Issue fork drupal-3615749

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

Yep nearly half the bytes for stylesheets, because this is a 218kb file. So if it gets served in three different aggregates on three different pages, that's 436kb of unnecessary file transfer.

220K -rw-r--r-- 1 catch catch 218K Aug  9 16:51 core/assets/vendor/ckeditor5/ckeditor5.css

catch opened merge request !16632

smustgrave’s picture

smustgrave commented

Not sure but could this be related?

    CKEditor5Allowed Tags (Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTags)
     ✔ Enabling to version 5 validation
     ✔ Image uploads remain enabled
     ✔ Switch to version 5
     ✔ Img added via upload plugin
     ✔ Allowed tags
     ✔ Media element allowed tags
     ✘ Full html
       ┐
       ├ /subdirectory/node/1/edit|Error: cannot call methods on dialog prior to initialization; attempted to call method 'option'
       ├     at T.error (http://localhost/subdirectory/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:2495)
       ├     at HTMLDivElement. (http://localhost/subdirectory/core/assets/vendor/jquery.ui/ui/widget-min.js?v=12.0-dev:9:2302)
       ├     at T.each (http://localhost/subdirectory/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:2869)
       ├     at T.fn.init.each (http://localhost/subdirectory/core/assets/vendor/jquery/jquery.min.js?v=4.0.0:2:1379)
       ├     at t.fn. [as dialog] (http://localhost/subdirectory/core/assets/vendor/jquery.ui/ui/widget-min.js?v=12.0-dev:9:2041)
       ├     at HTMLDocument.resetSize (http://localhost/subdirectory/core/misc/dialog/dialog.position.js?v=12.0-dev:112:25)
       ├     at later (http://localhost/subdirectory/core/misc/debounce.js?v=12.0-dev:37:23)
catch’s picture

catch
he/him
Primary language English
commented

@smustgrave it's not clear. There's pretty much no way this can be introducing a bug in ckeditor, but it probably is affecting page render timing, which could cause what was previously a random failure to be a persistent random failure. I reran that job a couple of times and the test failed pretty consistently, but I think it's something to be fixed in that test rather than this issue as such.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3615749-nr-bot_3pxq6o8i.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Know for a fact 

       ├ Behat\Mink\Exception\ExpectationException: The string "This is a test!" was not found anywhere in the HTML response of the current page.
       │
       │ /builds/vendor/behat/mink/src/WebAssert.php:888
       │ /builds/vendor/behat/mink/src/WebAssert.php:363
       │ /builds/core/tests/Drupal/Tests/WebAssert.php:569
       │ /builds/core/modules/ckeditor5/tests/src/FunctionalJavascript/EmphasisTest.php:142

Is random as it pops up on almost every MR.

  • godotislate committed 4e897a51 on main 
    task: #3615749 Exclude ckeditor5.css from aggregation

By: catch
By:...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 4e897a5 and pushed to main. Thanks!

11.x has conflicts in the test numbers as well as the libraries file, since it doesn't use the UMD.

catch opened merge request !16722

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » main
Status: Patch (to be ported) » Fixed

Started work on a backport then realised the dll ckeditor5 doesn't ship with a ckeditor5.css at all (or any other single CSS file that we include with a library). Moving back to fixed against main.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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