Robust changed content detection #2476

PeterNerlich · 2023-10-23T12:46:26Z

Short description

Rework changed content detection from using a dirty flag to comparing with the previous content and add a little indicator. With this we can reliably tell whether we need to autosave at all or not.

Proposed changes

Remove dirty flag and determine presence of changes by comparing with previously captured state
Only attatch beforeunload listener when dirty to allow the browsers back/forward cache (per recommendation: https://developer.chrome.com/articles/page-lifecycle-api/#the-beforeunload-event)
Prepend bullet to document title when dirty

Side effects

When closing the tab with unsaved changes, an autosave is triggered. However, the popup asking whether to really close the page is shown before the server can respond.
This leads to situations where the content is saved despite a warning being shown. This cannot be changed, since it's impossible to predict whether it will be successful or not. See also Show unsaved warning only when content model is changed #2132 and Show only unsaved warning when changes are not saved #2435.
There is a potential for the value representing the last saved state to be out of sync with what was actually sent as an auto save. This is because of the separation of the component tracking changes for the whole form and the autosave plugin to tinymce which performs the actual autosave: the former only receives the signal that an autosave is being attempted and then that it was indeed successful, but never the actual data that was sent. It might theoretically be possible for the content to change in the tiny instance between the autosave gathering the data to send to the server and the unsaved warning module receiving the signal and itself gathering the data as a candidate for the next saved state.
It might be possible to mitigate this by moving the autosave itself out of the editor plugin (then at most sending a signal to trigger it), since it requires the whole form data, which is technically out of the scope of a tinymce plugin (also more than one editor could be part of the same form, which would create problems, though we do this nowhere).
However, this is hopefully very unlikely and should not do any damage if it should occur: The state would likely only be different by a single character until the next autosave or reload, and a wrong save decision because of it would not pose a problem be caught and corrected reasonably soon.
The change indicator in the title relies on the title not being changed (by JS) over the life cycle of a page in the browser. This is because to reliably add the bullet, the original title is saved in a variable. Currently, we don't even give a different title to different CMS pages.

Resolved issues

Fixes #2477

Pull Request Review Guidelines

codeclimate · 2023-10-23T12:46:45Z

Code Climate has analyzed commit f6ee973 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 78.9% (0.0% change).

View more on Code Climate.

PeterNerlich · 2023-10-26T12:22:28Z

On second thought, there is no reason for this to require merging after #2401

seluianova · 2023-11-07T18:07:42Z

On second thought, there is no reason for this to require merging after #2401

@PeterNerlich so, is it still actual?

PeterNerlich · 2023-11-07T18:19:59Z

@PeterNerlich so, is it still actual?

Yes, feel free to review the PR and/or give an opinion on the feature with the bullet in the document title

seluianova

✳️ I'm observing some strange side effects on this branch.
For example:

Open Willkommen page in the editor
Add a point (.) at the end of the sentence
Publish --> success
Remove a point at the end of the sentence
Publish --> the point is returned back, "No changes detected, but date refreshed" message is shown.

I couldn't reproduce it on develop branch.

✳️ In addition, I have the following doubt:
I have a feeling that I, as a user, probably wouldn't notice the bullet point in the title...
(But I'm open for other opinions here)

✳️ Also, in some cases the indicator doesn't appear when changes are made, but so far I haven't been able to determine the exact steps to reproduce this (it seems pretty random?).

david-venhoff

Thanks for the pr! Unfortunately it seems to cause the editor to behave weirdly sometimes and I am not really sure why.

Currently we emit an input event for the form when the editor becomes dirty for the unsaved warning to update. This can probably be removed now:

integreat-cms/integreat_cms/static/src/js/forms/tinymce-init.ts

Lines 222 to 229 in dbd5c7d

    
           editor.on("dirty", () => { 
        
               if ((editor.targetElm as HTMLElement).hasAttribute("data-unsaved-warning-exclude")) { 
        
                   return; 
        
               } 
        
               document.querySelectorAll("[data-unsaved-warning]").forEach((element) => { 
        
                   element.dispatchEvent(new Event("input")); 
        
               }); 
        
           });

Also the contentChanged event that gets emitted on keyUp for the hix widget should probably be migrated to use your system:

integreat-cms/integreat_cms/static/src/js/forms/tinymce-init.ts

Lines 231 to 234 in dbd5c7d

    
           editor.on("keyup", () => 
        
               document.querySelectorAll("[data-content-changed]").forEach((element) => { 
        
                   element.dispatchEvent(new Event("contentChanged")); 
        
               })

integreat_cms/static/src/js/unsaved-warning.ts

integreat_cms/static/src/js/forms/tinymce-init.ts

david-venhoff · 2023-11-13T14:23:44Z

integreat_cms/static/src/js/unsaved-warning.ts

+    // Are there any unsaved changes in the editors?
+    for (const editor of editors) {
+        if (isActuallyDirty(editor)) {
+            return true;
        }
-        if (!dirty) {
-            console.debug("editing detected, enabled beforeunload warning");
+    }
+    return false;


I could reproduce the same problem that @seluianova mentioned, where the modified input would not get submitted sometimes, and could not figure out how exactly it is caused. However, commenting out these lines fixes the problem for me somehow so it might be related to those 🤷

Please see whether you can still reproduce it or whether my attempt to fix it was successful

PeterNerlich · 2023-11-20T22:21:14Z

Thank you for bringing up that issue! I totally missed that this happens.
It was annoying to figure out how to fix, but this is how I think it occurred: TinyMCE hides the original text field it's supposed to represent and builds its own interface, requiring it to save its state back to the form element before the form data gets captured and sent in order to be up to date. This is taken care of for the autosave functionality, but this didn't happen for the regular form submit.

So in order to ensure this also for the normal submit, I added a formdata listener that modifies the formdata gathered on submit (and also when calling new FormData(form)) to contain the latest tinymce content.

remove dirty flag and determine presence of changes by comparing with previously captured state only attatch beforeunload listener when dirty to allow the browsers back/forward cache prepend bullet to document title when dirty Co-authored-by: David Venhoff <[email protected]>

seluianova · 2023-11-25T13:58:06Z

I can still reproduce the problem where changes made are lost after clicking the "Update" button.
However, this appears to be a flaky problem and I don't have the exact steps to reproduce it.

PeterNerlich requested a review from a team as a code owner October 23, 2023 12:46

PeterNerlich mentioned this pull request Oct 23, 2023

Fix "HIX score outdated" #2401

Merged

PeterNerlich force-pushed the feature/robust_changed_content_detection branch 3 times, most recently from 0deb30b to 10e74cf Compare October 23, 2023 14:53

PeterNerlich mentioned this pull request Oct 23, 2023

Unsaved changes indicator #2477

Open

PeterNerlich force-pushed the feature/robust_changed_content_detection branch from 10e74cf to 5481d5e Compare October 26, 2023 12:20

seluianova requested changes Nov 13, 2023

View reviewed changes

david-venhoff requested changes Nov 13, 2023

View reviewed changes

PeterNerlich force-pushed the feature/robust_changed_content_detection branch from 5481d5e to be235c4 Compare November 20, 2023 22:20

PeterNerlich force-pushed the feature/robust_changed_content_detection branch 2 times, most recently from 766c65c to 558dbd4 Compare November 20, 2023 22:29

PeterNerlich force-pushed the feature/robust_changed_content_detection branch from 558dbd4 to f6ee973 Compare November 20, 2023 22:35

PeterNerlich marked this pull request as draft June 9, 2024 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Robust changed content detection #2476

Robust changed content detection #2476

PeterNerlich commented Oct 23, 2023 •

edited

Loading

codeclimate bot commented Oct 23, 2023 •

edited

Loading

PeterNerlich commented Oct 26, 2023

seluianova commented Nov 7, 2023

PeterNerlich commented Nov 7, 2023

seluianova left a comment •

edited

Loading

david-venhoff left a comment

david-venhoff Nov 13, 2023 •

edited

Loading

PeterNerlich Nov 20, 2023

PeterNerlich commented Nov 20, 2023

seluianova commented Nov 25, 2023 •

edited

Loading

	editor.on("dirty", () => {
	if ((editor.targetElm as HTMLElement).hasAttribute("data-unsaved-warning-exclude")) {
	return;
	}
	document.querySelectorAll("[data-unsaved-warning]").forEach((element) => {
	element.dispatchEvent(new Event("input"));
	});
	});

	editor.on("keyup", () =>
	document.querySelectorAll("[data-content-changed]").forEach((element) => {
	element.dispatchEvent(new Event("contentChanged"));
	})

Robust changed content detection #2476

Are you sure you want to change the base?

Robust changed content detection #2476

Conversation

PeterNerlich commented Oct 23, 2023 • edited Loading

Short description

Proposed changes

Side effects

Resolved issues

codeclimate bot commented Oct 23, 2023 • edited Loading

PeterNerlich commented Oct 26, 2023

seluianova commented Nov 7, 2023

PeterNerlich commented Nov 7, 2023

seluianova left a comment • edited Loading

Choose a reason for hiding this comment

david-venhoff left a comment

Choose a reason for hiding this comment

david-venhoff Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

PeterNerlich Nov 20, 2023

Choose a reason for hiding this comment

PeterNerlich commented Nov 20, 2023

seluianova commented Nov 25, 2023 • edited Loading

PeterNerlich commented Oct 23, 2023 •

edited

Loading

codeclimate bot commented Oct 23, 2023 •

edited

Loading

seluianova left a comment •

edited

Loading

david-venhoff Nov 13, 2023 •

edited

Loading

seluianova commented Nov 25, 2023 •

edited

Loading