Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Change update interval on CKEditor #3751

Conversation

Alvadda
Copy link
Collaborator

@Alvadda Alvadda commented Mar 14, 2024

Currently, the CKEditor send the onChange event after user stops typing for 500ms, this results in a lot of unnecessary events that bloats the server.

This adjustment was initially made in response to a new server behavior observed in Neos9, where the server temporarily blocks incoming change requests during publishing processes. Extending the debounce interval aims to mitigate the risk of overlap between change requests and publishing operations.

What I did
The debounce timer has been extended from 500ms to 1500ms. This reduces the frequency of onChange events, thereby decreasing server load.

An additional onChange event has been introduced to trigger whenever the editor loses focus. This ensures that all changes are captured, even if the user does not pause in their typing.

How to verify it
Engage with the CKEditor by typing. Upon ceasing typing, observe that the publish button (located in the top right corner) indicates a change was made, after a delay of 1500ms.

While typing in CKEditor, shift focus to another element before the 1500ms debounce period concludes. The publish button should still update to reflect a change, ensuring that all modifications are captured accurately.

@github-actions github-actions bot added Bug Label to mark the change as bugfix 8.3 labels Mar 14, 2024
Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Alvadda,

thanks for taking care of this!

I've pointed out one leftover console.log in the comments.

Additionally, could you please take a look at this test:

subSection('Inline validation');
// We have to wait for ajax requests to be triggered, since they are debounced for 0.5s
await t.wait(600);
await changeRequestLogger.clear();
await t
.expect(Selector('.test-headline h1').exists).ok('Validation tooltip appeared')
.click('.test-headline h1')
.pressKey('ctrl+a delete')
.switchToMainWindow()
.wait(600)
.expect(ReactSelector('InlineValidationTooltips').exists).ok('Validation tooltip appeared');
await t
.expect(changeRequestLogger.count(() => true)).eql(0, 'No requests were fired with invalid state');
await t
.switchToIframe(contentIframeSelector)
.typeText(Selector('.test-headline h1'), 'Some text')
.wait(600);
await t.expect(changeRequestLogger.count(() => true)).eql(1, 'Request fired when field became valid');

If you change the wait times that are currently set at 600 to 1600, the E2E tests will turn green :)

packages/neos-ui-ckeditor5-bindings/src/ckEditorApi.js Outdated Show resolved Hide resolved
@grebaldi
Copy link
Contributor

grebaldi commented Jul 8, 2024

I've altered the wait times in the E2E tests, and they're green now 🎉

I'm gonna merge this :)

@grebaldi grebaldi merged commit edd3098 into neos:8.3 Jul 8, 2024
9 checks passed
@mhsdesign
Copy link
Member

I fear this change introduced a regression which was not found during testing nor is covered by e2e.
I noticed that simply clicking into a ckeditor text field and afterwards selecting something else triggered a change.
This happens for every text element:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.3 Bug Label to mark the change as bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce number of NodePropertiesWereSet events during inline editing
4 participants