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

Commit 6cf91a1

Browse files
authored
Merge pull request #1431 from ckeditor/t/ckeditor5-table/12
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.
2 parents 457afde + 0c10532 commit 6cf91a1

23 files changed

+873
-46
lines changed

CHANGELOG.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ The good news is that the our focus when designing the new API was on developer
100100

101101
* 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))
102102
* Conversion utilities refactor. Closes [#1236](https://github.com/ckeditor/ckeditor5-engine/issues/1236). ([fd128a1](https://github.com/ckeditor/ckeditor5-engine/commit/fd128a1))
103-
* 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))
103+
* 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))
104104
* 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))
105105
* Keep the same marker instance when marker is updated. ([8eba5e9](https://github.com/ckeditor/ckeditor5-engine/commit/8eba5e9))
106106
* 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))

src/controller/editingcontroller.js

+1-5
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,7 @@ import Mapper from '../conversion/mapper';
1313
import DowncastDispatcher from '../conversion/downcastdispatcher';
1414
import { insertText, remove } from '../conversion/downcast-converters';
1515
import { convertSelectionChange } from '../conversion/upcast-selection-converters';
16-
import {
17-
convertRangeSelection,
18-
convertCollapsedSelection,
19-
clearAttributes
20-
} from '../conversion/downcast-selection-converters';
16+
import { clearAttributes, convertCollapsedSelection, convertRangeSelection } from '../conversion/downcast-selection-converters';
2117

2218
import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
2319
import mix from '@ckeditor/ckeditor5-utils/src/mix';

src/dev-utils/model.js

+1-3
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
import RootElement from '../model/rootelement';
1515
import Model from '../model/model';
16-
import Batch from '../model/batch';
1716
import ModelRange from '../model/range';
1817
import ModelPosition from '../model/position';
1918
import ModelSelection from '../model/selection';
@@ -94,7 +93,6 @@ export function setData( model, data, options = {} ) {
9493

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

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

114-
model.enqueueChange( batch, writer => {
112+
model.change( writer => {
115113
// Replace existing model in document by new one.
116114
writer.remove( ModelRange.createIn( modelRoot ) );
117115
writer.insert( modelDocumentFragment, modelRoot );

src/model/document.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ export default class Document {
155155
} );
156156

157157
// Wait for `_change` event from model, which signalizes that outermost change block has finished.
158-
// When this happens, check if there were any changes done on document, and if so, call post fixers,
158+
// When this happens, check if there were any changes done on document, and if so, call post-fixers,
159159
// fire `change` event for features and conversion and then reset the differ.
160160
// Fire `change:data` event when at least one operation or buffered marker changes the data.
161161
this.listenTo( model, '_change', ( evt, writer ) => {

src/model/model.js

+3
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import insertContent from './utils/insertcontent';
2626
import deleteContent from './utils/deletecontent';
2727
import modifySelection from './utils/modifyselection';
2828
import getSelectedContent from './utils/getselectedcontent';
29+
import { injectSelectionPostFixer } from './utils/selection-post-fixer';
2930

3031
/**
3132
* Editor's data model. Read about the model in the
@@ -111,6 +112,8 @@ export default class Model {
111112
this.schema.register( '$marker', {
112113
allowIn: [ '$root', '$block' ]
113114
} );
115+
116+
injectSelectionPostFixer( this );
114117
}
115118

116119
/**

src/model/schema.js

+20-7
Original file line numberDiff line numberDiff line change
@@ -333,18 +333,24 @@ export default class Schema {
333333

334334
/**
335335
* Returns `true` if the given item is defined to be
336-
* a limit element by {@link module:engine/model/schema~SchemaItemDefinition}'s `isLimit` property.
336+
* a limit element by {@link module:engine/model/schema~SchemaItemDefinition}'s `isLimit` or `isObject` property
337+
* (all objects are also limits).
337338
*
338339
* schema.isLimit( 'paragraph' ); // -> false
339340
* schema.isLimit( '$root' ); // -> true
340341
* schema.isLimit( editor.model.document.getRoot() ); // -> true
342+
* schema.isLimit( 'image' ); // -> true
341343
*
342344
* @param {module:engine/model/item~Item|module:engine/model/schema~SchemaContextItem|String} item
343345
*/
344346
isLimit( item ) {
345347
const def = this.getDefinition( item );
346348

347-
return !!( def && def.isLimit );
349+
if ( !def ) {
350+
return false;
351+
}
352+
353+
return !!( def.isLimit || def.isObject );
348354
}
349355

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

745-
// Do not split limit elements and objects.
746-
if ( this.isLimit( parent ) || this.isObject( parent ) ) {
756+
// Do not split limit elements.
757+
if ( this.isLimit( parent ) ) {
747758
return null;
748759
}
749760

@@ -969,7 +980,7 @@ mix( Schema, ObservableMixin );
969980
* * `allowAttributesOf` – a string or an array of strings. Inherit attributes from other items.
970981
* * `inheritTypesFrom` – a string or an array of strings. Inherit `is*` properties of other items.
971982
* * `inheritAllFrom` – a string. A shorthand for `allowContentOf`, `allowWhere`, `allowAttributesOf`, `inheritTypesFrom`.
972-
* * additionall, you can define the following `is*` properties: `isBlock`, `isLimit`, `isObject`. Read about them below.
983+
* * additionally, you can define the following `is*` properties: `isBlock`, `isLimit`, `isObject`. Read about them below.
973984
*
974985
* # The is* properties
975986
*
@@ -982,9 +993,11 @@ mix( Schema, ObservableMixin );
982993
* Most block type items will inherit from `$block` (through `inheritAllFrom`).
983994
* * `isLimit` – can be understood as whether this element should not be split by <kbd>Enter</kbd>.
984995
* Examples of limit elements – `$root`, table cell, image caption, etc. In other words, all actions which happen inside
985-
* a limit element are limitted to its content.
996+
* a limit element are limited to its content. **Note:** all objects (`isObject`) are treated as limit elements too.
986997
* * `isObject` – whether item is "self-contained" and should be treated as a whole. Examples of object elements –
987-
* `image`, `table`, `video`, etc.
998+
* `image`, `table`, `video`, etc. **Note:** an object is also a limit so
999+
* {@link module:engine/model/schema~Schema#isLimit `isLimit()`}
1000+
* returns `true` for object elements automatically.
9881001
*
9891002
* # Generic items
9901003
*

src/model/utils/deletecontent.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ function checkCanBeMerged( leftPos, rightPos, schema ) {
185185
const rangeToCheck = new Range( leftPos, rightPos );
186186

187187
for ( const value of rangeToCheck.getWalker() ) {
188-
if ( schema.isObject( value.item ) || schema.isLimit( value.item ) ) {
188+
if ( schema.isLimit( value.item ) ) {
189189
return false;
190190
}
191191
}

src/model/utils/insertcontent.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ class Insertion {
145145
} );
146146
}
147147

148-
// TMP this will become a postfixer.
148+
// TMP this will become a post-fixer.
149149
this.schema.removeDisallowedAttributes( this._filterAttributesOf, this.writer );
150150
this._filterAttributesOf = [];
151151
}
+235
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
/**
2+
* @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
3+
* For licensing, see LICENSE.md.
4+
*/
5+
6+
/**
7+
* @module engine/mode/utils/selection-post-fixer
8+
*/
9+
10+
import Range from '../range';
11+
import Position from '../position';
12+
13+
/**
14+
* Injects selection post-fixer to the model.
15+
*
16+
* The role of the selection post-fixer is to ensure that the selection is in a correct place
17+
* after a {@link module:engine/model/model~Model#change `change()`} block was executed.
18+
*
19+
* The correct position means that:
20+
*
21+
* * All collapsed selection ranges are in a place where the {@link module:engine/model/schema~Schema}
22+
* allows a `$text`.
23+
* * None of selection's non-collapsed ranges crosses a {@link module:engine/model/schema~Schema#isLimit limit element}
24+
* boundary (a range must be rooted within one limit element).
25+
* * Only {@link module:engine/model/schema~Schema#isObject object elements} can be selected from outside
26+
* (e.g. `[<paragraph>foo</paragraph>]` is invalid). This rule applies independently to both selection ends, so this
27+
* selection is correct – `<paragraph>f[oo</paragraph><image></image>]`.
28+
*
29+
* If the position is not correct, the post-fixer will automatically correct it.
30+
*
31+
* ## Fixing a non-collapsed selection
32+
*
33+
* See as an example a selection that starts in a P1 element and ends inside a text of a TD element
34+
* (`[` and `]` are range boundaries and `(l)` denotes element defines as `isLimit=true`):
35+
*
36+
* root
37+
* |- element P1
38+
* | |- "foo" root
39+
* |- element TABLE (l) P1 TABLE P2
40+
* | |- element TR (l) f o[o TR TR b a r
41+
* | | |- element TD (l) TD TD
42+
* | | |- "aaa" a]a a b b b
43+
* | |- element TR (l)
44+
* | | |- element TD (l) ||
45+
* | | |- "bbb" ||
46+
* |- element P2 VV
47+
* | |- "bar"
48+
* root
49+
* P1 TABLE] P2
50+
* f o[o TR TR b a r
51+
* TD TD
52+
* a a a b b b
53+
*
54+
* In the above example, the TABLE, TR and TD are defined as `isLimit=true` in the schema. The range which is not contained within
55+
* a single limit element must be expanded to select the outer most limit element. The range end is inside text node of TD element.
56+
* 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
57+
* to select whole TABLE element.
58+
*
59+
* **Note** If selection contains multiple ranges the method returns minimal set of ranges that are not intersecting after expanding them
60+
* to select `isLimit=true` elements.
61+
*
62+
* @param {module:engine/model/model~Model} model
63+
*/
64+
export function injectSelectionPostFixer( model ) {
65+
model.document.registerPostFixer( writer => selectionPostFixer( writer, model ) );
66+
}
67+
68+
// The selection post-fixer.
69+
//
70+
// @param {module:engine/model/writer~Writer} writer
71+
// @param {module:engine/model/model~Model} model
72+
function selectionPostFixer( writer, model ) {
73+
const selection = model.document.selection;
74+
const schema = model.schema;
75+
76+
const ranges = [];
77+
78+
let wasFixed = false;
79+
80+
for ( const modelRange of selection.getRanges() ) {
81+
// Go through all ranges in selection and try fixing each of them.
82+
// Those ranges might overlap but will be corrected later.
83+
const correctedRange = tryFixingRange( modelRange, schema );
84+
85+
if ( correctedRange ) {
86+
ranges.push( correctedRange );
87+
wasFixed = true;
88+
} else {
89+
ranges.push( modelRange );
90+
}
91+
}
92+
93+
// If any of ranges were corrected update the selection.
94+
if ( wasFixed ) {
95+
// The above algorithm might create ranges that intersects each other when selection contains more then one range.
96+
// This is case happens mostly on Firefox which creates multiple ranges for selected table.
97+
const combinedRanges = combineOverlapingRanges( ranges );
98+
99+
writer.setSelection( combinedRanges, { backward: selection.isBackward } );
100+
}
101+
}
102+
103+
// Tries fixing a range if it's incorrect.
104+
//
105+
// @param {module:engine/model/range~Range} range
106+
// @param {module:engine/model/schema~Schema} schema
107+
// @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
108+
function tryFixingRange( range, schema ) {
109+
if ( range.isCollapsed ) {
110+
return tryFixingCollapsedRange( range, schema );
111+
}
112+
113+
return tryFixingNonCollpasedRage( range, schema );
114+
}
115+
116+
// Tries to fix collapsed ranges.
117+
//
118+
// * Fixes situation when a range is in a place where $text is not allowed
119+
//
120+
// @param {module:engine/model/range~Range} range Collapsed range to fix.
121+
// @param {module:engine/model/schema~Schema} schema
122+
// @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
123+
function tryFixingCollapsedRange( range, schema ) {
124+
const originalPosition = range.start;
125+
126+
const nearestSelectionRange = schema.getNearestSelectionRange( originalPosition );
127+
128+
// This might be null ie when editor data is empty.
129+
// In such cases there is no need to fix the selection range.
130+
if ( !nearestSelectionRange ) {
131+
return null;
132+
}
133+
134+
const fixedPosition = nearestSelectionRange.start;
135+
136+
// Fixed position is the same as original - no need to return corrected range.
137+
if ( originalPosition.isEqual( fixedPosition ) ) {
138+
return null;
139+
}
140+
141+
// Check single node selection (happens in tables).
142+
if ( fixedPosition.nodeAfter && schema.isLimit( fixedPosition.nodeAfter ) ) {
143+
return new Range( fixedPosition, Position.createAfter( fixedPosition.nodeAfter ) );
144+
}
145+
146+
return new Range( fixedPosition );
147+
}
148+
149+
// Tries to fix a expanded range that overlaps limit nodes.
150+
//
151+
// @param {module:engine/model/range~Range} range Expanded range to fix.
152+
// @param {module:engine/model/schema~Schema} schema
153+
// @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
154+
function tryFixingNonCollpasedRage( range, schema ) {
155+
// No need to check flat ranges as they will not cross node boundary.
156+
if ( range.isFlat ) {
157+
return null;
158+
}
159+
160+
const start = range.start;
161+
const end = range.end;
162+
163+
const updatedStart = expandSelectionOnIsLimitNode( start, schema, 'start' );
164+
const updatedEnd = expandSelectionOnIsLimitNode( end, schema, 'end' );
165+
166+
if ( !start.isEqual( updatedStart ) || !end.isEqual( updatedEnd ) ) {
167+
return new Range( updatedStart, updatedEnd );
168+
}
169+
170+
return null;
171+
}
172+
173+
// Expands selection so it contains whole limit node.
174+
//
175+
// @param {module:engine/model/position~Position} position
176+
// @param {module:engine/model/schema~Schema} schema
177+
// @param {String} expandToDirection Direction of expansion - either 'start' or 'end' of the range.
178+
// @returns {module:engine/model/position~Position}
179+
function expandSelectionOnIsLimitNode( position, schema, expandToDirection ) {
180+
let node = position.parent;
181+
let parent = node;
182+
183+
// Find outer most isLimit block as such blocks might be nested (ie. in tables).
184+
while ( schema.isLimit( parent ) && parent.parent ) {
185+
node = parent;
186+
parent = parent.parent;
187+
}
188+
189+
if ( node === parent ) {
190+
// If there is not is limit block the return original position.
191+
return position;
192+
}
193+
194+
// Depending on direction of expanding selection return position before or after found node.
195+
return expandToDirection === 'start' ? Position.createBefore( node ) : Position.createAfter( node );
196+
}
197+
198+
// Returns minimal set of continuous ranges.
199+
//
200+
// @param {Array.<module:engine/model/range~Range>} ranges
201+
// @returns {Array.<module:engine/model/range~Range>}
202+
function combineOverlapingRanges( ranges ) {
203+
const combinedRanges = [];
204+
205+
// Seed the state.
206+
let previousRange = ranges[ 0 ];
207+
combinedRanges.push( previousRange );
208+
209+
// Go through each ranges and check if it can be merged with previous one.
210+
for ( const range of ranges ) {
211+
// Do not push same ranges (ie might be created in a table).
212+
if ( range.isEqual( previousRange ) ) {
213+
continue;
214+
}
215+
216+
// Merge intersecting range into previous one.
217+
if ( range.isIntersecting( previousRange ) ) {
218+
const newStart = previousRange.start.isBefore( range.start ) ? previousRange.start : range.start;
219+
const newEnd = range.end.isAfter( previousRange.end ) ? range.end : previousRange.end;
220+
const combinedRange = new Range( newStart, newEnd );
221+
222+
// Replace previous range with the combined one.
223+
combinedRanges.splice( combinedRanges.indexOf( previousRange ), 1, combinedRange );
224+
225+
previousRange = combinedRange;
226+
227+
continue;
228+
}
229+
230+
previousRange = range;
231+
combinedRanges.push( range );
232+
}
233+
234+
return combinedRanges;
235+
}

0 commit comments

Comments
 (0)