Merge pull request #17344 from ckeditor/ck/17343

Fix (ui): The dialog plugin should not handle Esc key press when default-prevented by the guest view. Closes #17343.
ckeditor · Oct 28, 2024 · bf14897 · bf14897
2 parents 49edd01 + 92ad76c
commit bf14897
Show file tree

Hide file tree

Showing 5 changed files with 265 additions and 9 deletions.
diff --git a/packages/ckeditor5-ui/src/dialog/dialogview.ts b/packages/ckeditor5-ui/src/dialog/dialogview.ts
@@ -290,8 +290,12 @@ export default class DialogView extends /* #__PURE__ */ DraggableViewMixin( View
 		super.render();
 
 		this.keystrokes.set( 'Esc', ( data, cancel ) => {
-			this.fire<DialogViewCloseEvent>( 'close', { source: 'escKeyPress' } );
-			cancel();
+			// Do not react to the Esc key if the event has already been handled and defaultPrevented
+			// by some logic of the dialog guest (child) view (https://github.com/ckeditor/ckeditor5/issues/17343).
+			if ( !data.defaultPrevented ) {
+				this.fire<DialogViewCloseEvent>( 'close', { source: 'escKeyPress' } );
+				cancel();
+			}
 		} );
 
 		// Support for dragging the modal.

diff --git a/packages/ckeditor5-ui/tests/dialog/dialogview.js b/packages/ckeditor5-ui/tests/dialog/dialogview.js
@@ -411,15 +411,71 @@ describe( 'DialogView', () => {
 			view.render();
 		} );
 
-		it( 'should emit the event with data upon pressing Esc', () => {
-			const spy = sinon.spy();
+		describe( 'Esc key press handling', () => {
+			it( 'should emit the "close" event with data upon pressing Esc', () => {
+				const spy = sinon.spy();
 
-			view.on( 'close', spy );
+				view.on( 'close', spy );
 
-			view.element.dispatchEvent( new KeyboardEvent( 'keydown', { keyCode: keyCodes.a } ) );
-			sinon.assert.notCalled( spy );
-			view.element.dispatchEvent( new KeyboardEvent( 'keydown', { keyCode: keyCodes.esc } ) );
-			sinon.assert.calledWithExactly( spy, sinon.match.any, { source: 'escKeyPress' } );
+				const unrelatedDomEvent = new KeyboardEvent( 'keydown', {
+					keyCode: keyCodes.a,
+					bubbles: true,
+					cancelable: true
+				} );
+
+				sinon.stub( unrelatedDomEvent, 'stopPropagation' );
+				sinon.stub( unrelatedDomEvent, 'preventDefault' );
+
+				view.element.dispatchEvent( unrelatedDomEvent );
+
+				sinon.assert.notCalled( spy );
+				sinon.assert.notCalled( unrelatedDomEvent.stopPropagation );
+				sinon.assert.notCalled( unrelatedDomEvent.preventDefault );
+
+				const escDomEvent = new KeyboardEvent( 'keydown', {
+					keyCode: keyCodes.esc,
+					bubbles: true,
+					cancelable: true
+				} );
+
+				sinon.stub( escDomEvent, 'stopPropagation' );
+				sinon.stub( escDomEvent, 'preventDefault' );
+
+				view.element.dispatchEvent( escDomEvent );
+
+				sinon.assert.calledWithExactly( spy, sinon.match.any, {
+					source: 'escKeyPress'
+				} );
+
+				sinon.assert.calledOnce( escDomEvent.stopPropagation );
+				sinon.assert.calledOnce( escDomEvent.preventDefault );
+			} );
+
+			it( 'should not emit the "close" event if the original DOM event was preventDefaulted by some other logic', () => {
+				const spy = sinon.spy();
+				const childView = createContentView( 'A' );
+
+				view.setupParts( { content: childView } );
+				view.on( 'close', spy );
+
+				// Some child view's logic handling the key press.
+				childView.element.addEventListener( 'keydown', evt => {
+					evt.preventDefault();
+				} );
+
+				const escDomEvent = new KeyboardEvent( 'keydown', {
+					keyCode: keyCodes.esc,
+					bubbles: true,
+					cancelable: true
+				} );
+
+				sinon.stub( escDomEvent, 'stopPropagation' );
+
+				childView.element.dispatchEvent( escDomEvent );
+
+				sinon.assert.notCalled( spy );
+				sinon.assert.notCalled( escDomEvent.stopPropagation );
+			} );
 		} );
 
 		it( 'should move the dialog upon the #drag event', () => {

diff --git a/packages/ckeditor5-ui/tests/manual/dialog/dialog-esc-key-handling.html b/packages/ckeditor5-ui/tests/manual/dialog/dialog-esc-key-handling.html
@@ -0,0 +1,24 @@
+<div id="editor">
+	<h2>Heading 1</h2>
+	<p>Paragraph</p>
+	<p><strong>Bold</strong> <i>Italic</i> <a href="https://ckeditor.com">Link</a></p>
+	<ul>
+		<li>UL List item 1</li>
+		<li>UL List item 2</li>
+	</ul>
+	<ol>
+		<li>OL List item 1</li>
+		<li>OL List item 2</li>
+	</ol>
+	<figure class="image image-style-side">
+		<img style="aspect-ratio:800/376;" src="sample.jpg" alt="bar" width="800" height="376">
+	</figure>
+	<blockquote>
+		<p>Quote</p>
+		<ul>
+			<li>Quoted UL List item 1</li>
+			<li>Quoted UL List item 2</li>
+		</ul>
+		<p>Quote</p>
+	</blockquote>
+</div>
diff --git a/packages/ckeditor5-ui/tests/manual/dialog/dialog-esc-key-handling.md b/packages/ckeditor5-ui/tests/manual/dialog/dialog-esc-key-handling.md
@@ -0,0 +1,6 @@
+# Esc key handling in dialogs
+
+1. This manual test showcases how the dialog system interacts with the Esc key press.
+2. Open the dialog by clicking the "Open dialog" button.
+3. Keep pressing Esc key according to the instructions.
+4. The dialog should not close until the last (10th) Esc key press. Only then the child view will stop `preventDefaulting` on the keydown event.
diff --git a/packages/ckeditor5-ui/tests/manual/dialog/dialog-esc-key-handling.ts b/packages/ckeditor5-ui/tests/manual/dialog/dialog-esc-key-handling.ts
@@ -0,0 +1,166 @@
+/**
+ * @license Copyright (c) 2003-2024, CKSource Holding sp. z o.o. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
+ */
+
+/* globals console, document */
+
+declare global {
+	interface Window { CKEditorInspector: any }
+}
+
+import { Essentials } from '@ckeditor/ckeditor5-essentials';
+import { Autoformat } from '@ckeditor/ckeditor5-autoformat';
+import { BlockQuote } from '@ckeditor/ckeditor5-block-quote';
+import { Bold, Italic } from '@ckeditor/ckeditor5-basic-styles';
+import { Heading } from '@ckeditor/ckeditor5-heading';
+import { Image, ImageCaption, ImageStyle, ImageToolbar } from '@ckeditor/ckeditor5-image';
+import { Indent } from '@ckeditor/ckeditor5-indent';
+import { Link } from '@ckeditor/ckeditor5-link';
+import { List } from '@ckeditor/ckeditor5-list';
+import { MediaEmbed } from '@ckeditor/ckeditor5-media-embed';
+import { Paragraph } from '@ckeditor/ckeditor5-paragraph';
+import { Table, TableToolbar } from '@ckeditor/ckeditor5-table';
+import { ClassicEditor } from '@ckeditor/ckeditor5-editor-classic';
+import FindAndReplace from '@ckeditor/ckeditor5-find-and-replace/src/findandreplace.js';
+import SpecialCharacters from '@ckeditor/ckeditor5-special-characters/src/specialcharacters.js';
+import SpecialCharactersEssentials from '@ckeditor/ckeditor5-special-characters/src/specialcharactersessentials.js';
+import SourceEditing from '@ckeditor/ckeditor5-source-editing/src/sourceediting.js';
+import { ButtonView, Dialog, View } from '../../../src/index.js';
+import { Plugin } from '@ckeditor/ckeditor5-core';
+
+class ViewWithEscSupport extends View {
+	declare public count: number;
+
+	constructor() {
+		super();
+
+		const bind = this.bindTemplate;
+
+		this.set( 'count', 0 );
+
+		this.setTemplate( {
+			tag: 'div',
+			attributes: {
+				tabindex: -1,
+				style: {
+					padding: '20px'
+				}
+			},
+			children: [
+				{
+					tag: 'p',
+					children: [
+						'Focus me and press Esc key 10 times. Count: ',
+						{ text: bind.to( 'count' ) }
+					]
+				}
+			],
+			on: {
+				keydown: bind.to( ( evt: Event ) => {
+					if ( ( evt as KeyboardEvent ).key == 'Escape' ) {
+						if ( this.count++ < 9 ) {
+							evt.preventDefault();
+						}
+					}
+				} )
+			}
+		} );
+	}
+
+	public focus() {
+		this.element!.focus();
+	}
+}
+
+class DialogWithEscapeableChildren extends Plugin {
+	public static get requires() {
+		return [ Dialog ] as const;
+	}
+
+	public init(): void {
+		const t = this.editor.locale.t;
+
+		this.editor.ui.componentFactory.add( 'dialogWithEscHandling', locale => {
+			const buttonView = new ButtonView( locale );
+
+			buttonView.set( {
+				label: t( 'Open dialog' ),
+				tooltip: true,
+				withText: true
+			} );
+
+			buttonView.on( 'execute', () => {
+				const dialog = this.editor.plugins.get( 'Dialog' );
+
+				dialog.show( {
+					id: 'dialogWithEscHandling',
+					isModal: true,
+					title: t( 'Dialog with esc handling' ),
+					content: new ViewWithEscSupport(),
+					actionButtons: [
+
+						{
+							label: t( 'Cancel' ),
+							withText: true,
+							onExecute: () => dialog.hide()
+						}
+					]
+				} );
+			} );
+
+			return buttonView;
+		} );
+	}
+}
+
+ClassicEditor.create( document.querySelector( '#editor' ) as HTMLElement, {
+	plugins: [
+		Essentials,
+		Autoformat,
+		BlockQuote,
+		Bold,
+		Heading,
+		Image,
+		ImageCaption,
+		ImageStyle,
+		ImageToolbar,
+		Indent,
+		Italic,
+		Link,
+		List,
+		MediaEmbed,
+		Paragraph,
+		Table,
+		TableToolbar,
+
+		FindAndReplace,
+		SpecialCharacters,
+		SpecialCharactersEssentials,
+		SourceEditing,
+
+		DialogWithEscapeableChildren
+	],
+	toolbar: {
+		items: [
+			'dialogWithEscHandling',
+			'|',
+			'accessibilityHelp', 'heading', 'bold', 'italic', 'link', 'sourceediting', 'findAndReplace'
+		],
+		shouldNotGroupWhenFull: true
+	},
+	image: {
+		toolbar: [ 'imageStyle:inline', 'imageStyle:block', 'imageStyle:side', '|', 'imageTextAlternative' ]
+	},
+	ui: {
+		viewportOffset: {
+			top: 50
+		}
+	}
+} )
+	.then( editor => {
+		Object.assign( window, { editor } );
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );