Code block dialog should not close on Escape if there are unsaved changes
Open, Needs TriagePublicBUG REPORT
Actions

Assigned To

Authored By

	• Catrope
	Apr 11 2023, 7:34 PM

Description

Steps to replicate the issue (include links if applicable):

Edit a page in VisualEditor
Insert a new code block (Insert -> Code block or type <syntax)
The Code block dialog opens
Type some code into the dialog
Accidentally press the Escape key

What happens?: The dialog closes. The code I typed is lost.

What should have happened instead?: The dialog should warn that I'm about to lose what I typed.

Software version (skip for WMF-hosted wikis like Wikipedia): WMF production

Other information (browser name/version, screenshots, etc.):

Details

Subject	Repo	Branch	Lines +/-
Close dialog without confirmation if the user hasn't interacted	mediawiki/extensions/Score	master	+8 -0
Close dialog without confirmation if the user hasn't interacted	mediawiki/extensions/Kartographer	master	+12 -1
Change confirmation behaviour when abandoning template edits	mediawiki/extensions/VisualEditor	master	+11 -3
Rename isModified to isSaveable and deprecate the old name.	mediawiki/extensions/VisualEditor	master	+10 -4
Confirm abort if saving would make a change to the underlying document	mediawiki/extensions/VisualEditor	master	+82 -1
Confirms before closing code block dialog if the user has pending changes	mediawiki/extensions/SyntaxHighlight_GeSHi	master	+61 -2
Removed the unused 'whitespace' field	mediawiki/extensions/VisualEditor	master	+0 -3

Customize query in gerrit

Related Objects

Mentioned In: T374027: Kartographer doesn't close when you press "escape" after interacting with the map

Event Timeline

• Catrope created this task.Apr 11 2023, 7:34 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 11 2023, 7:34 PM

It makes sense that it's happening, insofar as pressing the X on the dialog does the same and escape is just a shortcut to that. It's also a pretty general behavior -- I think every dialog behaves that way in VE, not just the code block one?

...that said, I'd agree that a confirmation step for discarding changes might be a sensible general addition to these things.

matmarex moved this task from To Triage to Triaged on the VisualEditor board.Apr 20 2023, 2:23 PM

There's one dialog with a confirmation step, the template dialog (added by WMDE as a part of their template work). It might make sense to add one to this dialog as well (but not necessarily every dialog, as many of them are quite simple). In particular, because in the code dialog, Escape has other functions (it cancels the code editor autocompletion), so it's easy to press by accident.

zoe subscribed.Apr 10 2024, 9:51 AM

Change #1020294 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/SyntaxHighlight_GeSHi@master] Confirms before closing code block dialog if the user has pending changes

https://gerrit.wikimedia.org/r/1020294

gerritbot added a project: Patch-For-Review.Apr 16 2024, 5:20 PM

Test wiki created on Patch demo by ESanders (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/3fbaeb58ab/wiki/Syntax

zoe claimed this task.Apr 18 2024, 9:32 AM

I have a patch ready for this specific plugin. However, it made sense to move this change over to the VE codebase such that any plugin that creates a dialog can benefit from the check for unsaved changes. I'll be submitting that shortly for feedback.

Change #1025369 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/VisualEditor@master] Confirm abort if saving would make a change to the underlying document

https://gerrit.wikimedia.org/r/1025369

Change #1025384 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/VisualEditor@master] Removed the unused 'whitespace' field

https://gerrit.wikimedia.org/r/1025384

Change #1025390 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/VisualEditor@master] Rename isModified to isSaveable and deprecate the old name.

https://gerrit.wikimedia.org/r/1025390

Change #1025384 abandoned by Zoe:

[mediawiki/extensions/VisualEditor@master] Removed the unused 'whitespace' field

Reason:

Conflicts with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/620362 which renders this unnecessary

https://gerrit.wikimedia.org/r/1025384

Change #1020294 abandoned by Zoe:

[mediawiki/extensions/SyntaxHighlight_GeSHi@master] Confirms before closing code block dialog if the user has pending changes

Reason:

Moved the change into VE as it's generic https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/1025369

https://gerrit.wikimedia.org/r/1020294

Esanders added a project: Editing-team (Kanban Board).Apr 30 2024, 3:59 PM

Esanders moved this task from Incoming to QA on the Editing-team (Kanban Board) board.

Esanders added a project: Editing QA.

Esanders moved this task from Inbox to Low Priority on the Editing QA board.

Change #1025369 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Confirm abort if saving would make a change to the underlying document

https://gerrit.wikimedia.org/r/1025369

Change #1025390 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Rename isModified to isSaveable and deprecate the old name.

https://gerrit.wikimedia.org/r/1025390

Maintenance_bot removed a project: Patch-For-Review.Apr 30 2024, 5:30 PM

ReleaseTaggerBot added a project: MW-1.43-notes (1.43.0-wmf.4; 2024-05-07).Apr 30 2024, 6:00 PM

zoe changed the task status from Open to In Progress.May 1 2024, 1:47 PM

Thanks for catching this.

Screenshot 2024-05-03 at 23.46.04.png (1×2 px, 151 KB)

Esanders moved this task from QA to Ready for Sign Off on the Editing-team (Kanban Board) board.May 8 2024, 5:17 PM

@zoe : After this change, we are now showing a confirmation dialog even if there are no pending changes which is not how other dialogs in VE work.

EAkinloose attached a referenced file: F50043827: Screenshot 2024-05-03 at 23.46.04.png. (Show Details)May 20 2024, 4:53 PM

ppelberg moved this task from Blocked / Needs More Work to Ready to Be Worked On on the Editing-team (Kanban Board) board.May 20 2024, 5:12 PM

Good catch – definitely not intended behaviour to prevent the dialog from closing in edit mode if the user has not made a change. I'll investigate why it's doing this.

Change #1034134 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/VisualEditor@master] Bugfix: don't confirm abandonment when template edits have no changes

https://gerrit.wikimedia.org/r/1034134

gerritbot added a project: Patch-For-Review.May 20 2024, 5:59 PM

I have confirmed that it was doing this because I introduced a bug and then didn't catch it before submitting the change for QA. Sorry about that.

ppelberg moved this task from Ready to Be Worked On to Code Review on the Editing-team (Kanban Board) board.May 21 2024, 5:28 PM

Change #1034134 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Change confirmation behaviour when abandoning template edits

https://gerrit.wikimedia.org/r/1034134

ReleaseTaggerBot edited projects, added MW-1.43-notes (1.43.0-wmf.8; 2024-06-04); removed MW-1.43-notes (1.43.0-wmf.4; 2024-05-07).May 29 2024, 5:00 PM

Maintenance_bot removed a project: Patch-For-Review.May 29 2024, 5:31 PM

matmarex moved this task from Code Review to QA on the Editing-team (Kanban Board) board.May 29 2024, 9:43 PM

matmarex added a project: Editing QA.

In T334513#9813698, @zoe wrote:

I have confirmed that it was doing this because I introduced a bug and then didn't catch it before submitting the change for QA. Sorry about that.

No problem at all @zoe! Thanks for fixing it.

Ryasmeen edited projects, added Verified; removed Editing QA.May 30 2024, 4:14 PM

In T334513#9813042, @Ryasmeen wrote:

@zoe : After this change, we are now showing a confirmation dialog even if there are no pending changes which is not how other dialogs in VE work.

@zoe: I am not sure if this issue has resurfaced again in some way but I am seeing for two dialogs: Musical Notation and Map, a confirmation dialog is still showing up even if there are no pending changes. It's possible it has been this way from before for these two dialogs.

Update: I think it might be expected for Map though, since it already has some default values populated inside it and attempting to close the dialog probably considers those as changes done in that session and hence triggering that confirmation?

I've been working on this one in the background. I've found that Kartographer (our plugin for maps) keeps its own state, so our attempt to retrieve an "initial" component state upon first load doesn't work as expected. I'll be submitting a change to Kartographer to make it do so. I'll look at Score afterwards.

zoe mentioned this in T374027: Kartographer doesn't close when you press "escape" after interacting with the map.Sep 4 2024, 4:13 PM

Change #1070893 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/Kartographer@master] Close dialog without confirmation if the user hasn't interacted

https://gerrit.wikimedia.org/r/1070893

gerritbot added a project: Patch-For-Review.Sep 5 2024, 10:12 AM

Change #1070893 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Close dialog without confirmation if the user hasn't interacted

https://gerrit.wikimedia.org/r/1070893

ReleaseTaggerBot edited projects, added MW-1.43-notes (1.43.0-wmf.22; 2024-09-10); removed MW-1.43-notes (1.43.0-wmf.8; 2024-06-04).Sep 5 2024, 2:00 PM

Maintenance_bot removed a project: Patch-For-Review.Sep 5 2024, 2:31 PM

Change #1071216 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/Score@master] Close dialog without confirmation if the user hasn't interacted

https://gerrit.wikimedia.org/r/1071216

gerritbot added a project: Patch-For-Review.Sep 9 2024, 10:12 AM

Oops, forgot to make sure that this was linked to the Score change.

This is landed for Maps, not yet landed for Music.

Test wiki created on Patch demo by Zoe-WMF using patch(es) linked to this task:
http://patchdemo.wmcloud.org/wikis/2692dab28b/w/

As above.

I've just noticed that Kartographer doesn't close if you open an existing map and then hit escape – I'll have to look into this – but other than that maps and music notation is behaving as expected.

Change #1071216 merged by jenkins-bot: