From 0c1d2e67c11b4e818c6a08f98f62e711095aa9db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Wed, 13 May 2020 20:03:29 +0200 Subject: [PATCH 1/9] Select all in parent limit elements for successive select-all commands. Closes #6621. --- .../src/selectallcommand.js | 17 ++++- .../tests/selectallcommand.js | 64 ++++++++++++++++++- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-select-all/src/selectallcommand.js b/packages/ckeditor5-select-all/src/selectallcommand.js index 392b0a3e902..6d62534a8f7 100644 --- a/packages/ckeditor5-select-all/src/selectallcommand.js +++ b/packages/ckeditor5-select-all/src/selectallcommand.js @@ -21,6 +21,7 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; * * If the selection was anchored in a {@glink framework/guides/tutorials/implementing-a-block-widget nested editable} * (e.g. a caption of an image), the new selection will contain its entire content. + * Successive execution - selecting all if entire content is selected - expands selection to the parent editable. * * @extends module:core/command~Command */ @@ -30,10 +31,22 @@ export default class SelectAllCommand extends Command { */ execute() { const model = this.editor.model; - const limitElement = model.schema.getLimitElement( model.document.selection ); + const selection = model.document.selection; + let limitElement = model.schema.getLimitElement( selection ); + + let place = 'in'; + // If entire element was already selected, try selecting all in a parent limit element (if any). + if ( selection.containsEntireContent( limitElement ) && limitElement.root !== limitElement ) { + do { + if ( limitElement.parent ) { + limitElement = limitElement.parent; + } + } while ( !model.schema.isLimit( limitElement ) ); + place = 'on'; + } model.change( writer => { - writer.setSelection( limitElement, 'in' ); + writer.setSelection( limitElement, place ); } ); } } diff --git a/packages/ckeditor5-select-all/tests/selectallcommand.js b/packages/ckeditor5-select-all/tests/selectallcommand.js index 4dbc09f48f2..18965adad63 100644 --- a/packages/ckeditor5-select-all/tests/selectallcommand.js +++ b/packages/ckeditor5-select-all/tests/selectallcommand.js @@ -70,13 +70,21 @@ describe( 'SelectAllCommand', () => { } ); it( 'should select all (selection in a nested editable)', () => { - setData( model, 'foo[bar]' ); + setData( model, 'foob[ar]' ); editor.execute( 'selectAll' ); expect( getData( model ) ).to.equal( 'foo[bar]' ); } ); + it( 'when entire editable is selected, should select all in parent limit element', () => { + setData( model, 'foo[bar]' ); + + editor.execute( 'selectAll' ); + + expect( getData( model ) ).to.equal( 'foo[bar]' ); + } ); + it( 'should select all in the closest nested editable (nested editable inside another nested editable)', () => { setData( model, 'foo' + @@ -103,5 +111,59 @@ describe( 'SelectAllCommand', () => { '' ); } ); + + it( 'consecutive execute() on nested editable, should select all in the parent limit element', () => { + setData( model, + 'foo' + + '' + + '' + + '' + + 'foo' + + '' + + '' + + '' + + '
b[]ar
' + ); + + editor.execute( 'selectAll' ); + editor.execute( 'selectAll' ); + + expect( getData( model ) ).to.equal( 'foo' + + '' + + '' + + '' + + 'foo' + + '[]' + + '' + + '' + + '
bar
' + ); + + editor.execute( 'selectAll' ); + + expect( getData( model ) ).to.equal( 'foo' + + '' + + '' + + '' + + '[foo' + + ']' + + '' + + '' + + '
bar
' + ); + + editor.execute( 'selectAll' ); + + expect( getData( model ) ).to.equal( 'foo' + + '[' + + '' + + '' + + 'foo' + + '' + + '' + + '' + + '
bar
]' + ); + } ); } ); } ); From f00e28284498e545c82082781ce9ffa7dfb23a4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Thu, 14 May 2020 15:34:56 +0200 Subject: [PATCH 2/9] Remove redundant check for parent element. --- packages/ckeditor5-select-all/src/selectallcommand.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-select-all/src/selectallcommand.js b/packages/ckeditor5-select-all/src/selectallcommand.js index 6d62534a8f7..6ae92ad5a52 100644 --- a/packages/ckeditor5-select-all/src/selectallcommand.js +++ b/packages/ckeditor5-select-all/src/selectallcommand.js @@ -33,14 +33,12 @@ export default class SelectAllCommand extends Command { const model = this.editor.model; const selection = model.document.selection; let limitElement = model.schema.getLimitElement( selection ); - let place = 'in'; // If entire element was already selected, try selecting all in a parent limit element (if any). if ( selection.containsEntireContent( limitElement ) && limitElement.root !== limitElement ) { do { - if ( limitElement.parent ) { - limitElement = limitElement.parent; - } + // Eventually, the $root (limitElement.root === limitElement) will be a limit. + limitElement = limitElement.parent; } while ( !model.schema.isLimit( limitElement ) ); place = 'on'; } From bd301c40911c6fa088fd7c77dfcc89e798b3fab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Sun, 24 May 2020 20:45:45 +0200 Subject: [PATCH 3/9] Select-all only in limit elements which accept text and paragraphs, as suggested in reviews https://github.com/ckeditor/ckeditor5/pull/6816#discussion_r425709607 and https://github.com/ckeditor/ckeditor5/pull/6816#pullrequestreview-412449415 . Closes #6621. --- .../src/selectallcommand.js | 31 +++++--- .../tests/selectallcommand.js | 76 ++++++++++++++++--- 2 files changed, 87 insertions(+), 20 deletions(-) diff --git a/packages/ckeditor5-select-all/src/selectallcommand.js b/packages/ckeditor5-select-all/src/selectallcommand.js index 6ae92ad5a52..e361a9cd445 100644 --- a/packages/ckeditor5-select-all/src/selectallcommand.js +++ b/packages/ckeditor5-select-all/src/selectallcommand.js @@ -32,19 +32,32 @@ export default class SelectAllCommand extends Command { execute() { const model = this.editor.model; const selection = model.document.selection; - let limitElement = model.schema.getLimitElement( selection ); - let place = 'in'; - // If entire element was already selected, try selecting all in a parent limit element (if any). - if ( selection.containsEntireContent( limitElement ) && limitElement.root !== limitElement ) { + let scopeElement = model.schema.getLimitElement( selection ); + + // If an entire scope is selected, or the selection's ancestor is not a scope yet, + // browse through ancestors to find the enclosing parent scope. + if ( selection.containsEntireContent( scopeElement ) || !isSelectAllScope( model.schema, scopeElement ) ) { do { - // Eventually, the $root (limitElement.root === limitElement) will be a limit. - limitElement = limitElement.parent; - } while ( !model.schema.isLimit( limitElement ) ); - place = 'on'; + scopeElement = scopeElement.parent; + // Do nothing, if the entire `root` is already selected. + if ( !scopeElement ) { + return; + } + } while ( !isSelectAllScope( model.schema, scopeElement ) ); } model.change( writer => { - writer.setSelection( limitElement, place ); + writer.setSelection( scopeElement, 'in' ); } ); } } + +// Checks whether the element is a valid select-all scope. +// Returns true, it the element {@link module:engine/model/schema~Schema#isLimit `isLimit()`}, +// and can contain any text or paragraph. +// @param {module:engine/model/schema~Schema} schema The schema to cheng against. +// @param {module:engine/model/element~Element} element +// @return {Boolean} +function isSelectAllScope( schema, element ) { + return schema.isLimit( element ) && ( schema.checkChild( element, '$text' ) || schema.checkChild( element, 'paragraph' ) ); +} diff --git a/packages/ckeditor5-select-all/tests/selectallcommand.js b/packages/ckeditor5-select-all/tests/selectallcommand.js index 18965adad63..46118ec8367 100644 --- a/packages/ckeditor5-select-all/tests/selectallcommand.js +++ b/packages/ckeditor5-select-all/tests/selectallcommand.js @@ -77,12 +77,50 @@ describe( 'SelectAllCommand', () => { expect( getData( model ) ).to.equal( 'foo[bar]' ); } ); - it( 'when entire editable is selected, should select all in parent limit element', () => { + it( 'should select all (within limit element selected)', () => { + setData( model, + 'foo' + + '' + + '' + + '' + + 'foo' + + '' + + '[' + + 'bar' + + ']' + + '[' + + 'baz' + + ']' + + '' + + '
' + ); + + editor.execute( 'selectAll' ); + + expect( getData( model ) ).to.equal( + '[foo' + + '' + + '' + + '' + + 'foo' + + '' + + '' + + 'bar' + + '' + + '' + + 'baz' + + '' + + '' + + '
]' + ); + } ); + + it( 'when entire editable is selected, should select all in parent select-all-limit element', () => { setData( model, 'foo[bar]' ); editor.execute( 'selectAll' ); - expect( getData( model ) ).to.equal( 'foo[bar]' ); + expect( getData( model ) ).to.equal( '[foobar]' ); } ); it( 'should select all in the closest nested editable (nested editable inside another nested editable)', () => { @@ -112,7 +150,7 @@ describe( 'SelectAllCommand', () => { ); } ); - it( 'consecutive execute() on nested editable, should select all in the parent limit element', () => { + it( 'consecutive execute() on nested editable, should select all in the parent sellect-all-limit element', () => { setData( model, 'foo' + '' + @@ -132,8 +170,8 @@ describe( 'SelectAllCommand', () => { '
' + '' + '' + - 'foo' + - '[]' + + '[foo' + + ']' + '' + '' + '
barbar
' @@ -141,21 +179,37 @@ describe( 'SelectAllCommand', () => { editor.execute( 'selectAll' ); - expect( getData( model ) ).to.equal( 'foo' + + expect( getData( model ) ).to.equal( '[foo' + '' + '' + '' + - '[foo' + - ']' + + 'foo' + + '' + '' + '' + - '
barbar
' + ']' + ); + + // editor.execute( 'selectAll' ); + } ); + + it( 'when entire editor is selected, should not change the selection', () => { + setData( model, + '[foo' + + '' + + '' + + '' + + 'foo' + + '' + + '' + + '' + + '
bar
]' ); editor.execute( 'selectAll' ); - expect( getData( model ) ).to.equal( 'foo' + - '[' + + expect( getData( model ) ).to.equal( '[foo' + + '
' + '' + '' + 'foo' + From 85c6777ad34c5d2f4b0e7efb032e36e79ffe9eb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Sun, 24 May 2020 20:50:59 +0200 Subject: [PATCH 4/9] Rephrase tests' descriptions to match the existing convention. --- .../tests/selectallcommand.js | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-select-all/tests/selectallcommand.js b/packages/ckeditor5-select-all/tests/selectallcommand.js index 46118ec8367..bb70a4317c5 100644 --- a/packages/ckeditor5-select-all/tests/selectallcommand.js +++ b/packages/ckeditor5-select-all/tests/selectallcommand.js @@ -77,7 +77,7 @@ describe( 'SelectAllCommand', () => { expect( getData( model ) ).to.equal( 'foo' ); } ); - it( 'should select all (within limit element selected)', () => { + it( 'should select all (selection within limit element)', () => { setData( model, 'foo' + '
[bar]
' + @@ -115,14 +115,6 @@ describe( 'SelectAllCommand', () => { ); } ); - it( 'when entire editable is selected, should select all in parent select-all-limit element', () => { - setData( model, 'foo' ); - - editor.execute( 'selectAll' ); - - expect( getData( model ) ).to.equal( '[foo]' ); - } ); - it( 'should select all in the closest nested editable (nested editable inside another nested editable)', () => { setData( model, 'foo' + @@ -150,7 +142,15 @@ describe( 'SelectAllCommand', () => { ); } ); - it( 'consecutive execute() on nested editable, should select all in the parent sellect-all-limit element', () => { + it( 'should select all in the parent select-all-limit element (the entire editable is selected)', () => { + setData( model, 'foo' ); + + editor.execute( 'selectAll' ); + + expect( getData( model ) ).to.equal( '[foo]' ); + } ); + + it( 'should select all in the parent sellect-all-limit element (consecutive execute() on a nested editable)', () => { setData( model, 'foo' + '
[bar]bar[bar]bar
' + @@ -193,7 +193,7 @@ describe( 'SelectAllCommand', () => { // editor.execute( 'selectAll' ); } ); - it( 'when entire editor is selected, should not change the selection', () => { + it( 'should not change the selection (the entire editor is selected)', () => { setData( model, '[foo' + '
' + From 57f2e2588e3abdb6bb558d06f85563bf996f5304 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 26 May 2020 14:17:04 +0200 Subject: [PATCH 5/9] Docs improvements and code refactoring. --- packages/ckeditor5-select-all/src/selectallcommand.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-select-all/src/selectallcommand.js b/packages/ckeditor5-select-all/src/selectallcommand.js index e361a9cd445..884b841fd9a 100644 --- a/packages/ckeditor5-select-all/src/selectallcommand.js +++ b/packages/ckeditor5-select-all/src/selectallcommand.js @@ -39,6 +39,7 @@ export default class SelectAllCommand extends Command { if ( selection.containsEntireContent( scopeElement ) || !isSelectAllScope( model.schema, scopeElement ) ) { do { scopeElement = scopeElement.parent; + // Do nothing, if the entire `root` is already selected. if ( !scopeElement ) { return; @@ -53,9 +54,10 @@ export default class SelectAllCommand extends Command { } // Checks whether the element is a valid select-all scope. -// Returns true, it the element {@link module:engine/model/schema~Schema#isLimit `isLimit()`}, +// Returns true, if the element is a {@link module:engine/model/schema~Schema#isLimit limit}, // and can contain any text or paragraph. -// @param {module:engine/model/schema~Schema} schema The schema to cheng against. +// +// @param {module:engine/model/schema~Schema} schema The schema to check against. // @param {module:engine/model/element~Element} element // @return {Boolean} function isSelectAllScope( schema, element ) { From 981fdc246f516e8d789b987f6f98bd20f59631bc Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 26 May 2020 14:22:37 +0200 Subject: [PATCH 6/9] Docs: Improved the documentation of the SelectAllCommand class. --- packages/ckeditor5-select-all/src/selectallcommand.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-select-all/src/selectallcommand.js b/packages/ckeditor5-select-all/src/selectallcommand.js index 884b841fd9a..eac8f25783e 100644 --- a/packages/ckeditor5-select-all/src/selectallcommand.js +++ b/packages/ckeditor5-select-all/src/selectallcommand.js @@ -20,8 +20,8 @@ import Command from '@ckeditor/ckeditor5-core/src/command'; * {@link module:engine/model/selection~Selection#anchor anchored} in. * * If the selection was anchored in a {@glink framework/guides/tutorials/implementing-a-block-widget nested editable} - * (e.g. a caption of an image), the new selection will contain its entire content. - * Successive execution - selecting all if entire content is selected - expands selection to the parent editable. + * (e.g. a caption of an image), the new selection will contain its entire content. Successive executions of this command + * will expand the selection to encompass more and more content up to the entire editable root of the editor. * * @extends module:core/command~Command */ From 24a4b685b9e97c1a9fe6563623c0bc73cbaa8729 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Wed, 27 May 2020 16:30:50 +0200 Subject: [PATCH 7/9] Remove an obsolete comment, as suggested at https://github.com/ckeditor/ckeditor5/pull/6816#pullrequestreview-418279184 --- packages/ckeditor5-select-all/tests/selectallcommand.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/ckeditor5-select-all/tests/selectallcommand.js b/packages/ckeditor5-select-all/tests/selectallcommand.js index bb70a4317c5..7c037cb1a2c 100644 --- a/packages/ckeditor5-select-all/tests/selectallcommand.js +++ b/packages/ckeditor5-select-all/tests/selectallcommand.js @@ -189,8 +189,6 @@ describe( 'SelectAllCommand', () => { '' + '
]' ); - - // editor.execute( 'selectAll' ); } ); it( 'should not change the selection (the entire editor is selected)', () => { From a2a44eb5b0b2de0b272ba01833357ba633b1ce35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Wed, 27 May 2020 16:39:16 +0200 Subject: [PATCH 8/9] Describe successive select-all behavior in the feature docs, as suggested at https://github.com/ckeditor/ckeditor5/pull/6816#pullrequestreview-418243766. --- .../docs/_snippets/features/select-all.html | 25 ++++++++++++++----- .../docs/features/select-all.md | 12 ++++++++- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html b/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html index b9220f5056e..9d073fb2a53 100644 --- a/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html +++ b/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html @@ -1,13 +1,26 @@

The three greatest things you learn from traveling

-
A lone wanderer looking at Mount Bromo volcano in Indonesia. -
Leaving your comfort zone might lead you to such beautiful sceneries like this one.
-
-

Like all the great things on earth traveling teaches us by example. Here are some of the most precious lessons I’ve learned over the years of traveling.

-

Appreciation of diversity

+
+ + + + + + + +
+

Appreciation of diversity

-

Getting used to an entirely different culture can be challenging. While it’s also nice to learn about cultures online or from books, nothing comes close to experiencing cultural diversity in person. You learn to appreciate each and every single one of the differences while you become more culturally fluid.

+

Getting used to an entirely different culture can be challenging. While it’s also nice to learn about cultures online or from books, nothing comes close to experiencing cultural diversity in person. You learn to appreciate each and every single one of the differences while you become more culturally fluid.

+ 		+

Leaving your comfort zone might lead you to such beautiful sceneries like this one.

+
+ A lone wanderer looking at Mount Bromo volcano in Indonesia. +
Mount Bromo, Indonesia.
+
+
+
diff --git a/packages/ckeditor5-select-all/docs/features/select-all.md b/packages/ckeditor5-select-all/docs/features/select-all.md index 1de2346c648..5a48a90f6f9 100644 --- a/packages/ckeditor5-select-all/docs/features/select-all.md +++ b/packages/ckeditor5-select-all/docs/features/select-all.md @@ -9,10 +9,20 @@ The {@link module:select-all/selectall~SelectAll} feature allows selecting the e ## Demo -Press Ctrl/⌘+A or use the toolbar button to select the entire content of the editor. Note that when editing an {@link features/image#image-captions image caption}, the selection will only expand to the boundaries of the caption. +Press Ctrl/⌘+A or use the toolbar button to select the entire content of the editor. {@snippet features/select-all} +### Boundaries + +Note that when editing an {@link features/image#image-captions image caption}, the selection will only expand to the boundaries of the caption. Successive use will expand the selection beyond these boundaries to encompass more and more content up to the entire editable root of the editor. This is to allow the user to do more focused editing and select only related content. + + + The Select All boundaries are created by any {@link framework/guides/deep-dive/schema#limit-elements limit element} which can contain a text or paragraph. + + + + ## Installation From a1ee014b29606cfd4498a28095ec6f20f5d69860 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 28 May 2020 10:20:32 +0200 Subject: [PATCH 9/9] Docs: Made the guide easier to understand for non-techies. Reverted the demo content so it resembles something that users may actually create in real-life. --- .../docs/_snippets/features/select-all.html | 25 +++++-------------- .../docs/features/select-all.md | 8 +----- 2 files changed, 7 insertions(+), 26 deletions(-) diff --git a/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html b/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html index 9d073fb2a53..b9220f5056e 100644 --- a/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html +++ b/packages/ckeditor5-select-all/docs/_snippets/features/select-all.html @@ -1,26 +1,13 @@

The three greatest things you learn from traveling

+
A lone wanderer looking at Mount Bromo volcano in Indonesia. +
Leaving your comfort zone might lead you to such beautiful sceneries like this one.
+
+

Like all the great things on earth traveling teaches us by example. Here are some of the most precious lessons I’ve learned over the years of traveling.

-
- - - - - - - -
-

Appreciation of diversity

+

Appreciation of diversity

-

Getting used to an entirely different culture can be challenging. While it’s also nice to learn about cultures online or from books, nothing comes close to experiencing cultural diversity in person. You learn to appreciate each and every single one of the differences while you become more culturally fluid.

- 		-

Leaving your comfort zone might lead you to such beautiful sceneries like this one.

-
- A lone wanderer looking at Mount Bromo volcano in Indonesia. -
Mount Bromo, Indonesia.
-
-
-
+

Getting used to an entirely different culture can be challenging. While it’s also nice to learn about cultures online or from books, nothing comes close to experiencing cultural diversity in person. You learn to appreciate each and every single one of the differences while you become more culturally fluid.

diff --git a/packages/ckeditor5-select-all/docs/features/select-all.md b/packages/ckeditor5-select-all/docs/features/select-all.md index 5a48a90f6f9..2d624f8de07 100644 --- a/packages/ckeditor5-select-all/docs/features/select-all.md +++ b/packages/ckeditor5-select-all/docs/features/select-all.md @@ -13,16 +13,10 @@ Press Ctrl/⌘+A or use the toolbar button to select the e {@snippet features/select-all} -### Boundaries - -Note that when editing an {@link features/image#image-captions image caption}, the selection will only expand to the boundaries of the caption. Successive use will expand the selection beyond these boundaries to encompass more and more content up to the entire editable root of the editor. This is to allow the user to do more focused editing and select only related content. - - The Select All boundaries are created by any {@link framework/guides/deep-dive/schema#limit-elements limit element} which can contain a text or paragraph. + When the selection is inside the {@link features/image#image-captions image caption}, it will only expand to the boundaries of the caption. Use the keystroke or the toolbar button again to include more content until the entire content of the editor is selected. The same rule applies, for instance, when the selection is inside a table cell or any self–contained (nested) editable region in the content. - - ## Installation