Merge pull request #109 from ckeditor/t/97

Fix: Keyboard navigation should work around widgets in RTL content. Closes #97.
ckeditor · Sep 19, 2019 · dfbf88d · dfbf88d
2 parents 7755615 + 91c10f0
commit dfbf88d
Show file tree

Hide file tree

Showing 5 changed files with 239 additions and 2 deletions.
diff --git a/src/widget.js b/src/widget.js
@@ -164,7 +164,8 @@ export default class Widget extends Plugin {
 	 */
 	_onKeydown( eventInfo, domEventData ) {
 		const keyCode = domEventData.keyCode;
-		const isForward = keyCode == keyCodes.arrowdown || keyCode == keyCodes.arrowright;
+		const isLtrContent = this.editor.locale.contentLanguageDirection === 'ltr';
+		const isForward = keyCode == keyCodes.arrowdown || keyCode == keyCodes[ isLtrContent ? 'arrowright' : 'arrowleft' ];
 		let wasHandled = false;
 
 		// Checks if the keys were handled and then prevents the default event behaviour and stops

diff --git a/tests/manual/keyboard.html b/tests/manual/keyboard.html
@@ -0,0 +1,46 @@
+<head>
+	<style>
+		body {
+			max-width: 800px;
+			margin: 20px auto;
+		}
+
+		.ck-content .widget {
+			background: rgba( 0, 0, 0, 0.1 );
+			min-height: 50px;
+		}
+
+		.ck-content placeholder {
+			background: #ffff00;
+			padding: 4px 2px;
+			outline-offset: -2px;
+			line-height: 1em;
+			/* Set margins before and after the inline widget so the cursor between two instances is visible when one of them is focused.
+			Optimized for Firefox due to bug in rendering selection in ZWS on Chrome. See ckeditor5#1607. */
+			margin-right: 1px;
+			margin-left: 3px;
+		}
+
+		.ck-content placeholder::selection {
+			display: none;
+		}
+	</style>
+</head>
+
+<h2>LTR content</h2>
+
+<div id="editor-ltr">
+	<h2>Heading 1</h2>
+	<p>Para<placeholder>inline widget</placeholder>graph</p>
+	<div class="widget"></div>
+	<p>Paragraph</p>
+</div>
+
+<h2>RTL content</h2>
+
+<div id="editor-rtl">
+	<h2>مرحبا</h2>
+	<p>مرحبا<placeholder>inline widget</placeholder>مرحبا</p>
+	<div class="widget"></div>
+	<p>مرحبا</p>
+</div>
diff --git a/tests/manual/keyboard.js b/tests/manual/keyboard.js
@@ -0,0 +1,130 @@
+/**
+ * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
+ */
+
+/* globals console, window, document */
+
+import Widget from '../../src/widget';
+import { toWidget, viewToModelPositionOutsideModelElement } from '../../src/utils';
+import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset';
+
+function BlockWidget( editor ) {
+	editor.model.schema.register( 'div', {
+		allowIn: [ '$root' ],
+		isObject: true
+	} );
+
+	editor.conversion.for( 'downcast' ).elementToElement( {
+		model: 'div',
+		view: ( modelElement, writer ) => {
+			return toWidget(
+				writer.createContainerElement( 'div', {
+					class: 'widget'
+				} ),
+				writer,
+				{ hasSelectionHandler: true }
+			);
+		}
+	} );
+
+	editor.conversion.for( 'upcast' ).elementToElement( {
+		model: 'div',
+		view: 'div'
+	} );
+}
+
+class InlineWidget extends Plugin {
+	constructor( editor ) {
+		super( editor );
+
+		editor.model.schema.register( 'placeholder', {
+			allowWhere: '$text',
+			isObject: true,
+			isInline: true
+		} );
+
+		editor.conversion.for( 'editingDowncast' ).elementToElement( {
+			model: 'placeholder',
+			view: ( modelItem, viewWriter ) => {
+				const widgetElement = createPlaceholderView( modelItem, viewWriter );
+
+				return toWidget( widgetElement, viewWriter );
+			}
+		} );
+
+		editor.conversion.for( 'dataDowncast' ).elementToElement( {
+			model: 'placeholder',
+			view: createPlaceholderView
+		} );
+
+		editor.conversion.for( 'upcast' ).elementToElement( {
+			view: 'placeholder',
+			model: 'placeholder'
+		} );
+
+		editor.editing.mapper.on(
+			'viewToModelPosition',
+			viewToModelPositionOutsideModelElement( editor.model, viewElement => viewElement.name == 'placeholder' )
+		);
+
+		function createPlaceholderView( modelItem, viewWriter ) {
+			const widgetElement = viewWriter.createContainerElement( 'placeholder' );
+			const viewText = viewWriter.createText( '{placeholder}' );
+
+			viewWriter.insert( viewWriter.createPositionAt( widgetElement, 0 ), viewText );
+
+			return widgetElement;
+		}
+	}
+}
+
+const config = {
+	plugins: [ ArticlePluginSet, Widget, InlineWidget, BlockWidget ],
+	toolbar: [
+		'heading',
+		'|',
+		'bold',
+		'italic',
+		'link',
+		'bulletedList',
+		'numberedList',
+		'blockQuote',
+		'insertTable',
+		'mediaEmbed',
+		'undo',
+		'redo'
+	],
+	image: {
+		toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ]
+	},
+	table: {
+		contentToolbar: [
+			'tableColumn',
+			'tableRow',
+			'mergeTableCells'
+		]
+	}
+};
+
+ClassicEditor
+	.create( document.querySelector( '#editor-ltr' ), config )
+	.then( editor => {
+		window.editorLtr = editor;
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );
+
+ClassicEditor
+	.create( document.querySelector( '#editor-rtl' ), Object.assign( {}, config, {
+		language: 'ar'
+	} ) )
+	.then( editor => {
+		window.editorRtl = editor;
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );
diff --git a/tests/manual/keyboard.md b/tests/manual/keyboard.md
@@ -0,0 +1,20 @@
+# Keyboard navigation across widgets
+
+1. Put a selection at the beginning of the document.
+2. Use the **right arrow** key to navigate forwards.
+3. Go through inline and block widgets.
+4. The navigation should
+	* be uninterrupted,
+	* go always forward,
+	* select widgets on its way.
+5. Reach the end of the document.
+6. Go backwards using the **left arrow** key and repeat the entire scenario.
+
+## RTL (right–to–left) content navigation
+
+In this scenario the content is written in Arabic.
+
+1. Repeat the scenario from the previous section but note that the content is mirrored, i.e. the beginning is on the right–hand side.
+2. Forwards navigation is done by pressing the **left arrow** key.
+3. To go backwards, use the **right arrow** key.
+4. Just like with the LTR content, the navigation should be seamless, always in the same direction.
diff --git a/tests/widget.js b/tests/widget.js
@@ -657,6 +657,44 @@ describe( 'Widget', () => {
 				'[<image></image>]' +
 				'<paragraph>foo</paragraph>'
 			);
+
+			describe( 'RTL (right-to-left) content', () => {
+				test(
+					'should move selection forward from selected object - left arrow',
+					'[<widget></widget>]<paragraph>foo</paragraph>',
+					keyCodes.arrowleft,
+					'<widget></widget><paragraph>[]foo</paragraph>',
+					null,
+					'rtl'
+				);
+
+				test(
+					'should move selection backward from selected object - right arrow',
+					'<paragraph>foo</paragraph>[<widget></widget>]',
+					keyCodes.arrowright,
+					'<paragraph>foo[]</paragraph><widget></widget>',
+					null,
+					'rtl'
+				);
+
+				test(
+					'should move selection to next widget - left arrow',
+					'[<widget></widget>]<widget></widget>',
+					keyCodes.arrowleft,
+					'<widget></widget>[<widget></widget>]',
+					null,
+					'rtl'
+				);
+
+				test(
+					'should move selection to previous widget - right arrow',
+					'<widget></widget>[<widget></widget>]',
+					keyCodes.arrowright,
+					'[<widget></widget>]<widget></widget>',
+					null,
+					'rtl'
+				);
+			} );
 		} );
 
 		describe( 'Ctrl+A', () => {
@@ -782,8 +820,10 @@ describe( 'Widget', () => {
 			);
 		} );
 
-		function test( name, data, keyCodeOrMock, expected, expectedView ) {
+		function test( name, data, keyCodeOrMock, expected, expectedView, contentLanguageDirection = 'ltr' ) {
 			it( name, () => {
+				testUtils.sinon.stub( editor.locale, 'contentLanguageDirection' ).value( contentLanguageDirection );
+
 				const domEventDataMock = ( typeof keyCodeOrMock == 'object' ) ? keyCodeOrMock : {
 					keyCode: keyCodeOrMock
 				};