T/ckeditor5 typing/92: Add support for "word" unit in `modifySelection()` helper. #1287

jodator · 2018-02-08T13:13:38Z

Suggested merge commit message (convention)

Feature: Add support for "word" unit in modifySelection() helper.

Additional information

This is a preview PR required for fixes in https://github.com/ckeditor/ckeditor5-typing/issues/92.

…lection()`.

jodator · 2018-02-08T13:16:41Z

I had some troubles with modifing getCorrectPosition() to some sane form.

It does too much now - maybe I should refactor it and extract word logic to other method?

Reinmar · 2018-02-14T16:49:56Z

src/model/utils/modifyselection.js

@@ -13,6 +13,8 @@ import Range from '../range';
 import { isInsideSurrogatePair, isInsideCombinedSymbol } from '@ckeditor/ckeditor5-utils/src/unicode';
 import DocumentSelection from '../documentselection';

+const wordBoundaryCharacters = ' ,.-():\'"';


Is ' a word boundary? What about "it's"?

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.

Yes it is - check the behovior on textarea or in text editor:

It's a trap!

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...

Hmm... we should expand the list of boundaryCharacters with at least: ?|; and probably some more common interpuntion characters.

@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.

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.

Reinmar · 2018-02-14T16:56:08Z

src/model/utils/modifyselection.js

@@ -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.


"a whole word"

So I'd go with:

// current: ' ,.-():\'"'; const wordBoundaryCharacters = ' ,.?!:;"-()';

Reinmar · 2018-02-14T16:58:29Z

src/model/utils/modifyselection.js

@@ -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


I guess that value is of the TreeWalkerValue type

Reinmar · 2018-02-14T16:58:58Z

src/model/utils/modifyselection.js


-		while ( isInsideSurrogatePair( data, offset ) || ( unit == 'character' && isInsideCombinedSymbol( data, offset ) ) ) {
+		while (
+			isInsideSurrogatePair( data, offset ) ||


This could be a separate function.

Reinmar · 2018-02-14T17:00:31Z

tests/model/utils/modifyselection.js

+				test(
+					'expands selection to the word start',
+					'<p>foo bar[b]az</p>',
+					// TODO: '<p>foo [barb]az</p>',


Why this TODO and other TODOs?

I've forgot to remove them while working.

Reinmar · 2018-02-14T17:03:59Z

I was pleased to see a great test suite. As for the code, you may try to improve it a bit – especially when it comes to overly long conditions. But it's completely internal stuff so it can be far from perfection.

Reinmar · 2018-02-14T17:08:43Z

I've tested it manually and it works very well.

coveralls · 2018-02-14T17:12:24Z

Coverage remained the same at 100.0% when pulling e0a0766 on t/ckeditor5-typing/92 into 4b7d435 on master.

…sition()` in modifyselection.

jodator added 4 commits February 7, 2018 16:50

Added: Support for "word" unit in modifyselection() helper.

3e0f1b7

Changed: Treat as whole words text nodes with different attributes.

f91ca83

Tests: add special case for "word" searching.

33219dc

Docs: Update modifyselection() documentation.

e8fb8b0

jodator requested a review from Reinmar February 8, 2018 13:13

Changed: Remove recursive call to getCorrectPosition() in `modifySe…

9ce6455

…lection()`.

jodator force-pushed the t/ckeditor5-typing/92 branch from c651496 to 9ce6455 Compare February 8, 2018 13:14

Reinmar reviewed Feb 14, 2018

View reviewed changes

Reinmar added the status:review- label Feb 14, 2018

Merge branch 'master' into t/ckeditor5-typing/92

dcc625d

jodator and others added 5 commits February 15, 2018 10:00

Remove TODOs from code.

aa1d559

Docs: Cleanup modifyselection()` documentation.

8f77225

Changed: Expand list of suppoprted word-break characters.

3d8223a

Changed: Extracted getCorrectWordBreakPosition() from `getCorrectPo…

74df0ae

…sition()` in modifyselection.

Minor test improvements.

e0a0766

Reinmar removed the status:review- label Feb 15, 2018

jodator mentioned this pull request Feb 15, 2018

Added: Properly discover delete-word keyboard modifier on mac and non-mac computers. ckeditor/ckeditor5-typing#138

Merged

Reinmar merged commit f37a97a into master Feb 16, 2018

Reinmar deleted the t/ckeditor5-typing/92 branch February 16, 2018 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/ckeditor5 typing/92: Add support for "word" unit in `modifySelection()` helper. #1287

T/ckeditor5 typing/92: Add support for "word" unit in `modifySelection()` helper. #1287

jodator commented Feb 8, 2018

jodator commented Feb 8, 2018 •

edited by Reinmar

Loading

Reinmar Feb 14, 2018

jodator Feb 14, 2018

jodator Feb 14, 2018 •

edited

Loading

jodator Feb 14, 2018

Reinmar Feb 15, 2018

Reinmar Feb 15, 2018

Reinmar Feb 14, 2018

jodator Feb 15, 2018

Reinmar Feb 14, 2018

Reinmar Feb 14, 2018

Reinmar Feb 14, 2018

Reinmar Feb 14, 2018

jodator Feb 15, 2018

Reinmar commented Feb 14, 2018

Reinmar commented Feb 14, 2018

coveralls commented Feb 14, 2018 •

edited

Loading

T/ckeditor5 typing/92: Add support for "word" unit in modifySelection() helper. #1287

T/ckeditor5 typing/92: Add support for "word" unit in modifySelection() helper. #1287

Conversation

jodator commented Feb 8, 2018

Suggested merge commit message (convention)

Additional information

jodator commented Feb 8, 2018 • edited by Reinmar Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodator Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Feb 14, 2018

Reinmar commented Feb 14, 2018

coveralls commented Feb 14, 2018 • edited Loading

T/ckeditor5 typing/92: Add support for "word" unit in `modifySelection()` helper. #1287

T/ckeditor5 typing/92: Add support for "word" unit in `modifySelection()` helper. #1287

jodator commented Feb 8, 2018 •

edited by Reinmar

Loading

jodator Feb 14, 2018 •

edited

Loading

coveralls commented Feb 14, 2018 •

edited

Loading