Skip to content

Commit

Permalink
Merge pull request #7387 from ckeditor/i/6693
Browse files Browse the repository at this point in the history 
Fix (engine): The model selection post-fixer should not set a new selection if the ranges before and after post-fixing are the same (see #6693).

Tests (image): Aligned image tests to the latest `WidgetTypeAround` plugin API (see #6693).

Other (paragraph): The `InsertParagraphCommand` should split ancestors of the `Position` to find a parent that allows `'paragraph'` (see #6693).

Tests (table): Aligned `Table` plugin tests to the latest `WidgetTypeAround` API. Code refactoring (see #6693).

Feature (theme-lark): Added styles for a "fake caret" brought by the `WidgetTypeAround` plugin (see #6693).

Feature (typing): Created a public `isNonTypingKeystroke()` helper (see #6693).

Feature (utils): Created `isArrowKeyCode()`, `getLocalizedArrowKeyCodeDirection()`, and `isForwardArrowKeyCode()` helpers (see #6693).

Feature (widget): Implemented keyboard support for inserting paragraphs around block widgets using a "fake horizontal caret" (`WidgetTypeAround`). Both "Insert new paragraph" buttons are now always displayed for all block widgets regardless of their location in the document. Closes #6693. Closes #6825. Closes #6694.

Also:

*   code refactoring in the `Widget` plugin,
*   aligned `Widget` tests to the new `WidgetTypeAround` keyboard behavior,
*   moved the `Enter` and `Shift+Enter` support from the `Widget` plugin to the `WidgetTypeAround` plugin.

MINOR BREAKING CHANGE (widget): Removed the `getWidgetTypeAroundPositions()` helper since the "Insert new paragraph" buttons are now visible regardless of the widget location in the document
  • Loading branch information
@niegowski
niegowski committed Jun 10, 2020
2 parents 76c762e + 893a577 commit eb8dd9f
Show file tree
Hide file tree
Showing 24 changed files with 2,370 additions and 573 deletions.
9 changes: 8 additions & 1 deletion packages/ckeditor5-engine/src/model/utils/selection-post-fixer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,14 @@ function selectionPostFixer( writer, model ) {
// Those ranges might overlap but will be corrected later.
const correctedRange = tryFixingRange( modelRange, schema );

if ( correctedRange ) {
// "Selection fixing" algorithms sometimes get lost. In consequence, it may happen
// that a new range is returned but, in fact, it has the same positions as the original
// range anyway. If this range is not discarded, a new selection will be set and that,
// for instance, would destroy the selection attributes. Let's make sure that the post-fixer
// actually worked first before setting a new selection.
//
// https://github.com/ckeditor/ckeditor5/issues/6693
if ( correctedRange && !correctedRange.isEqual( modelRange ) ) {
ranges.push( correctedRange );
wasFixed = true;
} else {
Expand Down
29 changes: 29 additions & 0 deletions packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,35 @@ describe( 'Selection post-fixer', () => {
'</table>]'
);
} );

it( 'should not reset the selection if the final range is the same as the initial one', () => {
setModelData( model,
'<table>' +
'<tableRow>' +
'<tableCell>[<image></image>]</tableCell>' +
'</tableRow>' +
'</table>'
);

// Setting a selection attribute will trigger the post-fixer. However, because this
// action does not affect ranges, the post-fixer should not set a new selection and,
// in consequence, should not clear the selection attribute (like it normally would when
// a new selection is set).
// https://github.com/ckeditor/ckeditor5/issues/6693
model.change( writer => {
writer.setSelectionAttribute( 'foo', 'bar' );
} );

assertEqualMarkup( getModelData( model ),
'<table>' +
'<tableRow>' +
'<tableCell>[<image></image>]</tableCell>' +
'</tableRow>' +
'</table>'
);

expect( model.document.selection.hasAttribute( 'foo' ) ).to.be.true;
} );
} );

describe( 'non-collapsed selection - image scenarios', () => {
Expand Down
6 changes: 0 additions & 6 deletions packages/ckeditor5-image/tests/image.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ describe( 'Image', () => {
expect( getViewData( view ) ).to.equal(
'[<figure class="' +
'ck-widget ' +
'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
'ck-widget_selected image" contenteditable="false"' +
'>' +
'<img alt="alt text" src="/assets/sample.png"></img>' +
Expand All @@ -82,7 +81,6 @@ describe( 'Image', () => {
expect( getViewData( view ) ).to.equal(
'[<figure class="' +
'ck-widget ' +
'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
'ck-widget_selected image" contenteditable="false"' +
'>' +
'<img alt="" src="/assets/sample.png"></img>' +
Expand All @@ -103,15 +101,13 @@ describe( 'Image', () => {
expect( getViewData( view ) ).to.equal(
'[<figure class="' +
'ck-widget ' +
'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
'ck-widget_selected image" contenteditable="false"' +
'>' +
'<img alt="alt text" src="/assets/sample.png"></img>' +
'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
'</figure>]' +
'<figure class="' +
'ck-widget ' +
'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
'image" contenteditable="false"' +
'>' +
'<img alt="alt text" src="/assets/sample.png"></img>' +
Expand All @@ -127,15 +123,13 @@ describe( 'Image', () => {
expect( getViewData( view ) ).to.equal(
'<figure class="' +
'ck-widget ' +
'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
'image" contenteditable="false"' +
'>' +
'<img alt="alt text" src="/assets/sample.png"></img>' +
'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
'</figure>' +
'[<figure class="' +
'ck-widget ' +
'ck-widget_can-type-around_after ck-widget_can-type-around_before ' +
'ck-widget_selected image" contenteditable="false"' +
'>' +
'<img alt="alt text" src="/assets/sample.png"></img>' +
Expand Down
23 changes: 18 additions & 5 deletions packages/ckeditor5-paragraph/src/insertparagraphcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
* position: editor.model.createPositionBefore( element )
* } );
*
* If a paragraph is disallowed in the context of the specific position, the command
* will attempt to split position ancestors to find a place where it is possible
* to insert a paragraph.
*
* **Note**: This command moves the selection to the inserted paragraph.
*
* @extends module:core/command~Command
Expand All @@ -33,15 +37,24 @@ export default class InsertParagraphCommand extends Command {
*/
execute( options ) {
const model = this.editor.model;

if ( !model.schema.checkChild( options.position, 'paragraph' ) ) {
return;
}
let position = options.position;

model.change( writer => {
const paragraph = writer.createElement( 'paragraph' );

model.insertContent( paragraph, options.position );
if ( !model.schema.checkChild( position.parent, paragraph ) ) {
const allowedParent = model.schema.findAllowedParent( position, paragraph );

// It could be there's no ancestor limit that would allow paragraph.
// In theory, "paragraph" could be disallowed even in the "$root".
if ( !allowedParent ) {
return;
}

position = writer.split( position, allowedParent ).position;
}

model.insertContent( paragraph, position );

writer.setSelection( paragraph, 'in' );
} );
Expand Down
34 changes: 28 additions & 6 deletions packages/ckeditor5-paragraph/tests/insertparagraphcommand.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ describe( 'InsertParagraphCommand', () => {
editor.commands.add( 'insertParagraph', command );
schema.register( 'paragraph', { inheritAllFrom: '$block' } );
schema.register( 'heading1', { inheritAllFrom: '$block', allowIn: 'headersOnly' } );
schema.register( 'headersOnly', { inheritAllFrom: '$block' } );
schema.register( 'allowP', { inheritAllFrom: '$block' } );
schema.register( 'disallowP', { inheritAllFrom: '$block', allowIn: [ 'allowP' ] } );
model.schema.extend( 'paragraph', { allowIn: [ 'allowP' ] } );
} );
} );

Expand All @@ -42,18 +44,38 @@ describe( 'InsertParagraphCommand', () => {
expect( getData( model ) ).to.equal( '<paragraph>[]</paragraph><heading1>foo</heading1>' );
} );

it( 'should do nothing if the paragraph is not allowed at the provided position', () => {
setData( model, '<headersOnly><heading1>foo[]</heading1></headersOnly>' );
it( 'should split ancestors down to a limit where a paragraph is allowed', () => {
setData( model, '<allowP><disallowP>foo</disallowP></allowP>' );

command.execute( {
position: model.createPositionBefore( root.getChild( 0 ).getChild( 0 ) )
// fo[]o
position: model.createPositionAt( root.getChild( 0 ).getChild( 0 ), 2 )
} );

expect( getData( model ) ).to.equal(
'<allowP>' +
'<disallowP>fo</disallowP>' +
'<paragraph>[]</paragraph>' +
'<disallowP>o</disallowP>' +
'</allowP>'
);
} );

it( 'should do nothing if the paragraph is not allowed at the provided position', () => {
// Create a situation where "paragraph" is disallowed even in the "root".
schema.addChildCheck( ( context, childDefinition ) => {
if ( context.endsWith( '$root' ) && childDefinition.name == 'paragraph' ) {
return false;
}
} );

setData( model, '<heading1>foo[]</heading1>' );

command.execute( {
position: model.createPositionAfter( root.getChild( 0 ).getChild( 0 ) )
position: model.createPositionBefore( root.getChild( 0 ) )
} );

expect( getData( model ) ).to.equal( '<headersOnly><heading1>foo[]</heading1></headersOnly>' );
expect( getData( model ) ).to.equal( '<heading1>foo[]</heading1>' );
} );

describe( 'interation with existing paragraphs in the content', () => {
Expand Down
86 changes: 10 additions & 76 deletions packages/ckeditor5-table/src/tablekeyboard.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import TableWalker from './tablewalker';
import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect';
import priorities from '@ckeditor/ckeditor5-utils/src/priorities';
import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
import {
isArrowKeyCode,
getLocalizedArrowKeyCodeDirection
} from '@ckeditor/ckeditor5-utils/src/keyboard';
import { getSelectedTableCells, getTableCellsContainingSelection } from './utils/selection';
import { findAncestor } from './utils/common';

Expand Down Expand Up @@ -50,11 +53,10 @@ export default class TableKeyboard extends Plugin {
this.editor.keystrokes.set( 'Tab', this._getTabHandler( true ), { priority: 'low' } );
this.editor.keystrokes.set( 'Shift+Tab', this._getTabHandler( false ), { priority: 'low' } );

// Note: This listener has the "high+1" priority because we would like to avoid collisions with other features
// (like Widgets), which take over the keydown events with the "high" priority. Table navigation takes precedence
// over Widgets in that matter (widget arrow handler stops propagation of event if object element was selected
// but getNearestSelectionRange didn't returned any range).
this.listenTo( viewDocument, 'keydown', ( ...args ) => this._onKeydown( ...args ), { priority: priorities.get( 'high' ) + 1 } );
// Note: This listener has the "high-10" priority because it should allow the Widget plugin to handle the default
// behavior first ("high") but it should not be "prevent–defaulted" by the Widget plugin ("high-20") because of
// the fake selection retention on the fully selected widget.
this.listenTo( viewDocument, 'keydown', ( ...args ) => this._onKeydown( ...args ), { priority: priorities.get( 'high' ) - 10 } );
}

/**
Expand Down Expand Up @@ -163,13 +165,14 @@ export default class TableKeyboard extends Plugin {
* @param {module:engine/view/observer/domeventdata~DomEventData} domEventData
*/
_onKeydown( eventInfo, domEventData ) {
const editor = this.editor;
const keyCode = domEventData.keyCode;

if ( !isArrowKeyCode( keyCode ) ) {
return;
}

const direction = getDirectionFromKeyCode( keyCode, this.editor.locale.contentLanguageDirection );
const direction = getLocalizedArrowKeyCodeDirection( keyCode, editor.locale.contentLanguageDirection );
const wasHandled = this._handleArrowKeys( direction, domEventData.shiftKey );

if ( wasHandled ) {
Expand Down Expand Up @@ -226,19 +229,6 @@ export default class TableKeyboard extends Plugin {
return true;
}

// If this is an object selected and it's not at the start or the end of cell content
// then let's allow widget handler to take care of it.
const objectElement = selection.getSelectedElement();

if ( objectElement && model.schema.isObject( objectElement ) ) {
return false;
}

// If next to the selection there is an object then this is not the cell boundary (widget handler should handle this).
if ( this._isObjectElementNextToSelection( selection, isForward ) ) {
return false;
}

// If there isn't any $text position between cell edge and selection then we shall move the selection to next cell.
const textRange = this._findTextRangeFromSelection( cellRange, selection, isForward );

Expand Down Expand Up @@ -303,27 +293,6 @@ export default class TableKeyboard extends Plugin {
return focus.isEqual( probe.focus );
}

/**
* Checks if there is an {@link module:engine/model/element~Element element} next to the current
* {@link module:engine/model/selection~Selection model selection} marked in the
* {@link module:engine/model/schema~Schema schema} as an `object`.
*
* @private
* @param {module:engine/model/selection~Selection} modelSelection The selection.
* @param {Boolean} isForward The direction of checking.
* @returns {Boolean}
*/
_isObjectElementNextToSelection( modelSelection, isForward ) {
const model = this.editor.model;
const schema = model.schema;

const probe = model.createSelection( modelSelection );
model.modifySelection( probe, { direction: isForward ? 'forward' : 'backward' } );
const objectElement = isForward ? probe.focus.nodeBefore : probe.focus.nodeAfter;

return objectElement && schema.isObject( objectElement );
}

/**
* Truncates the range so that it spans from the last selection position
* to the last allowed `$text` position (mirrored if `isForward` is false).
Expand Down Expand Up @@ -515,38 +484,3 @@ export default class TableKeyboard extends Plugin {
}
}

// Returns `true` if the provided key code represents one of the arrow keys.
//
// @private
// @param {Number} keyCode
// @returns {Boolean}
function isArrowKeyCode( keyCode ) {
return keyCode == keyCodes.arrowright ||
keyCode == keyCodes.arrowleft ||
keyCode == keyCodes.arrowup ||
keyCode == keyCodes.arrowdown;
}

// Returns the direction name from `keyCode`.
//
// @private
// @param {Number} keyCode
// @param {String} contentLanguageDirection The content language direction.
// @returns {'left'|'up'|'right'|'down'} Arrow direction.
function getDirectionFromKeyCode( keyCode, contentLanguageDirection ) {
const isLtrContent = contentLanguageDirection === 'ltr';

switch ( keyCode ) {
case keyCodes.arrowleft:
return isLtrContent ? 'left' : 'right';

case keyCodes.arrowright:
return isLtrContent ? 'right' : 'left';

case keyCodes.arrowup:
return 'up';

case keyCodes.arrowdown:
return 'down';
}
}
10 changes: 7 additions & 3 deletions packages/ckeditor5-table/tests/converters/upcasttable.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
import { getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import ImageEditing from '@ckeditor/ckeditor5-image/src/image/imageediting';
Expand All @@ -17,8 +17,8 @@ describe( 'upcastTable()', () => {
let editor, model;

beforeEach( () => {
return VirtualTestEditor
.create( {
return ClassicTestEditor
.create( '', {
plugins: [ TableEditing, Paragraph, ImageEditing, Widget ]
} )
.then( newEditor => {
Expand All @@ -31,6 +31,10 @@ describe( 'upcastTable()', () => {
} );
} );

afterEach( () => {
editor.destroy();
} );

function expectModel( data ) {
assertEqualMarkup( getModelData( model, { withoutSelection: true } ), data );
}
Expand Down
Loading

0 comments on commit eb8dd9f

Please sign in to comment.