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

T/ckeditor5 table/99: Selection post-fixer improvements. #1608

Merged
merged 20 commits into from
Dec 20, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
3b19330
Update selection post-fixer tests to reflect tables structure.
jodator Dec 6, 2018
efa318b
Remove redundant plugins from table manual test.
jodator Dec 6, 2018
ecffa73
Make selection post-fixer should allow selection of an objects.
jodator Dec 6, 2018
6e47945
Add tests for blockQuote in table in selection post-fixer tests.
jodator Dec 7, 2018
ef8352c
Fix requirements checking on general elements in selection post-fixer.
jodator Dec 7, 2018
1d54e74
Add test case for object in object case.
jodator Dec 7, 2018
33aa691
Simplify 'should not fix #4 - object in object' test.
jodator Dec 7, 2018
02ce918
Merge branch 'master' into t/ckeditor5-table/99
jodator Dec 19, 2018
6862a79
Code style changes in selection-post-fixer.
scofalik Dec 20, 2018
d1a522f
Code style changes in selection-post-fixer.
scofalik Dec 20, 2018
92f9a9f
Code style changes in selection-post-fixer.
scofalik Dec 20, 2018
d1debb9
Fix code style because of Github.
jodator Dec 20, 2018
5a055e1
Revert "Code style changes in selection-post-fixer."
jodator Dec 20, 2018
b16b0f0
Revert "Code style changes in selection-post-fixer."
jodator Dec 20, 2018
d964375
Revert "Code style changes in selection-post-fixer."
jodator Dec 20, 2018
9cf3a14
Update comment in tryFixingNonCollapsedRage() of selection-post-fixer.
jodator Dec 20, 2018
9172eee
Remove empty line in selection-post-fixer.
jodator Dec 20, 2018
77d39f2
Code style - remove redundant cast to boolean.
jodator Dec 20, 2018
da417e0
Rename findOuterMostIsLimitAncestor to findOutermostLimitAncestor().
jodator Dec 20, 2018
3398e5d
Update checkSelectionOnNonLimitElements() description.
jodator Dec 20, 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
60 changes: 43 additions & 17 deletions src/model/utils/selection-post-fixer.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ function tryFixingCollapsedRange( range, schema ) {
//
// @param {module:engine/model/range~Range} range Expanded range to fix.
// @param {module:engine/model/schema~Schema} schema

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

// @returns {module:engine/model/range~Range|null} Returns fixed range or null if range is valid.
function tryFixingNonCollapsedRage( range, schema ) {
const start = range.start;
Expand All @@ -183,9 +184,13 @@ function tryFixingNonCollapsedRage( range, schema ) {
// - [<block>foo</block>] -> <block>[foo]</block>
// - [<block>foo]</block> -> <block>[foo]</block>
// - <block>f[oo</block>] -> <block>f[oo]</block>
// - [<block>foo</block><object></object>] -> <block>[foo</block><object></object>]
if ( checkSelectionOnNonLimitElements( start, end, schema ) ) {
const fixedStart = schema.getNearestSelectionRange( start, 'forward' );
const fixedEnd = schema.getNearestSelectionRange( end, 'backward' );
const isStartObject = start.nodeAfter && schema.isObject( start.nodeAfter );
const fixedStart = isStartObject ? null : schema.getNearestSelectionRange( start, 'forward' );
jodator marked this conversation as resolved.
Show resolved Hide resolved

const isEndObject = end.nodeBefore && schema.isObject( end.nodeBefore );
const fixedEnd = isEndObject ? null : schema.getNearestSelectionRange( end, 'backward' );
jodator marked this conversation as resolved.
Show resolved Hide resolved

return new Range( fixedStart ? fixedStart.start : start, fixedEnd ? fixedEnd.start : end );
jodator marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -197,13 +202,23 @@ function tryFixingNonCollapsedRage( range, schema ) {
// At this point we eliminated valid positions on text nodes so if one of range positions is placed inside a limit element
// then the range crossed limit element boundaries and needs to be fixed.
if ( isStartInLimit || isEndInLimit ) {
const bothInSameParent = ( !!start.nodeAfter && !!end.nodeBefore ) && start.nodeAfter === end.nodeBefore;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can omit !!.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I am not sure if the name is correct. It's more that the selection is on the same element, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name is correct but the check is not. Updated to look for parent check.


const expandStart = isStartInLimit && ( !bothInSameParent || !isInObject( start.nodeAfter, schema ) );
const expandEnd = isEndInLimit && ( !bothInSameParent || !isInObject( end.nodeBefore, schema ) );

// Although we've already found limit element on start/end positions we must find the outer-most limit element.
// as limit elements might be nested directly inside (ie table > tableRow > tableCell).
const startPosition = Position._createAt( startLimitElement, 0 );
const endPosition = Position._createAt( endLimitElement, 0 );
let fixedStart = start;
let fixedEnd = end;

if ( expandStart ) {
fixedStart = Position._createBefore( findOuterMostIsLimitAncestor( startLimitElement, schema ) );
}

const fixedStart = isStartInLimit ? expandSelectionOnIsLimitNode( startPosition, schema, 'start' ) : start;
const fixedEnd = isEndInLimit ? expandSelectionOnIsLimitNode( endPosition, schema, 'end' ) : end;
if ( expandEnd ) {
fixedEnd = Position._createAfter( findOuterMostIsLimitAncestor( endLimitElement, schema ) );
}

return new Range( fixedStart, fixedEnd );
}
Expand All @@ -212,34 +227,45 @@ function tryFixingNonCollapsedRage( range, schema ) {
return null;
}

// Expands selection so it contains whole limit node.
// Finds the outer-most ancestor.
//
// @param {module:engine/model/position~Position} position
// @param {module:engine/model/node~Node} startingNode
// @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;
// @returns {module:engine/model/node~Node}
function findOuterMostIsLimitAncestor( startingNode, schema ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call it findOutermostLimitAncestor. The is part makes the name confusing. I know that we are looking for an object that passes isLimit() in Schema but still. Also AFAIR outermost is a one word.

let isLimitNode = startingNode;
let parent = isLimitNode;

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

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

// Checks whether both range ends are placed around non-limit elements.
// Checks whether one of range ends is placed around non-limit elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

ends => boundaries

Copy link
Contributor

Choose a reason for hiding this comment

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

one => any

//
// @param {module:engine/model/position~Position} start
// @param {module:engine/model/position~Position} end
// @param {module:engine/model/schema~Schema} schema
// @returns {Boolean}
function checkSelectionOnNonLimitElements( start, end, schema ) {
const startIsOnBlock = ( start.nodeAfter && !schema.isLimit( start.nodeAfter ) ) || schema.checkChild( start, '$text' );
const endIsOnBlock = ( end.nodeBefore && !schema.isLimit( end.nodeBefore ) ) || schema.checkChild( end, '$text' );

return startIsOnBlock && endIsOnBlock;
// We should fix such selection when one of those nodes needs fixing.
return startIsOnBlock || endIsOnBlock;
}

// Checks if node exists and if it's an object.
//
// @param {module:engine/model/node~Node} node
// @param {module:engine/model/schema~Schema} schema
// @returns {Boolean}
function isInObject( node, schema ) {
return node && schema.isObject( node );
}

Loading