TryGhost/Ghost
1
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2024-12-25 03:44:29 +03:00
Code Issues Projects Releases Wiki Activity

🐛 Fixed editor 'are you sure?' modal displaying when no user changes occurred (#20370)

Browse Source 
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.
This commit is contained in:
Steve Larson 2024-06-13 11:07:56 -05:00 committed by GitHub
parent 555a2a4e8d
commit 59b304dfca
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 125 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
ghost/admin/app/components/modals/editor/confirm-leave.hbs
View File

@ -1,4 +1,4 @@
<div class="modal-content">
<div class="modal-content" data-test-modal="unsaved-post-changes">
<header class="modal-header">
<h1>Are you sure you want to leave this page?</h1>
</header>
@ -14,8 +14,8 @@
</div>
<div class="modal-footer">
<button class="gh-btn" type="button" {{on "click" @close}}><span>Stay</span></button>
<button class="gh-btn gh-btn-red" type="button" {{on "click" (fn @close true)}}><span>Leave</span></button>
<button class="gh-btn" data-test-stay-button type="button" {{on "click" @close}}><span>Stay</span></button>
<button class="gh-btn gh-btn-red" data-test-leave-button type="button" {{on "click" (fn @close true)}}><span>Leave</span></button>
</div>
</div>

17
ghost/admin/app/controllers/lexical-editor.js
View File

@ -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;
}

73
ghost/admin/tests/acceptance/editor/publish-flow-test.js
View File

@ -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;
});
});
});

33
ghost/admin/tests/unit/controllers/editor-test.js
View File

@ -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;
});
});
});