Bound view roots to model roots. #1233
Conversation
src/controller/editingcontroller.js
Outdated
| @@ -31,6 +32,10 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix'; | |||
| * including selection handling. It also creates {@link ~EditingController#view view document} which build a | |||
| * browser-independent virtualization over the DOM elements. Editing controller also attach default converters. | |||
| * | |||
| * Editing controller binds {@link module:engine/view/document~Document#roots view roots collection} to | |||
There was a problem hiding this comment.
I don't think this is essential for the class overview. I believe this is an implementation detail. Even if, right now, this is an important behavior, for the person who wants to learn what EditingController is, binging roots is the least important information. I would move this comment to the place where binding is implemented.
src/model/document.js
Outdated
| @@ -148,14 +149,18 @@ export default class Document { | |||
| /** | |||
| * Creates a new top-level root. | |||
| * | |||
| * **Note**: Creating model root automatically creates corresponding | |||
| * {@link module:engine/view/document~Document#roots view root} with the same name. Name of view root | |||
| * will be hanged later on when dom root will be {@link module:engine/view/document~Document#attachDomRoot attached}. | |||
There was a problem hiding this comment.
I don't think we need to have such information in the model. The model has nothing to do with view and bing. The information in the editing controller and view is enough.
src/model/document.js
Outdated
| @@ -213,7 +218,7 @@ export default class Document { | |||
| * @returns {Boolean} | |||
| */ | |||
| hasRoot( name ) { | |||
| return this.roots.has( name ); | |||
| return !!this.roots.get( name ); | |||
There was a problem hiding this comment.
Can we remove hasRoot method and replace its usage with getRoot (which should not throw but return falsy value if there is no such root? I feel that now when the collection does not have has method this code is ridiculous.
src/view/rooteditableelement.js
Outdated
| /** | ||
| * Overrides old element name and sets new one. | ||
| * This is needed because name of view root has to be changed as the same as dom root name | ||
| * when dom root is attached to view root. |
There was a problem hiding this comment.
This is needed because view roots are created before they are attached to the DOM. The name of the root element is unknown at this stage. It has to be changed when the view root element is attached to the DOM element.
| @@ -40,7 +42,8 @@ describe( 'advanced-converters', () => { | |||
|
|
|||
| const editing = new EditingController( model ); | |||
|
|
|||
| viewRoot = editing.createRoot( 'div' ); | |||
| editing.view.attachDomRoot( document.createElement( 'div' ) ); | |||
| viewRoot = editing.view.getRoot(); | |||
There was a problem hiding this comment.
Can't you just set the name of the view root element without attaching it to the DOM (with a comment what is going on)? Tests which do not use DOM are usually more stable.
| @@ -63,7 +65,7 @@ describe( 'Model converter builder', () => { | |||
| modelRoot = modelDoc.createRoot(); | |||
|
|
|||
| controller = new EditingController( model ); | |||
| controller.createRoot( 'div' ); | |||
| controller.view.attachDomRoot( document.createElement( 'div' ) ); | |||
Suggested merge commit message (convention)
Fix: Bound collection of view roots to collection of model roots. Closes https://github.com/ckeditor/ckeditor5-core/issues/110.
BREAKING CHANGE:
view/Document#rootsandmodel/Document#rootsnow areCollectioninstances.BREAKING CHANGE
view/Document#createRootis removed in favor of roots collections binding.BREAKING CHANGE:
view/Model#hasRootis removed in favor ofview/Model#getRoot.BREAKING CHANGE:
EditingController#createRootis removed.Part of: ckeditor/ckeditor5-core#114