Skip to content

Commit

Permalink
Clear selection in editor#destroy, make editorDomRenderer#destroy safer
Browse files Browse the repository at this point in the history
  * Add a `hasRendered` property to Editor and EditorDomRenderer instances.
  * If an editor is destroyed when it has an active cursor, clear that selection.
    This fixes an issue with calling `editor.cursor.offsets` when the
    editor's dom element is selected but the editor has not yet
    rendered. See bustle/ember-mobiledoc-editor#39
  * Update Cursor `hasCursor` and `hasSelection` methods to always
    return false when the editor has not yet rendered.
  * Fix a bug when calling EditorDomRenderer#destroy if it has not yet
    rendered.
  • Loading branch information
bantic committed Dec 17, 2015
1 parent e003067 commit 15ceb0f
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 13 deletions.
25 changes: 16 additions & 9 deletions src/js/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
} from '../utils/paste-utils';
import { DIRECTION } from 'mobiledoc-kit/utils/key';
import { TAB } from 'mobiledoc-kit/utils/characters';
import assert from '../utils/assert';

export const EDITOR_ELEMENT_CLASS_NAME = '__mobiledoc-editor';

Expand Down Expand Up @@ -119,8 +120,9 @@ class Editor {
} else if (this.html) {
if (typeof this.html === 'string') {
return new HTMLParser(this.builder).parse(this.html);
} else { // DOM
return this._parser.parse(this.html);
} else {
let dom = this.html;
return this._parser.parse(dom);
}
} else {
return this.builder.createPost();
Expand All @@ -132,9 +134,8 @@ class Editor {

// if we haven't rendered this post's renderNode before, mark it dirty
if (!postRenderNode.element) {
if (!this.element) {
throw new Error('Initial call to `render` must happen before `rerender` can be called.');
}
assert('Must call `render` before `rerender` can be called',
this.hasRendered);
postRenderNode.element = this.element;
postRenderNode.markDirty();
}
Expand All @@ -147,10 +148,9 @@ class Editor {
}

render(element) {
if (this.element) {
throw new Error('Cannot render an editor twice. Use `rerender` to update the rendering of an existing editor instance');
}

assert('Cannot render an editor twice. Use `rerender` to update the ' +
'rendering of an existing editor instance.',
!this.hasRendered);

addClassName(element, EDITOR_ELEMENT_CLASS_NAME);
element.spellcheck = this.spellcheck;
Expand Down Expand Up @@ -437,6 +437,9 @@ class Editor {

destroy() {
this._isDestroyed = true;
if (this.cursor.hasCursor()) {
this.cursor.clearSelection();
}
this.removeMutationObserver();
this._mutationObserver = null;
this.removeAllEventListeners();
Expand Down Expand Up @@ -769,6 +772,10 @@ class Editor {
cardSection.setInitialMode(mode);
}
}

get hasRendered() {
return !!this.element;
}
}

mixin(Editor, EventEmitter);
Expand Down
5 changes: 5 additions & 0 deletions src/js/renderers/editor-dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -393,9 +393,13 @@ export default class Renderer {
this.editor = editor;
this.visitor = new Visitor(editor, cards, unknownCardHandler, options);
this.nodes = [];
this.hasRendered = false;
}

destroy() {
if (!this.hasRendered) {
return;
}
let renderNode = this.renderTree.rootNode;
let force = true;
removeDestroyedChildren(renderNode, force);
Expand All @@ -413,6 +417,7 @@ export default class Renderer {
}

render(renderTree) {
this.hasRendered = true;
this.renderTree = renderTree;
let renderNode = renderTree.rootNode;
let method, postNode;
Expand Down
6 changes: 4 additions & 2 deletions src/js/utils/cursor.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@ const Cursor = class Cursor {
* editor's element or a selection that is contained in the editor's element
*/
hasCursor() {
return this._hasCollapsedSelection() || this._hasSelection();
return this.editor.hasRendered &&
(this._hasCollapsedSelection() || this._hasSelection());
}

hasSelection() {
return this._hasSelection();
return this.editor.hasRendered &&
this._hasSelection();
}

/**
Expand Down
40 changes: 40 additions & 0 deletions tests/unit/editor/editor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,43 @@ test('activeSections of a rendered blank mobiledoc is an empty array', (assert)
assert.equal(0, editor.activeSections.length,
'empty activeSections');
});

test('editor.cursor.hasCursor() is false before rendering', (assert) => {
let mobiledoc = Helpers.mobiledoc.build(({post}) => post());
editor = new Editor({mobiledoc});

assert.ok(!editor.cursor.hasCursor(), 'no cursor before rendering');

Helpers.dom.moveCursorTo(editorElement, 0);

assert.ok(!editor.cursor.hasCursor(),
'no cursor before rendering, even when selection exists');
});

test('#destroy clears selection if it has one', (assert) => {
let mobiledoc = Helpers.mobiledoc.build(({post}) => post());
editor = new Editor({mobiledoc});
editor.render(editorElement);

Helpers.dom.moveCursorTo(editorElement, 0);
assert.ok(editor.cursor.hasCursor(), 'precond - has cursor');

editor.destroy();

assert.equal(window.getSelection().rangeCount, 0,
'selection is cleared');
});

test('#destroy does not clear selection if it is outside the editor element', (assert) => {
let mobiledoc = Helpers.mobiledoc.build(({post}) => post());
editor = new Editor({mobiledoc});
editor.render(editorElement);

Helpers.dom.moveCursorTo($('#qunit-fixture')[0], 0);
assert.ok(!editor.cursor.hasCursor(), 'precond - has no cursor');
assert.equal(window.getSelection().rangeCount, 1, 'precond - has selection');

editor.destroy();

assert.equal(window.getSelection().rangeCount, 1, 'selection is not cleared');
});
21 changes: 19 additions & 2 deletions tests/unit/renderers/editor-dom-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,22 @@ const ZWNJ = '\u200c';
import placeholderImageSrc from 'mobiledoc-kit/utils/placeholder-image-src';
let builder;

let renderer;
function render(renderTree, cards=[]) {
let editor = {};
let renderer = new Renderer(editor, cards);
renderer = new Renderer(editor, cards);
return renderer.render(renderTree);
}

module('Unit: Renderer: Editor-Dom', {
beforeEach() {
builder = new PostNodeBuilder();
},
afterEach() {
if (renderer) {
renderer.destroy();
renderer = null;
}
}
});

Expand Down Expand Up @@ -174,7 +181,6 @@ test('renders a post with multiple markers', (assert) => {
assert.equal(renderTree.rootElement.innerHTML, '<p>hello <b>bold, <i>italic,</i></b> world.</p>');
});


test('renders a post with image', (assert) => {
let url = placeholderImageSrc;
let post = builder.createPost();
Expand Down Expand Up @@ -598,6 +604,17 @@ test('renders a bunch of spaces with nbsp', (assert) => {
assert.equal(renderTree.rootElement.innerHTML, expectedDOM.outerHTML);
});

test('#destroy is safe to call if renderer has not rendered', (assert) => {
let mockEditor = {}, cards = [];
let renderer = new Renderer(mockEditor, cards);

assert.ok(!renderer.hasRendered, 'precond - has not rendered');

renderer.destroy();

assert.ok(true, 'ok to destroy');
});

/*
test("It renders a renderTree with rendered dirty section", (assert) => {
/*
Expand Down

0 comments on commit 15ceb0f

Please sign in to comment.