Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1431 from ckeditor/t/ckeditor5-table/12
Browse files Browse the repository at this point in the history
Feature: Introduced a selection post-fixer. Its role is to ensure that after all changes are applied the selection is placed in a correct position. Closes #1156. Closes #1176. Closes #1182. Closes ckeditor/ckeditor5-table#11. Closes ckeditor/ckeditor5-table#12. Closes ckeditor/ckeditor5-table#15. Closes ckeditor/ckeditor5#562. Closes ckeditor/ckeditor5#611.
  • Loading branch information
Reinmar committed Jun 18, 2018
2 parents 457afde + 0c10532 commit 6cf91a1
Show file tree
Hide file tree
Showing 23 changed files with 873 additions and 46 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ The good news is that the our focus when designing the new API was on developer

* Cleaned up the model, document and controllers API. Closes [#1208](https://github.com/ckeditor/ckeditor5-engine/issues/1208). ([aea6119](https://github.com/ckeditor/ckeditor5-engine/commit/aea6119))
* Conversion utilities refactor. Closes [#1236](https://github.com/ckeditor/ckeditor5-engine/issues/1236). ([fd128a1](https://github.com/ckeditor/ckeditor5-engine/commit/fd128a1))
* Fix `render()` and `change()` flow. Introduce postfixers in view. Closes [#1312](https://github.com/ckeditor/ckeditor5-engine/issues/1312). ([63b9d14](https://github.com/ckeditor/ckeditor5-engine/commit/63b9d14))
* Fixed `render()` and `change()` methods flow. Introduced post-fixers in the view. Closes [#1312](https://github.com/ckeditor/ckeditor5-engine/issues/1312). ([63b9d14](https://github.com/ckeditor/ckeditor5-engine/commit/63b9d14))
* Introduced several improvements to conversion helpers. Closes [#1295](https://github.com/ckeditor/ckeditor5-engine/issues/1295). Closes [#1293](https://github.com/ckeditor/ckeditor5-engine/issues/1293). Closes [#1292](https://github.com/ckeditor/ckeditor5-engine/issues/1292). Closes [#1291](https://github.com/ckeditor/ckeditor5-engine/issues/1291). Closes [#1290](https://github.com/ckeditor/ckeditor5-engine/issues/1290). Closes [#1305](https://github.com/ckeditor/ckeditor5-engine/issues/1305). ([809ea24](https://github.com/ckeditor/ckeditor5-engine/commit/809ea24))
* Keep the same marker instance when marker is updated. ([8eba5e9](https://github.com/ckeditor/ckeditor5-engine/commit/8eba5e9))
* Make `Position` and `Range` immutable in model and view. Closes [#897](https://github.com/ckeditor/ckeditor5-engine/issues/897). ([836dfd8](https://github.com/ckeditor/ckeditor5-engine/commit/836dfd8))
Expand Down
6 changes: 1 addition & 5 deletions src/controller/editingcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,7 @@ import Mapper from '../conversion/mapper';
import DowncastDispatcher from '../conversion/downcastdispatcher';
import { insertText, remove } from '../conversion/downcast-converters';
import { convertSelectionChange } from '../conversion/upcast-selection-converters';
import {
convertRangeSelection,
convertCollapsedSelection,
clearAttributes
} from '../conversion/downcast-selection-converters';
import { clearAttributes, convertCollapsedSelection, convertRangeSelection } from '../conversion/downcast-selection-converters';

import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
import mix from '@ckeditor/ckeditor5-utils/src/mix';
Expand Down
4 changes: 1 addition & 3 deletions src/dev-utils/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

import RootElement from '../model/rootelement';
import Model from '../model/model';
import Batch from '../model/batch';
import ModelRange from '../model/range';
import ModelPosition from '../model/position';
import ModelSelection from '../model/selection';
Expand Down Expand Up @@ -94,7 +93,6 @@ export function setData( model, data, options = {} ) {

let modelDocumentFragment, selection;
const modelRoot = model.document.getRoot( options.rootName || 'main' );
const batch = new Batch( options.batchType || 'transparent' );

// Parse data string to model.
const parsedResult = setData._parse( data, model.schema, {
Expand All @@ -111,7 +109,7 @@ export function setData( model, data, options = {} ) {
modelDocumentFragment = parsedResult;
}

model.enqueueChange( batch, writer => {
model.change( writer => {
// Replace existing model in document by new one.
writer.remove( ModelRange.createIn( modelRoot ) );
writer.insert( modelDocumentFragment, modelRoot );
Expand Down
2 changes: 1 addition & 1 deletion src/model/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ export default class Document {
} );

// Wait for `_change` event from model, which signalizes that outermost change block has finished.
// When this happens, check if there were any changes done on document, and if so, call post fixers,
// When this happens, check if there were any changes done on document, and if so, call post-fixers,
// fire `change` event for features and conversion and then reset the differ.
// Fire `change:data` event when at least one operation or buffered marker changes the data.
this.listenTo( model, '_change', ( evt, writer ) => {
Expand Down
3 changes: 3 additions & 0 deletions src/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import insertContent from './utils/insertcontent';
import deleteContent from './utils/deletecontent';
import modifySelection from './utils/modifyselection';
import getSelectedContent from './utils/getselectedcontent';
import { injectSelectionPostFixer } from './utils/selection-post-fixer';

/**
* Editor's data model. Read about the model in the
Expand Down Expand Up @@ -111,6 +112,8 @@ export default class Model {
this.schema.register( '$marker', {
allowIn: [ '$root', '$block' ]
} );

injectSelectionPostFixer( this );
}

/**
Expand Down
27 changes: 20 additions & 7 deletions src/model/schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -333,18 +333,24 @@ export default class Schema {

/**
* Returns `true` if the given item is defined to be
* a limit element by {@link module:engine/model/schema~SchemaItemDefinition}'s `isLimit` property.
* a limit element by {@link module:engine/model/schema~SchemaItemDefinition}'s `isLimit` or `isObject` property
* (all objects are also limits).
*
* schema.isLimit( 'paragraph' ); // -> false
* schema.isLimit( '$root' ); // -> true
* schema.isLimit( editor.model.document.getRoot() ); // -> true
* schema.isLimit( 'image' ); // -> true
*
* @param {module:engine/model/item~Item|module:engine/model/schema~SchemaContextItem|String} item
*/
isLimit( item ) {
const def = this.getDefinition( item );

return !!( def && def.isLimit );
if ( !def ) {
return false;
}

return !!( def.isLimit || def.isObject );
}

/**
Expand Down Expand Up @@ -375,6 +381,11 @@ export default class Schema {
* } );
* schema.checkChild( model.document.getRoot(), paragraph ); // -> true
*
* Note: When verifying whether the given node can be a child of the given context,
* schema also verifies the entire context – from its root to its last element. Therefore, it is possible
* for `checkChild()` to return `false` even though context's last element can contain the checked child.
* It happens if one of the context's elements does not allow its child.
*
* @fires checkChild
* @param {module:engine/model/schema~SchemaContextDefinition} context Context in which the child will be checked.
* @param {module:engine/model/node~Node|String} def The child to check.
Expand Down Expand Up @@ -742,8 +753,8 @@ export default class Schema {
return parent;
}

// Do not split limit elements and objects.
if ( this.isLimit( parent ) || this.isObject( parent ) ) {
// Do not split limit elements.
if ( this.isLimit( parent ) ) {
return null;
}

Expand Down Expand Up @@ -969,7 +980,7 @@ mix( Schema, ObservableMixin );
* * `allowAttributesOf` – a string or an array of strings. Inherit attributes from other items.
* * `inheritTypesFrom` – a string or an array of strings. Inherit `is*` properties of other items.
* * `inheritAllFrom` – a string. A shorthand for `allowContentOf`, `allowWhere`, `allowAttributesOf`, `inheritTypesFrom`.
* * additionall, you can define the following `is*` properties: `isBlock`, `isLimit`, `isObject`. Read about them below.
* * additionally, you can define the following `is*` properties: `isBlock`, `isLimit`, `isObject`. Read about them below.
*
* # The is* properties
*
Expand All @@ -982,9 +993,11 @@ mix( Schema, ObservableMixin );
* Most block type items will inherit from `$block` (through `inheritAllFrom`).
* * `isLimit` – can be understood as whether this element should not be split by <kbd>Enter</kbd>.
* Examples of limit elements – `$root`, table cell, image caption, etc. In other words, all actions which happen inside
* a limit element are limitted to its content.
* a limit element are limited to its content. **Note:** all objects (`isObject`) are treated as limit elements too.
* * `isObject` – whether item is "self-contained" and should be treated as a whole. Examples of object elements –
* `image`, `table`, `video`, etc.
* `image`, `table`, `video`, etc. **Note:** an object is also a limit so
* {@link module:engine/model/schema~Schema#isLimit `isLimit()`}
* returns `true` for object elements automatically.
*
* # Generic items
*
Expand Down
2 changes: 1 addition & 1 deletion src/model/utils/deletecontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ function checkCanBeMerged( leftPos, rightPos, schema ) {
const rangeToCheck = new Range( leftPos, rightPos );

for ( const value of rangeToCheck.getWalker() ) {
if ( schema.isObject( value.item ) || schema.isLimit( value.item ) ) {
if ( schema.isLimit( value.item ) ) {
return false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/model/utils/insertcontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ class Insertion {
} );
}

// TMP this will become a postfixer.
// TMP this will become a post-fixer.
this.schema.removeDisallowedAttributes( this._filterAttributesOf, this.writer );
this._filterAttributesOf = [];
}
Expand Down
235 changes: 235 additions & 0 deletions src/model/utils/selection-post-fixer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
/**
* @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module engine/mode/utils/selection-post-fixer
*/

import Range from '../range';
import Position from '../position';

/**
* Injects selection post-fixer to the model.
*
* The role of the selection post-fixer is to ensure that the selection is in a correct place
* after a {@link module:engine/model/model~Model#change `change()`} block was executed.
*
* The correct position means that:
*
* * All collapsed selection ranges are in a place where the {@link module:engine/model/schema~Schema}
* allows a `$text`.
* * None of selection's non-collapsed ranges crosses a {@link module:engine/model/schema~Schema#isLimit limit element}
* boundary (a range must be rooted within one limit element).
* * Only {@link module:engine/model/schema~Schema#isObject object elements} can be selected from outside
* (e.g. `[<paragraph>foo</paragraph>]` is invalid). This rule applies independently to both selection ends, so this
* selection is correct – `<paragraph>f[oo</paragraph><image></image>]`.
*
* If the position is not correct, the post-fixer will automatically correct it.
*
* ## Fixing a non-collapsed selection
*
* See as an example a selection that starts in a P1 element and ends inside a text of a TD element
* (`[` and `]` are range boundaries and `(l)` denotes element defines as `isLimit=true`):
*
* root
* |- element P1
* | |- "foo" root
* |- element TABLE (l) P1 TABLE P2
* | |- element TR (l) f o[o TR TR b a r
* | | |- element TD (l) TD TD
* | | |- "aaa" a]a a b b b
* | |- element TR (l)
* | | |- element TD (l) ||
* | | |- "bbb" ||
* |- element P2 VV
* | |- "bar"
* root
* P1 TABLE] P2
* f o[o TR TR b a r
* TD TD
* a a a b b b
*
* In the above example, the TABLE, TR and TD are defined as `isLimit=true` in the schema. The range which is not contained within
* a single limit element must be expanded to select the outer most limit element. The range end is inside text node of TD element.
* As TD element is a child of TR element and TABLE elements which both are defined as `isLimit=true` in schema the range must be expanded
* to select whole TABLE element.
*
* **Note** If selection contains multiple ranges the method returns minimal set of ranges that are not intersecting after expanding them
* to select `isLimit=true` elements.
*
* @param {module:engine/model/model~Model} model
*/
export function injectSelectionPostFixer( model ) {
model.document.registerPostFixer( writer => selectionPostFixer( writer, model ) );
}

// The selection post-fixer.
//
// @param {module:engine/model/writer~Writer} writer
// @param {module:engine/model/model~Model} model
function selectionPostFixer( writer, model ) {
const selection = model.document.selection;
const schema = model.schema;

const ranges = [];

let wasFixed = false;

for ( const modelRange of selection.getRanges() ) {
// Go through all ranges in selection and try fixing each of them.
// Those ranges might overlap but will be corrected later.
const correctedRange = tryFixingRange( modelRange, schema );

if ( correctedRange ) {
ranges.push( correctedRange );
wasFixed = true;
} else {
ranges.push( modelRange );
}
}

// If any of ranges were corrected update the selection.
if ( wasFixed ) {
// The above algorithm might create ranges that intersects each other when selection contains more then one range.
// This is case happens mostly on Firefox which creates multiple ranges for selected table.
const combinedRanges = combineOverlapingRanges( ranges );

writer.setSelection( combinedRanges, { backward: selection.isBackward } );
}
}

// Tries fixing a range if it's incorrect.
//
// @param {module:engine/model/range~Range} range
// @param {module:engine/model/schema~Schema} schema
// @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
function tryFixingRange( range, schema ) {
if ( range.isCollapsed ) {
return tryFixingCollapsedRange( range, schema );
}

return tryFixingNonCollpasedRage( range, schema );
}

// Tries to fix collapsed ranges.
//
// * Fixes situation when a range is in a place where $text is not allowed
//
// @param {module:engine/model/range~Range} range Collapsed range to fix.
// @param {module:engine/model/schema~Schema} schema
// @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
function tryFixingCollapsedRange( range, schema ) {
const originalPosition = range.start;

const nearestSelectionRange = schema.getNearestSelectionRange( originalPosition );

// This might be null ie when editor data is empty.
// In such cases there is no need to fix the selection range.
if ( !nearestSelectionRange ) {
return null;
}

const fixedPosition = nearestSelectionRange.start;

// Fixed position is the same as original - no need to return corrected range.
if ( originalPosition.isEqual( fixedPosition ) ) {
return null;
}

// Check single node selection (happens in tables).
if ( fixedPosition.nodeAfter && schema.isLimit( fixedPosition.nodeAfter ) ) {
return new Range( fixedPosition, Position.createAfter( fixedPosition.nodeAfter ) );
}

return new Range( fixedPosition );
}

// Tries to fix a expanded range that overlaps limit nodes.
//
// @param {module:engine/model/range~Range} range Expanded range to fix.
// @param {module:engine/model/schema~Schema} schema
// @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
function tryFixingNonCollpasedRage( range, schema ) {
// No need to check flat ranges as they will not cross node boundary.
if ( range.isFlat ) {
return null;
}

const start = range.start;
const end = range.end;

const updatedStart = expandSelectionOnIsLimitNode( start, schema, 'start' );
const updatedEnd = expandSelectionOnIsLimitNode( end, schema, 'end' );

if ( !start.isEqual( updatedStart ) || !end.isEqual( updatedEnd ) ) {
return new Range( updatedStart, updatedEnd );
}

return null;
}

// Expands selection so it contains whole limit node.
//
// @param {module:engine/model/position~Position} position
// @param {module:engine/model/schema~Schema} schema
// @param {String} expandToDirection Direction of expansion - either 'start' or 'end' of the range.
// @returns {module:engine/model/position~Position}
function expandSelectionOnIsLimitNode( position, schema, expandToDirection ) {
let node = position.parent;
let parent = node;

// Find outer most isLimit block as such blocks might be nested (ie. in tables).
while ( schema.isLimit( parent ) && parent.parent ) {
node = parent;
parent = parent.parent;
}

if ( node === parent ) {
// If there is not is limit block the return original position.
return position;
}

// Depending on direction of expanding selection return position before or after found node.
return expandToDirection === 'start' ? Position.createBefore( node ) : Position.createAfter( node );
}

// Returns minimal set of continuous ranges.
//
// @param {Array.<module:engine/model/range~Range>} ranges
// @returns {Array.<module:engine/model/range~Range>}
function combineOverlapingRanges( ranges ) {
const combinedRanges = [];

// Seed the state.
let previousRange = ranges[ 0 ];
combinedRanges.push( previousRange );

// Go through each ranges and check if it can be merged with previous one.
for ( const range of ranges ) {
// Do not push same ranges (ie might be created in a table).
if ( range.isEqual( previousRange ) ) {
continue;
}

// Merge intersecting range into previous one.
if ( range.isIntersecting( previousRange ) ) {
const newStart = previousRange.start.isBefore( range.start ) ? previousRange.start : range.start;
const newEnd = range.end.isAfter( previousRange.end ) ? range.end : previousRange.end;
const combinedRange = new Range( newStart, newEnd );

// Replace previous range with the combined one.
combinedRanges.splice( combinedRanges.indexOf( previousRange ), 1, combinedRange );

previousRange = combinedRange;

continue;
}

previousRange = range;
combinedRanges.push( range );
}

return combinedRanges;
}
Loading

0 comments on commit 6cf91a1

Please sign in to comment.