Off canvas filling up localStorage's quota, causing errors

#3076641: Can't edit or save content with very long texts

that patch has few problems:

1.
Should be 2 keys

CKEDITOR_TIMESTAMP: pwr2i2
CKEDITOR_CSS_RESET: css

2.
This is even loaded or inserted even the CKEDITOR hasn't used inside the Tray or starting to use Tray

3.
A bit odd to inserted into BODY and bottom.

 document.body.appendChild(offCanvasCss);

4.
Possible hit cross-origin and can't load.
$.when($.get(editorCssPath), $.get(dialogCssPath))

5.
I know @nod_ put a lot of effort to get rid of the shorthand function.
$.when($.get(editorCssPath), $.get(dialogCssPath)).done(

Here is a patch that removes previous items before adding a new one.

Closed #3098917: CKEDITOR error for Drupal 8.8.0 as it duplicates this one.

Patch #4 seems to have solved the issue for me. No longer receiving the console error.

@olli thanks for the patch. 1 suggestion

+++ b/core/modules/ckeditor/js/ckeditor.off-canvas-css-reset.es6.js
@@ -94,6 +94,12 @@
+          for (let i = 0; i < window.localStorage.length; i++) {
+            const key = window.localStorage.key(i);
+            if (key.indexOf('Drupal.off-canvas.css.') === 0) {
+              window.localStorage.removeItem(key);
+            }
+          }

I think it would simpler to use Object.keys()
like

          Object.keys(window.localStorage).forEach(key => {
            if (key.indexOf('Drupal.off-canvas.css.') === 0) {
              window.localStorage.removeItem(key);
            }
          });

#4 works Thanks

Here I have made changes as suggested in comment #9.

#11 fixes the problem for me. Thanks.

+1 on the RTBC, It address my only suggesting in #9. Thanks @ravi.shankar

@interdruper thanks for reporting that it fixed the issue.

In the future though when you set a patch to Reviewed & tested by the community make it clear that you have reviewed the patch and that it addresses any outstanding feedback on the issue.

If you aren't able to review the patch reporting that it fixed the problem is very helpful.. It is just not "Reviewed & tested by the community"

Can we add an assertion to \Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest::testOffCanvasStyles() to prove this is fixed?

Can we add an assertion to \Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest::testOffCanvasStyles() to prove this is fixed?

Thanks @alexpott, I've added a test for that. It could probably be made a bit quicker by using _drupal_flush_css_js() but I thought that might be considered a little dirty (: Also it evals Object.keys() in the browser session, which AFAICT is fine from a compatibility PoV, but I can see that it's transformed in our ES6 files so thought I should highlight it.

IIUC as the patch stands if you cache any styles, than you'll always have some cached styles, even if you've stopped using the site. I was just wondering if it made sense to switch to using session storage? It's a trade-off: as I understand it this would mean multiple parsing and storage of styles if there are multiple tabs open or if you close a tab and revisit the site later while the cache would still be valid; but on the positive it wouldn't need any cache invalidation logic and wouldn't leave a permanent footprint. Just a thought...

Thanks!

Update tags

Tests pass and fail as expected, moving to need review.

@AndyF thanks for the test! Looks mostly good.

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -211,6 +211,19 @@ public function testOffCanvasStyles() {
+    $cached_style_count = $this->getSession()->evaluateScript('Object.keys(window.localStorage).filter(function (i) {return i.indexOf(\'Drupal.off-canvas.css.\') === 0}).length');
+    $this->assertEqual($cached_style_count, 1);

I think we should be even more explicit here.

This just tests for sure that there is only 1 key not that it is not the same as the key before.

Before this whole new code but we should tests the keys but instead of returning .length from the JS we should return the actually keys and do
$this->assertCount(1, $cached_keys);

Then after instead here we could do

$this->assertCount(1, $new_cached_keys);
$this->assertNotEqual($cached_keys, $new_cached_keys);

So we would know before and after there was 1 key and it is different after.

We could store the JS string as a var since we would use the exact same JS 2x

Thanks for the review @tedbow! I've updated it to also test that the cache is actually busted.

@AndyF thanks updating the test.! 1 more suggestion. sorry trying to look at this test like I didn't know how the tested code is written.

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -212,7 +212,10 @@
+    $old_keys = $this->getSession()->evaluateScript($get_cache_keys);

@@ -222,8 +225,10 @@
+    $this->assertCount(1, $new_keys, 'Only one off-canvas style was cached.');
+    $this->assertNotEquals($old_keys, $new_keys, 'Clearing caches changed the off-canvas style cache key.');

So because we never do
$this->assertCount(1, $old_keys);
When we assert assertNotEquals() we don't 100% know that the $new_keys aren't in the $old_keys array.

It could be the $old_keys array had a count of 2 and it contained the 1 element in the $new_keys array.

If we knew that count($old_keys) === 1 we could be sure when we
$this->assertNotEquals($old_keys, $new_keys); They aren't just not equal because $old_keys has some extra element.

I know this couldn't be the case now but putting this 1 extra check in makes sure that if something changes in the JS and this happens this test will stop passing(as it should)

Thanks again @tedbow for the thoughtful review!

putting this 1 extra check in makes sure that if something changes in the JS and this happens this test will stop passing

👍 Understood, updated.

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -211,6 +211,25 @@ public function testOffCanvasStyles() {
   +    $this->assertCount(1, $old_keys, 'Only one off-canvas style was cached before clearing caches.');
   +    $this->assertCount(1, $new_keys, 'Only one off-canvas style was cached after clearing caches.');
   +    $this->assertNotEquals($old_keys, $new_keys, 'Clearing caches changed the off-canvas style cache key.');
   

   This looks great now! Thanks

2. +++ b/core/modules/ckeditor/js/ckeditor.off-canvas-css-reset.js
   @@ -44,10 +44,16 @@
   +        for (var i = 0; i < window.localStorage.length; i++) {
   +          var key = window.localStorage.key(i);
   

   I think this is now out of sync

   I ran
   yarn build:js
   and this filed changed a bunch.
   This should be run and updated.

besides the last point I think this is RTBC

Thanks @tedbow, it looks like #11 only updated the ES6 (which led me to incorrectly say in #15 that Object.keys() gets transformed, turns out it doesn't). I've attached the regenerated JS, cheers!

@AndyF thanks!
I ran
yarn build:js
locally and now no changes were made!

I think that's the random test failure from #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest): moving back to RTBC, thanks.

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -211,6 +211,25 @@ public function testOffCanvasStyles() {
+    $old_keys = $this->getSession()->evaluateScript($get_cache_keys);
+    drupal_flush_all_caches();
+    // Normally flushing caches regenerates the cache busting query string, but
+    // as it's based on the request time, it won't change within this test so
+    // explicitly set it.
+    \Drupal::state()->set('system.css_js_query_string', '0');

If we have to reset the css/js query string anyway, do we need to flush all caches? And if we do, could we add a comment explaining why both are necessary?

Thanks @catch. As I understand it we just need ckeditor_library_info_alter() to run again to update the timestamp in drupalSettings (the test fails without the cache clear). Here's an updated patch, cheers!

This now needs a 9.0.x version of the patch.

Thanks, here's #30 rolled against 9.0.x.

Tests pass - as it's a reroll I think it's ok to put back to RTBC, thanks!

Updating credit.

Committed 8d5200a and pushed to 9.1.x, cherry-picked to 9.0.x, 8.9.x and 8.8.x. Thanks!