Skip to content

Commit

Permalink
Disable event manager when editing is disabled
Browse files Browse the repository at this point in the history
Previously, manual (as opposed to programmatic) editing was being
prevented for an editor after calling `#disableEditing` by setting
"contenteditable" back to false for the editor. This prevents the
browser from sending most of the edit-related events (keyup, keydown,
etc) that occur when editing manually. But some events (like paste) are
still emitted by the browser even though "contenteditable" is false.

This change calls `EventManager#stop` when editing is disabled, and
internally the event manager ignores all events when it is stopped.

Also some rearrangement

Other changes
  * Add test for triggering paste event in a disabled editor
  * add assertion `assert.isBlank`
  * better assertion actual/expected values for `hasElement` and `hasNoElement`
  * add test helper `getData` (for parity with the `setData` in
    element-utils and to avoid using jQuery's `.data()` method)
  * consolidate the editing-disabled tests into a new suite
  * make `editor#render` call `disableEditing` or `enableEditing` after
    it sets `hasRendered` to ensure that the event manager is stopped if
    the user calls `editor.disableEditing()` before calling
    `editor.render()`

Fixes #572
  • Loading branch information
bantic committed Aug 17, 2017
1 parent 3d8cf49 commit f5af1eb
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 54 deletions.
18 changes: 10 additions & 8 deletions src/js/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class Editor {
assert('editor create accepts an options object. For legacy usage passing an element for the first argument, consider the `html` option for loading DOM or HTML posts. For other cases call `editor.render(domNode)` after editor creation',
(options && !options.nodeType));
this._views = [];
this.isEditable = null;
this.isEditable = true;
this._parserPlugins = options.parserPlugins || [];

// FIXME: This should merge onto this.options
Expand Down Expand Up @@ -239,10 +239,6 @@ class Editor {

this.element = element;

if (this.isEditable === null) {
this.enableEditing();
}

this._addTooltip();

// A call to `run` will trigger the didUpdatePostCallbacks hooks with a
Expand All @@ -258,6 +254,12 @@ class Editor {
this._mutationHandler.init();
this._eventManager.init();

if (this.isEditable === false) {
this.disableEditing();
} else {
this.enableEditing();
}

if (this.autofocus) {
this.selectRange(this.post.headPosition());
}
Expand Down Expand Up @@ -629,10 +631,9 @@ class Editor {
* @public
*/
disableEditing() {
if (this.isEditable === false) { return; }

this.isEditable = false;
if (this.hasRendered) {
this._eventManager.stop();
this.element.setAttribute('contentEditable', false);
this.setPlaceholder('');
this.selectRange(Range.blankRange());
Expand All @@ -648,7 +649,8 @@ class Editor {
*/
enableEditing() {
this.isEditable = true;
if (this.element) {
if (this.hasRendered) {
this._eventManager.start();
this.element.setAttribute('contentEditable', true);
this.setPlaceholder(this.placeholder);
}
Expand Down
14 changes: 14 additions & 0 deletions src/js/editor/event-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export default class EventManager {

this._selectionManager = new SelectionManager(
this.editor, this.selectionDidChange.bind(this));
this.started = true;
}

init() {
Expand All @@ -41,6 +42,14 @@ export default class EventManager {
this._selectionManager.start();
}

start() {
this.started = true;
}

stop() {
this.started = false;
}

registerInputHandler(inputHandler) {
this._textInputHandler.register(inputHandler);
}
Expand Down Expand Up @@ -90,6 +99,11 @@ export default class EventManager {

_handleEvent(type, event) {
let {target: element} = event;
if (!this.started) {
// abort handling this event
return true;
}

if (!this.isElementAddressable(element)) {
// abort handling this event
return true;
Expand Down
41 changes: 0 additions & 41 deletions tests/acceptance/basic-editor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,47 +31,6 @@ test('sets element as contenteditable', (assert) => {
'element is contenteditable');
});

test('#disableEditing before render is meaningful', (assert) => {
editor = new Editor();
editor.disableEditing();
editor.render(editorElement);

assert.ok(!editorElement.hasAttribute('contenteditable'),
'element is not contenteditable');
editor.enableEditing();
assert.equal(editorElement.getAttribute('contenteditable'),
'true',
'element is contenteditable');
});

test('when editing is disabled, the placeholder is not shown', (assert) => {
editor = new Editor({placeholder: 'the placeholder'});
editor.disableEditing();
editor.render(editorElement);

assert.ok(!$('#editor').data('placeholder'), 'no placeholder when disabled');
editor.enableEditing();
assert.equal($('#editor').data('placeholder'), 'the placeholder',
'placeholder is shown when editable');
});

test('#disableEditing and #enableEditing toggle contenteditable', (assert) => {
editor = new Editor();
editor.render(editorElement);

assert.equal(editorElement.getAttribute('contenteditable'),
'true',
'element is contenteditable');
editor.disableEditing();
assert.equal(editorElement.getAttribute('contenteditable'),
'false',
'element is not contenteditable');
editor.enableEditing();
assert.equal(editorElement.getAttribute('contenteditable'),
'true',
'element is contenteditable');
});

test('clicking outside the editor does not raise an error', (assert) => {
const done = assert.async();
editor = new Editor({autofocus: false});
Expand Down
81 changes: 81 additions & 0 deletions tests/acceptance/editor-disable-editing-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { Editor } from 'mobiledoc-kit';
import Helpers from '../test-helpers';
import { TAB, ENTER } from 'mobiledoc-kit/utils/characters';
import { MIME_TEXT_PLAIN } from 'mobiledoc-kit/utils/parse-utils';

const { test, module } = Helpers;

const cards = [{
name: 'my-card',
type: 'dom',
render() {},
edit() {}
}];

let editor, editorElement;

module('Acceptance: editor: #disableEditing', {
beforeEach() {
editorElement = $('#editor')[0];
},
afterEach() {
if (editor) { editor.destroy(); }
}
});

test('#disableEditing before render is meaningful', (assert) => {
editor = new Editor();
editor.disableEditing();
editor.render(editorElement);

assert.equal(editorElement.getAttribute('contenteditable'),'false',
'element is not contenteditable');
editor.enableEditing();
assert.equal(editorElement.getAttribute('contenteditable'), 'true',
'element is contenteditable');
});

test('when editing is disabled, the placeholder is not shown', (assert) => {
editor = new Editor({placeholder: 'the placeholder'});
editor.disableEditing();
editor.render(editorElement);

assert.isBlank(Helpers.dom.getData(editorElement, 'placeholder'),
'no placeholder when disabled');
editor.enableEditing();
assert.equal(Helpers.dom.getData(editorElement, 'placeholder'), 'the placeholder',
'placeholder is shown when editable');
});

test('#disableEditing and #enableEditing toggle contenteditable', (assert) => {
editor = new Editor();
editor.render(editorElement);

assert.equal(editorElement.getAttribute('contenteditable'),
'true',
'element is contenteditable');
editor.disableEditing();
assert.equal(editorElement.getAttribute('contenteditable'),
'false',
'element is not contenteditable');
editor.enableEditing();
assert.equal(editorElement.getAttribute('contenteditable'),
'true',
'element is contenteditable');
});

// https://github.com/bustle/mobiledoc-kit/issues/572
test('pasting after #disableEditing does not insert text', function(assert) {
editor = Helpers.editor.buildFromText('abc|', {element: editorElement});

Helpers.dom.setCopyData(MIME_TEXT_PLAIN, 'def');
Helpers.dom.triggerPasteEvent(editor);
assert.hasElement('#editor:contains(abcdef)', 'precond - text is pasted');

editor.disableEditing();

Helpers.dom.selectText(editor, 'def');
Helpers.dom.setCopyData(MIME_TEXT_PLAIN, 'ghi');
Helpers.dom.triggerPasteEvent(editor);
assert.hasNoElement('#editor:contains(ghi)', 'text is not pasted after #disableEditing');
});
17 changes: 13 additions & 4 deletions tests/helpers/assertions.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,22 @@ function comparePostNode(actual, expected, assert, path='root', deepCompare=fals
/* eslint-enable complexity */

export default function registerAssertions(QUnit) {
QUnit.assert.isBlank = function(val, message=`value is blank`) {
this.pushResult({
result: val === null || val === undefined || val === '' || val === false,
actual: `${val} (typeof ${typeof val})`,
expected: `null|undefined|''|false`,
message
});
};

QUnit.assert.hasElement = function(selector,
message=`hasElement "${selector}"`) {
let found = $(selector);
this.pushResult({
result: found.length > 0,
actual: found.length,
expected: selector,
actual: `${found.length} matches for '${selector}'`,
expected: `>0 matches for '${selector}'`,
message: message
});
return found;
Expand All @@ -144,8 +153,8 @@ export default function registerAssertions(QUnit) {
let found = $(selector);
this.pushResult({
result: found.length === 0,
actual: found.length,
expected: selector,
actual: `${found.length} matches for '${selector}'`,
expected: `0 matches for '${selector}'`,
message: message
});
return found;
Expand Down
12 changes: 11 additions & 1 deletion tests/helpers/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
MIME_TEXT_PLAIN,
MIME_TEXT_HTML
} from 'mobiledoc-kit/utils/parse-utils';
import { dasherize } from 'mobiledoc-kit/utils/string-utils';

function assertEditor(editor) {
if (!(editor instanceof Editor)) {
Expand Down Expand Up @@ -365,6 +366,14 @@ function blur() {
input.focus();
}

function getData(element, name) {
if (element.dataset) {
return element.dataset[name];
} else {
return element.getAttribute(dasherize(name));
}
}

const DOMHelper = {
moveCursorTo,
moveCursorWithoutNotifyingEditorTo,
Expand Down Expand Up @@ -394,7 +403,8 @@ const DOMHelper = {
clearCopyData,
createMockEvent,
findTextNode,
blur
blur,
getData
};

export { triggerEvent };
Expand Down

0 comments on commit f5af1eb

Please sign in to comment.