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

T/1436: Fixing selection that cross limit node boundaries. #1450

Merged
merged 12 commits into from
Jul 6, 2018
Merged

T/1436: Fixing selection that cross limit node boundaries. #1450

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8be680d
Added more tests for selection post-fixer.
Reinmar Jun 17, 2018
de7ab75
Other: Update selection post-fixer to cover more cases.
jodator Jun 28, 2018
93b927d
Merge branch 'master' into t/1436
jodator Jun 28, 2018
c744608
Code style: Change the selection post-fixer internals for better read…
jodator Jul 3, 2018
4b381bf
Merge branch 'master' into t/1436
jodator Jul 3, 2018
f16620d
Tests: Restore previous `setSelection()` tests semantics using setDat…
jodator Jul 3, 2018
8ea5fb5
Merge branch 'master' into t/1436
Reinmar Jul 4, 2018
6060d76
Tests: Use parse() method from model dev utils.
jodator Jul 4, 2018
a7b40b7
Tests: Prefer parse() method or direct params instead of mode.change(…
jodator Jul 4, 2018
9a84bb2
Docs: Update schema.getLimitElement() param type.
jodator Jul 4, 2018
f5bffd4
Tests: Fix deletecontent tests.
jodator Jul 4, 2018
2445ef2
Fix: Complex selection brakes when spanning over widget.
jodator Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 90 additions & 9 deletions src/model/utils/selection-post-fixer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ function selectionPostFixer( writer, model ) {
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 );
const combinedRanges = combineOverlappingRanges( ranges );

writer.setSelection( combinedRanges, { backward: selection.isBackward } );
}
Expand All @@ -110,7 +110,7 @@ function tryFixingRange( range, schema ) {
return tryFixingCollapsedRange( range, schema );
}

return tryFixingNonCollpasedRage( range, schema );
return tryFixingNonCollapsedRage( range, schema );
}

// Tries to fix collapsed ranges.
Expand Down Expand Up @@ -146,27 +146,87 @@ function tryFixingCollapsedRange( range, schema ) {
return new Range( fixedPosition );
}

// Tries to fix a expanded range that overlaps limit nodes.
// Tries to fix an expanded range.
//
// @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 ) {
function tryFixingNonCollapsedRage( range, schema ) {
const start = range.start;
const end = range.end;

// Flat range on the same text node is always valid - no need to fix:
Copy link
Member

@Reinmar Reinmar Jun 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this comment isn't entirely correct – I mean the "same" part. What about:

<p>f[oo<inlineWidget/>ba]r</p>

Also, what about:

<p>[foo]</p>

In this case, neither the start's nor end's textNode getter will return something.

And there's one more case which is missed here:

<p>[<inlineWidget/>foo<inlineWidget/>]</p>

You might've had a different goal by adding this condition – filtering some cases which might be broken by the following logic of this function. But, if the logic of such a check isn't "complete" in a bigger picture, a wider context of the entire algorithm, then it may be misleading. So, instead of patching this algorithm for a hidden purpose, let's thing wider.

E.g. what kind of non-collapsed selections don't need to be fixed? Those which meet the following criteria (all):

  • schema.getLimitElement( range.start ) == schema.getLimitElement( range.end ) – it means the selection starts and ends in the same limit element, so it does not cross any limit element boundary. If a range violates this rule, it needs to be extended to cover the lowest common limit element of both its ends (schem.getLimitElement( range )).

    Note: Right now, schema.getLimitElement() works only with entire selections, but it's not hard to "downgrade" it to support single ranges (returns LCA like for the entire selection) and single positions (returns a lowest limit element in which this position is contained) too.

  • schema.checkChild( range.start, '$text' ) && schema.checkChild( range.end, '$text' ) – both its ends are in a place where a text is allowed. They don't need to be placed in/next to a text (<p>[<inlineWidget/>...) – it's enough that a text is allowed there.

    Note: A range containing a block widget will not be rooted in a place where text is allowed, so it will not pass this check. However, it doesn't mean that it's incorrect – I was describing a range which is certainly correct (i.e. a possible early return check), not an algorithm for deciding whether a range is correct or not.

    Therefore, if a range violates this rule, it needs to be checked further:

    • Does it select an object element? If yes, it's ok. If not it needs to be fixed.
    • Hm... and that seems to be all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By structuring the algorithm as I described above, the rules by which it works will be easier to read. Right now, it consists of "let's try fixing this, let's try that" which is harder for me to digest.

// - <limit>f[o]o</limit>
// - <limit>f[oo]</limit>
// - <limit>[fo]o</limit>
if ( range.isFlat && ( start.textNode || end.textNode ) ) {
return null;
}

const start = range.start;
const end = range.end;
// Try to fix selection that is not on limit nodes:
// - [<p>foo</p>] -> <p>[foo]</p>
// - [<p>foo</p><p>bar</p>] -> <p>[foo</p><p>bar]</p>
if ( ( start.nodeAfter && !schema.isLimit( start.nodeAfter ) ) && ( end.nodeBefore && !schema.isLimit( end.nodeBefore ) ) ) {
const fixedStart = schema.getNearestSelectionRange( start, 'forward' );
const fixedEnd = schema.getNearestSelectionRange( end, 'backward' );

// This might be null ie when editor data is empty or selection is already properly set.
// In such cases there is no need to fix the selection range.
if ( fixedStart && fixedStart.start.isEqual( start ) && fixedEnd && fixedEnd.start.isEqual( end ) ) {
return null;
}

return new Range( fixedStart ? fixedStart.start : start, fixedEnd ? fixedEnd.start : end );
}

// This will fix selection on limit elements:
// - <table>[<tableRow><tableCell></tableCell></tableRow>]<table> -> [<table><tableRow><tableCell></tableCell></tableRow><table>]
// - <image>[<caption>xxx</caption>]</image> -> [<image><caption>xxx</caption></image>]
//
// And when selection crosses limit element:
// - <image>[<caption>xx]x</caption></image> -> [<image><caption>xxx</caption></image>]
const updatedStart = expandSelectionOnIsLimitNode( start, schema, 'start' );
const updatedEnd = expandSelectionOnIsLimitNode( end, schema, 'end' );

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

// If the flat range was not fixed at this point the range is valid.
if ( range.isFlat ) {
return null;
}

// Check if selection crosses limit node boundaries.
// - <table> [<table>
// <tableRow> <tableRow>
// <tableCell><p>f[oo</p></tableCell> -> <tableCell><p>foo</p></tableCell>
// <tableCell><p>b]ar</p></tableCell> <tableCell><p>bar</p></tableCell>
// </tableRow> </tableRow>
// </table> </table>]
// -[<table> [<table>
// <tableRow> <tableRow>
// <tableCell><p>fo]o</p></tableCell> -> <tableCell><p>foo</p></tableCell>
// </tableRow> </tableRow>
// </table> </table>]
const startParentLimitNode = findParentLimitNodePosition( start, schema );
const endParentLimitNode = findParentLimitNodePosition( end, schema );

if ( startParentLimitNode || endParentLimitNode ) {
let updatedStart = start;
let updatedEnd = end;

if ( startParentLimitNode ) {
updatedStart = expandSelectionOnIsLimitNode( startParentLimitNode, schema, 'start' );
}

if ( endParentLimitNode ) {
updatedEnd = expandSelectionOnIsLimitNode( endParentLimitNode, schema, 'end' );
}

return new Range( updatedStart, updatedEnd );
}

return null;
}

Expand Down Expand Up @@ -199,7 +259,7 @@ function expandSelectionOnIsLimitNode( position, schema, expandToDirection ) {
//
// @param {Array.<module:engine/model/range~Range>} ranges
// @returns {Array.<module:engine/model/range~Range>}
function combineOverlapingRanges( ranges ) {
function combineOverlappingRanges( ranges ) {
const combinedRanges = [];

// Seed the state.
Expand Down Expand Up @@ -233,3 +293,24 @@ function combineOverlapingRanges( ranges ) {

return combinedRanges;
}

// Goes up to the root trying to find any `isLimit=true` parent elements. Returns null if not found.
//
// @param {module:engine/model/position~Position} position
// @param {module:engine/model/schema~Schema} schema
// @returns {module:engine/model/position~Position|null}
function findParentLimitNodePosition( position, schema ) {
let parent = position.parent;

while ( parent ) {
if ( parent === parent.root ) {
return null;
}

if ( schema.isLimit( parent ) ) {
return Position.createAt( parent );
}

parent = parent.parent;
}
}
18 changes: 9 additions & 9 deletions tests/conversion/downcast-selection-converters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,9 +494,9 @@ describe( 'downcast-selection-converters', () => {

describe( 'table cell selection converter', () => {
beforeEach( () => {
model.schema.register( 'table' );
model.schema.register( 'tr' );
model.schema.register( 'td' );
model.schema.register( 'table', { isLimit: true } );
model.schema.register( 'tr', { isLimit: true } );
model.schema.register( 'td', { isLimit: true } );

model.schema.extend( 'table', { allowIn: '$root' } );
model.schema.extend( 'tr', { allowIn: 'table' } );
Expand All @@ -519,16 +519,16 @@ describe( 'downcast-selection-converters', () => {
}

for ( const range of selection.getRanges() ) {
const node = range.start.nodeAfter;
const node = range.start.parent;

if ( node == range.end.nodeBefore && node instanceof ModelElement && node.name == 'td' ) {
if ( node instanceof ModelElement && node.name == 'td' ) {
conversionApi.consumable.consume( selection, 'selection' );

const viewNode = conversionApi.mapper.toViewElement( node );
conversionApi.writer.addClass( 'selected', viewNode );
}
}
} );
}, { priority: 'high' } );
} );

it( 'should not be used to convert selection that is not on table cell', () => {
Expand All @@ -542,7 +542,7 @@ describe( 'downcast-selection-converters', () => {
it( 'should add a class to the selected table cell', () => {
test(
// table tr#0 |td#0, table tr#0 td#0|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the comments up to date now?

[ [ 0, 0, 0 ], [ 0, 0, 1 ] ],
[ [ 0, 0, 0, 0 ], [ 0, 0, 0, 3 ] ],
'<table><tr><td>foo</td></tr><tr><td>bar</td></tr></table>',
'<table><tr><td class="selected">foo</td></tr><tr><td>bar</td></tr></table>'
);
Expand All @@ -551,9 +551,9 @@ describe( 'downcast-selection-converters', () => {
it( 'should not be used if selection contains more than just a table cell', () => {
test(
// table tr td#1, table tr#2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here too?

[ [ 0, 0, 0, 1 ], [ 0, 0, 2 ] ],
[ [ 0, 0, 0, 1 ], [ 0, 0, 1, 3 ] ],
'<table><tr><td>foo</td><td>bar</td></tr></table>',
'<table><tr><td>f{oo</td><td>bar</td>]</tr></table>'
'[<table><tr><td>foo</td><td>bar</td></tr></table>]'
);
} );
} );
Expand Down
8 changes: 4 additions & 4 deletions tests/model/schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,12 +1123,12 @@ describe( 'Schema', () => {
schema.extend( 'img', { allowAttributes: 'bold' } );
schema.extend( '$text', { allowIn: 'img' } );

setData( model, '[<p>foo<img>xxx</img>bar</p>]' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the semantics of this test. Let's better use parse(), especially that later on we do selection.getRanges() anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not resolved.

setData( model, '<p>[foo<img>xxx</img>bar]</p>' );

const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute );
const sel = new Selection( validRanges );

expect( stringify( root, sel ) ).to.equal( '[<p>foo<img>]xxx[</img>bar</p>]' );
expect( stringify( root, sel ) ).to.equal( '<p>[foo<img>]xxx[</img>bar]</p>' );
} );

it( 'should return three ranges when attribute is not allowed on one element but is allowed on its child', () => {
Expand All @@ -1141,12 +1141,12 @@ describe( 'Schema', () => {
}
} );

setData( model, '[<p>foo<img>xxx</img>bar</p>]' );
setData( model, '<p>[foo<img>xxx</img>bar]</p>' );

const validRanges = schema.getValidRanges( doc.selection.getRanges(), attribute );
const sel = new Selection( validRanges );

expect( stringify( root, sel ) ).to.equal( '[<p>foo]<img>[xxx]</img>[bar</p>]' );
expect( stringify( root, sel ) ).to.equal( '<p>[foo]<img>[xxx]</img>[bar]</p>' );
} );

it( 'should not leak beyond the given ranges', () => {
Expand Down
26 changes: 22 additions & 4 deletions tests/model/utils/deletecontent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ describe( 'DataController utils', () => {
schema.register( 'image', { allowWhere: '$text' } );
schema.register( 'paragraph', { inheritAllFrom: '$block' } );
schema.register( 'heading1', { inheritAllFrom: '$block' } );
schema.register( 'blockWidget' );
schema.register( 'blockWidget', { isLimit: true } );
schema.register( 'restrictedRoot', {
isLimit: true
} );
Expand Down Expand Up @@ -621,7 +621,7 @@ describe( 'DataController utils', () => {
deleteContent( model, selection );

expect( getData( model, { rootName: 'bodyRoot' } ) )
.to.equal( '[<paragraph>x</paragraph>]<paragraph></paragraph><paragraph>z</paragraph>' );
.to.equal( '<paragraph>[x]</paragraph><paragraph></paragraph><paragraph>z</paragraph>' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can change the initial selection too, so this test is less confusing (we're not checking the sel post-fixer here).

} );

it( 'creates a paragraph when text is not allowed (block widget selected)', () => {
Expand All @@ -644,7 +644,16 @@ describe( 'DataController utils', () => {
{ rootName: 'bodyRoot' }
);

deleteContent( model, doc.selection );
model.change( writer => {
// Set selection to[<heading1>yyy</heading1>] in change() block due to selection post-fixer.
const range = new Range(
new Position( doc.getRoot( 'bodyRoot' ), [ 1 ] ),
new Position( doc.getRoot( 'bodyRoot' ), [ 2 ] )
);
writer.setSelection( range );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to set that selection. deleteContent() can work with any selection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it'd be good if you changed it in the setData() call above to something irrelevant to this test to not create a confusion.


deleteContent( model, doc.selection );
} );

expect( getData( model, { rootName: 'bodyRoot' } ) )
.to.equal( '<paragraph>x</paragraph><paragraph>[]</paragraph><paragraph>z</paragraph>' );
Expand All @@ -657,7 +666,16 @@ describe( 'DataController utils', () => {
{ rootName: 'bodyRoot' }
);

deleteContent( model, doc.selection );
model.change( writer => {
// Set selection to[<heading1>yyy</heading1><paragraph>yyy</paragraph>] in change() block due to selection post-fixer.
const range = new Range(
new Position( doc.getRoot( 'bodyRoot' ), [ 1 ] ),
new Position( doc.getRoot( 'bodyRoot' ), [ 3 ] )
);
writer.setSelection( range );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above– no need to set it. Also, it'd be good if you changed it in the setData() call above to something irrelevant to this test to not create a confusion.


deleteContent( model, doc.selection );
} );

expect( getData( model, { rootName: 'bodyRoot' } ) )
.to.equal( '<paragraph>x</paragraph><paragraph>[]</paragraph><paragraph>z</paragraph>' );
Expand Down
2 changes: 1 addition & 1 deletion tests/model/utils/getselectedcontent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ describe( 'DataController utils', () => {

schema.register( 'paragraph', { inheritAllFrom: '$block' } );
schema.register( 'heading1', { inheritAllFrom: '$block' } );
schema.register( 'blockImage' );
schema.register( 'blockImage', { isObject: true } );
schema.register( 'caption' );
schema.register( 'image', { allowWhere: '$text' } );

Expand Down
Loading