From 05b414e9f29a9d2df1f4c0a12601a03c6d92f758 Mon Sep 17 00:00:00 2001 From: yanasang Date: Fri, 31 Jul 2020 13:28:41 -0400 Subject: [PATCH 1/5] Workaround for isCollapsed bug with shadow root selections --- packages/ckeditor5-engine/src/view/domconverter.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/view/domconverter.js b/packages/ckeditor5-engine/src/view/domconverter.js index 4beac20d1ac..e97c81ac688 100644 --- a/packages/ckeditor5-engine/src/view/domconverter.js +++ b/packages/ckeditor5-engine/src/view/domconverter.js @@ -850,7 +850,9 @@ export default class DomConverter { * @returns {Boolean} */ isDomSelectionBackward( selection ) { - if ( selection.isCollapsed ) { + // Due to a bug with .isCollapsed always returning true when the selection is from a shadowRoot, also check that the selection + // has no ranges or its range is collapsed. + if ( selection.isCollapsed && ( selection.rangeCount === 0 || selection.getRangeAt( 0 ).collapsed ) ) { return false; } From 4f835fbddcbde65cddeb1e9a24d04d9c0519b4fe Mon Sep 17 00:00:00 2001 From: yanasang Date: Fri, 31 Jul 2020 17:17:12 -0400 Subject: [PATCH 2/5] Pass source element root through to view --- packages/ckeditor5-core/src/editor/editor.js | 3 ++- .../src/controller/editingcontroller.js | 4 ++-- .../src/view/observer/selectionobserver.js | 12 ++++++++++-- packages/ckeditor5-engine/src/view/view.js | 8 ++++---- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/ckeditor5-core/src/editor/editor.js b/packages/ckeditor5-core/src/editor/editor.js index 95e82222709..34231349c3e 100644 --- a/packages/ckeditor5-core/src/editor/editor.js +++ b/packages/ckeditor5-core/src/editor/editor.js @@ -182,7 +182,8 @@ export default class Editor { * @readonly * @member {module:engine/controller/editingcontroller~EditingController} */ - this.editing = new EditingController( this.model, stylesProcessor ); + this.sourceElementRoot = config.sourceElementRoot ? config.sourceElementRoot : document; // eslint-disable-line no-undef + this.editing = new EditingController( this.model, stylesProcessor, this.sourceElementRoot ); this.editing.view.document.bind( 'isReadOnly' ).to( this ); /** diff --git a/packages/ckeditor5-engine/src/controller/editingcontroller.js b/packages/ckeditor5-engine/src/controller/editingcontroller.js index f0d3f8caacd..a4072a42585 100644 --- a/packages/ckeditor5-engine/src/controller/editingcontroller.js +++ b/packages/ckeditor5-engine/src/controller/editingcontroller.js @@ -33,7 +33,7 @@ export default class EditingController { * @param {module:engine/model/model~Model} model Editing model. * @param {module:engine/view/stylesmap~StylesProcessor} stylesProcessor The styles processor instance. */ - constructor( model, stylesProcessor ) { + constructor( model, stylesProcessor, sourceElementRoot = document ) { // eslint-disable-line no-undef /** * Editor model. * @@ -48,7 +48,7 @@ export default class EditingController { * @readonly * @member {module:engine/view/view~View} */ - this.view = new View( stylesProcessor ); + this.view = new View( stylesProcessor, sourceElementRoot ); /** * Mapper which describes the model-view binding. diff --git a/packages/ckeditor5-engine/src/view/observer/selectionobserver.js b/packages/ckeditor5-engine/src/view/observer/selectionobserver.js index 4b3193c7762..a2c084c1224 100644 --- a/packages/ckeditor5-engine/src/view/observer/selectionobserver.js +++ b/packages/ckeditor5-engine/src/view/observer/selectionobserver.js @@ -26,7 +26,7 @@ import { debounce } from 'lodash-es'; * @extends module:engine/view/observer/observer~Observer */ export default class SelectionObserver extends Observer { - constructor( view ) { + constructor( view, sourceElementRoot = document ) { // eslint-disable-line no-undef super( view ); /** @@ -87,6 +87,8 @@ export default class SelectionObserver extends Observer { * @member {Number} module:engine/view/observer/selectionobserver~SelectionObserver#_loopbackCounter */ this._loopbackCounter = 0; + + this.sourceElementRoot = sourceElementRoot; } /** @@ -135,7 +137,13 @@ export default class SelectionObserver extends Observer { // If there were mutations then the view will be re-rendered by the mutation observer and selection // will be updated, so selections will equal and event will not be fired, as expected. - const domSelection = domDocument.defaultView.getSelection(); + // const domSelection = domDocument.defaultView.getSelection(); + let domSelection; + if ( this.sourceElementRoot === domDocument ) { + domSelection = domDocument.defaultView.getSelection(); + } else { + domSelection = this.sourceElementRoot.getSelection(); + } const newViewSelection = this.domConverter.domSelectionToView( domSelection ); // Do not convert selection change if the new view selection has no ranges in it. diff --git a/packages/ckeditor5-engine/src/view/view.js b/packages/ckeditor5-engine/src/view/view.js index 15fdcb4961c..90cb21605e1 100644 --- a/packages/ckeditor5-engine/src/view/view.js +++ b/packages/ckeditor5-engine/src/view/view.js @@ -65,7 +65,7 @@ export default class View { /** * @param {module:engine/view/stylesmap~StylesProcessor} stylesProcessor The styles processor instance. */ - constructor( stylesProcessor ) { + constructor( stylesProcessor, sourceElementRoot = document ) { // eslint-disable-line no-undef /** * Instance of the {@link module:engine/view/document~Document} associated with this view controller. * @@ -179,7 +179,7 @@ export default class View { // Add default observers. this.addObserver( MutationObserver ); - this.addObserver( SelectionObserver ); + this.addObserver( SelectionObserver, sourceElementRoot ); this.addObserver( FocusObserver ); this.addObserver( KeyObserver ); this.addObserver( FakeSelectionObserver ); @@ -333,14 +333,14 @@ export default class View { * Should create an instance inheriting from {@link module:engine/view/observer/observer~Observer}. * @returns {module:engine/view/observer/observer~Observer} Added observer instance. */ - addObserver( Observer ) { + addObserver( Observer, sourceElementRoot ) { let observer = this._observers.get( Observer ); if ( observer ) { return observer; } - observer = new Observer( this ); + observer = new Observer( this, sourceElementRoot ); this._observers.set( Observer, observer ); From 639d23ee29720a4bc843502519c099b53395227c Mon Sep 17 00:00:00 2001 From: yanasang Date: Fri, 31 Jul 2020 17:41:24 -0400 Subject: [PATCH 3/5] Don't use property for sourceElementRoot --- packages/ckeditor5-core/src/editor/editor.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-core/src/editor/editor.js b/packages/ckeditor5-core/src/editor/editor.js index 34231349c3e..3786896bf87 100644 --- a/packages/ckeditor5-core/src/editor/editor.js +++ b/packages/ckeditor5-core/src/editor/editor.js @@ -182,8 +182,8 @@ export default class Editor { * @readonly * @member {module:engine/controller/editingcontroller~EditingController} */ - this.sourceElementRoot = config.sourceElementRoot ? config.sourceElementRoot : document; // eslint-disable-line no-undef - this.editing = new EditingController( this.model, stylesProcessor, this.sourceElementRoot ); + const sourceElementRoot = config.sourceElementRoot ? config.sourceElementRoot : document; // eslint-disable-line no-undef + this.editing = new EditingController( this.model, stylesProcessor, sourceElementRoot ); this.editing.view.document.bind( 'isReadOnly' ).to( this ); /** From 9d5c1a3e103ccb64b89e986f20d673eaae0ade29 Mon Sep 17 00:00:00 2001 From: yanasang Date: Mon, 10 Aug 2020 10:44:39 -0400 Subject: [PATCH 4/5] PR changes --- packages/ckeditor5-core/src/editor/editor.js | 5 ++++- .../ckeditor5-engine/src/view/observer/selectionobserver.js | 5 ++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-core/src/editor/editor.js b/packages/ckeditor5-core/src/editor/editor.js index 3786896bf87..6aaef81f485 100644 --- a/packages/ckeditor5-core/src/editor/editor.js +++ b/packages/ckeditor5-core/src/editor/editor.js @@ -7,6 +7,8 @@ * @module core/editor/editor */ +/* global document */ + import Context from '../context'; import Config from '@ckeditor/ckeditor5-utils/src/config'; import EditingController from '@ckeditor/ckeditor5-engine/src/controller/editingcontroller'; @@ -175,6 +177,8 @@ export default class Editor { */ this.data = new DataController( this.model, stylesProcessor ); + const sourceElementRoot = config.sourceElementRoot ? config.sourceElementRoot : document; + /** * The {@link module:engine/controller/editingcontroller~EditingController editing controller}. * Controls user input and rendering the content for editing. @@ -182,7 +186,6 @@ export default class Editor { * @readonly * @member {module:engine/controller/editingcontroller~EditingController} */ - const sourceElementRoot = config.sourceElementRoot ? config.sourceElementRoot : document; // eslint-disable-line no-undef this.editing = new EditingController( this.model, stylesProcessor, sourceElementRoot ); this.editing.view.document.bind( 'isReadOnly' ).to( this ); diff --git a/packages/ckeditor5-engine/src/view/observer/selectionobserver.js b/packages/ckeditor5-engine/src/view/observer/selectionobserver.js index a2c084c1224..636a0efb2a1 100644 --- a/packages/ckeditor5-engine/src/view/observer/selectionobserver.js +++ b/packages/ckeditor5-engine/src/view/observer/selectionobserver.js @@ -7,7 +7,7 @@ * @module engine/view/observer/selectionobserver */ -/* global setInterval, clearInterval */ +/* global setInterval, clearInterval, document */ import Observer from './observer'; import MutationObserver from './mutationobserver'; @@ -26,7 +26,7 @@ import { debounce } from 'lodash-es'; * @extends module:engine/view/observer/observer~Observer */ export default class SelectionObserver extends Observer { - constructor( view, sourceElementRoot = document ) { // eslint-disable-line no-undef + constructor( view, sourceElementRoot = document ) { super( view ); /** @@ -137,7 +137,6 @@ export default class SelectionObserver extends Observer { // If there were mutations then the view will be re-rendered by the mutation observer and selection // will be updated, so selections will equal and event will not be fired, as expected. - // const domSelection = domDocument.defaultView.getSelection(); let domSelection; if ( this.sourceElementRoot === domDocument ) { domSelection = domDocument.defaultView.getSelection(); From 113e8b01dd7937a33274039cfb860181bb3da227 Mon Sep 17 00:00:00 2001 From: yanasang Date: Fri, 14 Aug 2020 16:59:01 -0400 Subject: [PATCH 5/5] Add comments, remove eslint disable --- packages/ckeditor5-engine/src/controller/editingcontroller.js | 4 +++- packages/ckeditor5-engine/src/view/domconverter.js | 2 +- packages/ckeditor5-engine/src/view/view.js | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-engine/src/controller/editingcontroller.js b/packages/ckeditor5-engine/src/controller/editingcontroller.js index a4072a42585..ec9b0b1ec11 100644 --- a/packages/ckeditor5-engine/src/controller/editingcontroller.js +++ b/packages/ckeditor5-engine/src/controller/editingcontroller.js @@ -3,6 +3,8 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/* global document */ + /** * @module engine/controller/editingcontroller */ @@ -33,7 +35,7 @@ export default class EditingController { * @param {module:engine/model/model~Model} model Editing model. * @param {module:engine/view/stylesmap~StylesProcessor} stylesProcessor The styles processor instance. */ - constructor( model, stylesProcessor, sourceElementRoot = document ) { // eslint-disable-line no-undef + constructor( model, stylesProcessor, sourceElementRoot = document ) { /** * Editor model. * diff --git a/packages/ckeditor5-engine/src/view/domconverter.js b/packages/ckeditor5-engine/src/view/domconverter.js index e97c81ac688..8f0acdd016f 100644 --- a/packages/ckeditor5-engine/src/view/domconverter.js +++ b/packages/ckeditor5-engine/src/view/domconverter.js @@ -851,7 +851,7 @@ export default class DomConverter { */ isDomSelectionBackward( selection ) { // Due to a bug with .isCollapsed always returning true when the selection is from a shadowRoot, also check that the selection - // has no ranges or its range is collapsed. + // has no ranges or its range is collapsed. More info: https://bugs.chromium.org/p/chromium/issues/detail?id=447523 if ( selection.isCollapsed && ( selection.rangeCount === 0 || selection.getRangeAt( 0 ).collapsed ) ) { return false; } diff --git a/packages/ckeditor5-engine/src/view/view.js b/packages/ckeditor5-engine/src/view/view.js index 90cb21605e1..69b51e71cf8 100644 --- a/packages/ckeditor5-engine/src/view/view.js +++ b/packages/ckeditor5-engine/src/view/view.js @@ -3,6 +3,8 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/* global document */ + /** * @module engine/view/view */ @@ -65,7 +67,7 @@ export default class View { /** * @param {module:engine/view/stylesmap~StylesProcessor} stylesProcessor The styles processor instance. */ - constructor( stylesProcessor, sourceElementRoot = document ) { // eslint-disable-line no-undef + constructor( stylesProcessor, sourceElementRoot = document ) { /** * Instance of the {@link module:engine/view/document~Document} associated with this view controller. *