From 59b304dfcab07c8b2b6de420a698a5b640a52ea3 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Thu, 13 Jun 2024 11:07:56 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20editor=20'are=20you=20su?= =?UTF-8?q?re=3F'=20modal=20displaying=20when=20no=20user=20changes=20occu?= =?UTF-8?q?rred=20(#20370)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/tryghost/issue/ENG-661 - added a dirty check to ignore the `direction` field from the lexical object; this is set dynamically and shouldn't be serialized, see facebook/lexical/issues/4998 - fixed a bug where the modal wouldn't display on leaving the editor if the post had no revisions (e.g. import); this could result in content being saved over published content with no user action - added Sentry logging for the modal We would sometimes see the "Are you sure?" modal pop up when opening a post in the editor and attempting to navigate away immediately, without any changes to the post. This appears to be an issue with the serialized Lexical data, which would change after loading into the editor, resulting in the scratch and model's lexical values to differ, making Admin think the user changed the content. Ideally we'll see a fix upstream (or fix it ourselves). We may need to revisit this if we experience other such situations. It's awfully difficult to be able to set a flag saying 'the editor is done loading', so this seems to be the best path for the moment. Testing is difficult because we don't actually load the new Lexical editor into e2e/acceptance tests. I've added a unit test that can at least simulate the editor state changing on editor load. --- .../modals/editor/confirm-leave.hbs | 6 +- ghost/admin/app/controllers/lexical-editor.js | 17 ++++- .../acceptance/editor/publish-flow-test.js | 73 +++++++++++++++++++ .../tests/unit/controllers/editor-test.js | 33 +++++++++ 4 files changed, 125 insertions(+), 4 deletions(-) diff --git a/ghost/admin/app/components/modals/editor/confirm-leave.hbs b/ghost/admin/app/components/modals/editor/confirm-leave.hbs index b41d909857..dc088fb052 100644 --- a/ghost/admin/app/components/modals/editor/confirm-leave.hbs +++ b/ghost/admin/app/components/modals/editor/confirm-leave.hbs @@ -1,4 +1,4 @@ -
+

Are you sure you want to leave this page?

@@ -14,8 +14,8 @@
- - + +
\ No newline at end of file diff --git a/ghost/admin/app/controllers/lexical-editor.js b/ghost/admin/app/controllers/lexical-editor.js index ea4991a106..c42b6cd975 100644 --- a/ghost/admin/app/controllers/lexical-editor.js +++ b/ghost/admin/app/controllers/lexical-editor.js @@ -1084,7 +1084,8 @@ export default class LexicalEditorController extends Controller { && (state.isSaving || !state.hasDirtyAttributes); // If leaving the editor and the post has changed since we last saved a revision (and it's not deleted), always save a new revision - if (!this._saveOnLeavePerformed && hasChangedSinceLastRevision && hasDirtyAttributes && !state.isDeleted) { + // but we should never autosave when leaving published or soon-to-be published content (scheduled); this should require the user to intervene + if (!this._saveOnLeavePerformed && hasChangedSinceLastRevision && hasDirtyAttributes && !state.isDeleted && post.get('status') === 'draft') { transition.abort(); if (this._autosaveRunning) { this.cancelAutosave(); @@ -1123,6 +1124,7 @@ export default class LexicalEditorController extends Controller { if (this.post) { Object.assign(this._leaveModalReason, {status: this.post.status}); } + Sentry.captureMessage('showing leave editor modal', {extra: this._leaveModalReason}); console.log('showing leave editor modal', this._leaveModalReason); // eslint-disable-line const reallyLeave = await this.modals.open(ConfirmEditorLeaveModal); @@ -1248,6 +1250,19 @@ export default class LexicalEditorController extends Controller { // additional guard in case we are trying to compare null with undefined if (scratch || lexical) { if (scratch !== lexical) { + // lexical can dynamically set direction on loading editor state (e.g. "rtl"/"ltr") per the DOM context + // and we need to ignore this as a change from the user; see https://github.com/facebook/lexical/issues/4998 + const scratchChildNodes = scratch ? JSON.parse(scratch).root?.children : []; + const lexicalChildNodes = lexical ? JSON.parse(lexical).root?.children : []; + + // // nullling is typically faster than delete + scratchChildNodes.forEach(child => child.direction = null); + lexicalChildNodes.forEach(child => child.direction = null); + + if (JSON.stringify(scratchChildNodes) === JSON.stringify(lexicalChildNodes)) { + return false; + } + this._leaveModalReason = {reason: 'lexical is different', context: {current: lexical, scratch}}; return true; } diff --git a/ghost/admin/tests/acceptance/editor/publish-flow-test.js b/ghost/admin/tests/acceptance/editor/publish-flow-test.js index 5cbb3c95f6..38bc4ee02a 100644 --- a/ghost/admin/tests/acceptance/editor/publish-flow-test.js +++ b/ghost/admin/tests/acceptance/editor/publish-flow-test.js @@ -624,4 +624,77 @@ describe('Acceptance: Publish flow', function () { it('handles server error when confirming'); it('handles email sending error'); }); + + describe('Are you sure you want to leave? modal', function () { + // draft content should autosave and leave without warning + it(`Doesn't display for draft content`, async function () { + await loginAsRole('Administrator', this.server); + const post = this.server.create('post', { + title: 'Test Post', + status: 'draft' + }); + await visit('/editor/post/' + post.id); + await fillIn('[data-test-editor-title-input]', 'New Title'); + await click('[data-test-link="posts"]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.not.exist; + }); + // published content should never autosave and should warn on leaving when there's changes + it('Displays when published content title has changed', async function () { + await loginAsRole('Administrator', this.server); + const post = this.server.create('post', { + title: 'Test Post', + status: 'published' + }); + await visit('/editor/post/' + post.id); + await fillIn('[data-test-editor-title-input]', 'New Title'); + await click('[data-test-link="posts"]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.exist; + }); + it('Displays when scheduled content has changed', async function () { + await loginAsRole('Administrator', this.server); + const post = this.server.create('post', { + title: 'Test Post', + status: 'scheduled' + }); + await visit('/editor/post/' + post.id); + await fillIn('[data-test-editor-title-input]', 'New Title'); + await click('[data-test-link="posts"]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.exist; + }); + // published and edited content should not warn when changes are reverted (either via undo or manually) + it(`Does not display when changed content is changed back`, async function () { + await loginAsRole('Administrator', this.server); + const post = this.server.create('post', { + title: 'Test Post', + status: 'published', + lexical: `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}` + }); + await visit('/editor/post/' + post.id); + await fillIn('[data-test-editor-title-input]', 'New Title'); + await click('[data-test-link="posts"]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.exist; + await click('[data-test-stay-button]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.not.exist; + // revert title + await fillIn('[data-test-editor-title-input]', 'Test Post'); + await click('[data-test-link="posts"]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.not.exist; + }); + it(`Does not save changes when leaving`, async function () { + await loginAsRole('Administrator', this.server); + const post = this.server.create('post', { + title: 'Test Post', + status: 'published', + lexical: `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}` + }); + await visit('/editor/post/' + post.id); + await fillIn('[data-test-editor-title-input]', 'New Title'); + await click('[data-test-link="posts"]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.exist; + await click('[data-test-leave-button]'); + expect(find('[data-test-modal="unsaved-post-changes"]'), 'unsaved changes modal').to.not.exist; + // check that the title wasn't saved + expect(this.server.db.posts.find(post.id).title === 'Test Post').to.be.true; + }); + }); }); diff --git a/ghost/admin/tests/unit/controllers/editor-test.js b/ghost/admin/tests/unit/controllers/editor-test.js index ce086550f4..c430fff1ab 100644 --- a/ghost/admin/tests/unit/controllers/editor-test.js +++ b/ghost/admin/tests/unit/controllers/editor-test.js @@ -180,4 +180,37 @@ describe('Unit: Controller: lexical-editor', function () { expect(controller.get('tkCount')).to.equal(1); }); }); + + describe('hasDirtyAttributes', function () { + it('Changes in the direction field in the lexical string are not considered dirty', async function () { + let controller = this.owner.lookup('controller:lexical-editor'); + + const initialLexicalString = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content","type": "extended-text","version": 1}],"direction": null,"format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; + const lexicalStringNoNullDirection = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Sample content","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; + const lexicalStringUpdatedContent = `{"root":{"children":[{"children": [{"detail": 0,"format": 0,"mode": "normal","style": "","text": "Here's some new text","type": "extended-text","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "paragraph","version": 1}],"direction": "ltr","format": "","indent": 0,"type": "root","version": 1}}`; + + // we can't seem to call setPost directly, so we have to set the post manually + controller.set('post', EmberObject.create({ + title: 'this is a title', + titleScratch: 'this is a title', + status: 'published', + lexical: initialLexicalString, + lexicalScratch: initialLexicalString + })); + + // synthetically update the lexicalScratch as if the editor itself made the modifications on loading the initial editorState + controller.send('updateScratch',JSON.parse(lexicalStringNoNullDirection)); + + // this should NOT result in the post being dirty - while lexical !== lexicalScratch, we ignore the direction field + let isDirty = controller.get('hasDirtyAttributes'); + expect(isDirty).to.be.false; + + // now we try a synthetic change in the actual text content that should result in a dirty post + controller.send('updateScratch',JSON.parse(lexicalStringUpdatedContent)); + + // this should NOT result in the post being dirty - while lexical !== lexicalScratch, we ignore the direction field + isDirty = controller.get('hasDirtyAttributes'); + expect(isDirty).to.be.true; + }); + }); });