This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 40
T/ckeditor5 typing/92: Add support for "word" unit in modifySelection()
helper.
#1287
Merged
Merged
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
3e0f1b7
Added: Support for "word" unit in `modifyselection()` helper.
jodator f91ca83
Changed: Treat as whole words text nodes with different attributes.
jodator 33219dc
Tests: add special case for "word" searching.
jodator e8fb8b0
Docs: Update `modifyselection()` documentation.
jodator 9ce6455
Changed: Remove recursive call to `getCorrectPosition()` in `modifySe…
jodator dcc625d
Merge branch 'master' into t/ckeditor5-typing/92
Reinmar aa1d559
Remove TODOs from code.
jodator 8f77225
Docs: Cleanup `modifyselection()`` documentation.
jodator 3d8223a
Changed: Expand list of suppoprted word-break characters.
jodator 74df0ae
Changed: Extracted `getCorrectWordBreakPosition()` from `getCorrectPo…
jodator e0a0766
Minor test improvements.
Reinmar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ import Range from '../range'; | |
import { isInsideSurrogatePair, isInsideCombinedSymbol } from '@ckeditor/ckeditor5-utils/src/unicode'; | ||
import DocumentSelection from '../documentselection'; | ||
|
||
const wordBoundaryCharacters = ' ,.-():\'"'; | ||
|
||
/** | ||
* Modifies the selection. Currently, the supported modifications are: | ||
* | ||
|
@@ -31,6 +33,7 @@ import DocumentSelection from '../documentselection'; | |
* For example `𨭎` is represented in `String` by `\uD862\uDF4E`. Both `\uD862` and `\uDF4E` do not have any meaning | ||
* outside the pair (are rendered as ? when alone). Position between them would be incorrect. In this case, selection | ||
* extension will include whole "surrogate pair". | ||
* * `'word'` - moves selection by whole word. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "a whole word" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I'd go with: // current: ' ,.-():\'"';
const wordBoundaryCharacters = ' ,.?!:;"-()'; |
||
* | ||
* **Note:** if you extend a forward selection in a backward direction you will in fact shrink it. | ||
* | ||
|
@@ -39,7 +42,7 @@ import DocumentSelection from '../documentselection'; | |
* @param {module:engine/model/selection~Selection} selection The selection to modify. | ||
* @param {Object} [options] | ||
* @param {'forward'|'backward'} [options.direction='forward'] The direction in which the selection should be modified. | ||
* @param {'character'|'codePoint'} [options.unit='character'] The unit by which selection should be modified. | ||
* @param {'character'|'codePoint'|'word'} [options.unit='character'] The unit by which selection should be modified. | ||
*/ | ||
export default function modifySelection( model, selection, options = {} ) { | ||
const schema = model.schema; | ||
|
@@ -79,11 +82,13 @@ export default function modifySelection( model, selection, options = {} ) { | |
} | ||
|
||
// Checks whether the selection can be extended to the the walker's next value (next position). | ||
// @param {{ walker, unit, isForward, schema }} data | ||
// @param {{ item, nextPosition, type}} value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess that |
||
function tryExtendingTo( data, value ) { | ||
// If found text, we can certainly put the focus in it. Let's just find a correct position | ||
// based on the unit. | ||
if ( value.type == 'text' ) { | ||
return getCorrectPosition( data.walker, data.unit ); | ||
return getCorrectPosition( data.walker, data.unit, data.isForward ); | ||
} | ||
|
||
// Entering an element. | ||
|
@@ -117,17 +122,48 @@ function tryExtendingTo( data, value ) { | |
|
||
// Finds a correct position by walking in a text node and checking whether selection can be extended to given position | ||
// or should be extended further. | ||
function getCorrectPosition( walker, unit ) { | ||
const textNode = walker.position.textNode; | ||
// | ||
// @param {module:engine/model/treewalker~TreeWalker} walker | ||
// @param {String} unit The unit by which selection should be modified. | ||
// @param {Boolean} isForward Is the direction in which the selection should be modified is forward. | ||
function getCorrectPosition( walker, unit, isForward ) { | ||
let textNode = walker.position.textNode; | ||
|
||
if ( textNode ) { | ||
const data = textNode.data; | ||
let data = textNode.data; | ||
let offset = walker.position.offset - textNode.startOffset; | ||
let isAtNodeBoundary = offset === ( isForward ? textNode.endOffset : 0 ); | ||
|
||
while ( isInsideSurrogatePair( data, offset ) || ( unit == 'character' && isInsideCombinedSymbol( data, offset ) ) ) { | ||
while ( | ||
isInsideSurrogatePair( data, offset ) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be a separate function. |
||
( unit == 'character' && isInsideCombinedSymbol( data, offset ) ) || | ||
( unit == 'word' && ( !( isAtNodeBoundary || isAtWordBoundary( textNode.data, offset, isForward ) ) ) ) | ||
) { | ||
walker.next(); | ||
|
||
// Check of adjacent text nodes with different attributes (like BOLD). | ||
// Example : 'foofoo []bar<$text bold="true">bar</$text> bazbaz' | ||
// should expand to : 'foofoo [bar<$text bold="true">bar</$text>] bazbaz'. | ||
if ( unit == 'word' && !isAtNodeBoundary ) { | ||
const nextNode = isForward ? walker.position.nodeAfter : walker.position.nodeBefore; | ||
|
||
if ( nextNode ) { | ||
// Check boundary char of an adjacent text node. | ||
const boundaryChar = nextNode.data.charAt( isForward ? 0 : nextNode.data.length - 1 ); | ||
|
||
// Go to the next node if the character at the boundary of that node belongs to the same word. | ||
if ( !wordBoundaryCharacters.includes( boundaryChar ) ) { | ||
// If adjacent text node belongs to the same word go to it & reset values. | ||
walker.next(); | ||
|
||
textNode = walker.position.textNode; | ||
data = textNode.data; | ||
} | ||
} | ||
} | ||
|
||
offset = walker.position.offset - textNode.startOffset; | ||
isAtNodeBoundary = offset === ( isForward ? textNode.endOffset : 0 ); | ||
} | ||
} | ||
|
||
|
@@ -144,3 +180,15 @@ function getSearchRange( start, isForward ) { | |
return new Range( searchEnd, start ); | ||
} | ||
} | ||
|
||
// Checks if selection is on word boundary. | ||
// | ||
// @param {module:engine/view/text~Text} textNode The text node to investigate. | ||
// @param {Number} offset Position offset. | ||
// @param {Boolean} isForward Is the direction in which the selection should be modified is forward. | ||
function isAtWordBoundary( data, offset, isForward ) { | ||
// The offset to check depends on direction. | ||
const offsetToCheck = offset + ( isForward ? 0 : -1 ); | ||
|
||
return wordBoundaryCharacters.includes( data.charAt( offsetToCheck ) ); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
'
a word boundary? What about "it's"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks that it is (as far as I can test it in textarea and Gnome Text editor:
it's a trap
- the text will stop: on spaces and on'
.Also check this docs: https://www.unicode.org/reports/tr29/#Default_Word_Boundaries.
We might think of expanding this list to full unicode support but I've used the most common ones for the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is - check the behovior on textarea or in text editor:
In the above the cursor will stop on spaces and on
'
.Also check this docs: https://www.unicode.org/reports/tr29/#Default_Word_Boundaries.
We might think of expanding this list to full unicode support but I've used the most common ones for the text.
edit: to be precise: https://www.unicode.org/reports/tr29/#Single_Quote
But examples shows that
can't
is one word...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... we should expand the list of
boundaryCharacters
with at least:?|;
and probably some more common interpuntion characters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wwalc told me that we need to differentiate between "it's" (which is made of two words so the caret should stop after "s") and "Peter's" where it's a single word which should be removed at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm joking of course. Since a single quote is popular in English and e.g. in macOS it's not treated as a word boundary, I'd not treat it as such.