From 5ae07eed6792362cc6350b6733d809d2343967f3 Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Mon, 11 Jan 2016 13:45:01 -0500 Subject: [PATCH] Always `setRange` in `toggleMarkup` and `toggleSection` This helps prevent a situation where the editor element has a selection but it is not the active element. Typing while another element (like a button) is focused (active) but the editor element has the selection is problematic: The browser refocuses on the editor element and starts inserting text, but it handles the input before it has focused, which means the key* handlers do not fire. Also renamed Position#emptyPosition and Range#emptyRange -> blankPosition, blankRange. Adds `isBlank` property to Position and Range. Defensively avoid attempting to render a cursor when the range is blank. When cursor contains non-markerable, toggleMarkup is no-op Fixes #285 Fixes #287 --- src/js/editor/editor.js | 6 +- src/js/editor/post.js | 14 +-- src/js/models/_section.js | 8 ++ src/js/utils/cursor.js | 2 +- src/js/utils/cursor/position.js | 7 +- src/js/utils/cursor/range.js | 8 +- tests/unit/editor/post-test.js | 179 +++++++++++++++++++++++++++++++- 7 files changed, 209 insertions(+), 15 deletions(-) diff --git a/src/js/editor/editor.js b/src/js/editor/editor.js index 969844fed..4564b4e79 100644 --- a/src/js/editor/editor.js +++ b/src/js/editor/editor.js @@ -306,7 +306,11 @@ class Editor { // @private renderRange() { - this.cursor.selectRange(this.range); + if (this.range.isBlank) { + this.cursor.clearSelection(); + } else { + this.cursor.selectRange(this.range); + } this._reportSelectionState(); // ensure that the range is "cleaned"/un-cached after diff --git a/src/js/editor/post.js b/src/js/editor/post.js index 1309f5f82..c7a8bc576 100644 --- a/src/js/editor/post.js +++ b/src/js/editor/post.js @@ -849,13 +849,10 @@ class PostEditor { * @param {Markup|String} markupOrString Either a markup object created using * the builder (useful when adding a markup with attributes, like an 'a' markup), * or, if a string, the tag name of the markup (e.g. 'strong', 'em') to toggle. + * @param {Range} range in which to toggle, defaults to current editor range * @public */ - toggleMarkup(markupOrMarkupString) { - const range = this.editor.cursor.offsets; - if (range.isCollapsed) { - return; - } + toggleMarkup(markupOrMarkupString, range=this.editor.range) { const markup = typeof markupOrMarkupString === 'string' ? this.builder.createMarkup(markupOrMarkupString) : markupOrMarkupString; @@ -869,7 +866,8 @@ class PostEditor { } else { this.addMarkupToRange(range, markup); } - this.scheduleAfterRender(() => this.editor.selectRange(range)); + + this.setRange(range); } /** @@ -887,6 +885,7 @@ class PostEditor { toggleSection(sectionTagName, range=this.editor.range) { sectionTagName = normalizeTagName(sectionTagName); let { post } = this.editor; + let nextRange = range; let everySectionHasTagName = true; post.walkMarkerableSections(range, section => { @@ -903,8 +902,9 @@ class PostEditor { }); if (firstChanged) { - this.setRange(new Range(firstChanged.headPosition())); + nextRange = new Range(firstChanged.headPosition()); } + this.setRange(nextRange); } _isSameSectionType(section, sectionTagName) { diff --git a/src/js/models/_section.js b/src/js/models/_section.js index 0f93bd503..193a93964 100644 --- a/src/js/models/_section.js +++ b/src/js/models/_section.js @@ -54,6 +54,14 @@ export default class Section extends LinkedItem { unimplementedMethod('join', this); } + /** + * Markerable sections should override this method + */ + splitMarkerAtOffset() { + let blankEdit = { added: [], removed: [] }; + return blankEdit; + } + nextLeafSection() { const next = this.next; if (next) { diff --git a/src/js/utils/cursor.js b/src/js/utils/cursor.js index 64de23f4f..24d64ad18 100644 --- a/src/js/utils/cursor.js +++ b/src/js/utils/cursor.js @@ -59,7 +59,7 @@ const Cursor = class Cursor { * @return {Range} Cursor#Range object */ get offsets() { - if (!this.hasCursor()) { return Range.emptyRange(); } + if (!this.hasCursor()) { return Range.blankRange(); } const { selection, renderTree } = this; diff --git a/src/js/utils/cursor/position.js b/src/js/utils/cursor/position.js index 5b1e43d01..afc7e3685 100644 --- a/src/js/utils/cursor/position.js +++ b/src/js/utils/cursor/position.js @@ -37,16 +37,17 @@ const Position = class Position { constructor(section, offset=0) { this.section = section; this.offset = offset; + this.isBlank = false; } - static emptyPosition() { + static blankPosition() { return { section: null, offset: 0, marker: null, offsetInTextNode: 0, - _isEmpty: true, - isEqual(other) { return other._isEmpty; }, + isBlank: true, + isEqual(other) { return other.isBlank; }, markerPosition: {} }; } diff --git a/src/js/utils/cursor/range.js b/src/js/utils/cursor/range.js index db0e11e07..7f74c1e11 100644 --- a/src/js/utils/cursor/range.js +++ b/src/js/utils/cursor/range.js @@ -19,8 +19,8 @@ export default class Range { return new Range(section.headPosition(), section.tailPosition()); } - static emptyRange() { - return new Range(Position.emptyPosition(), Position.emptyPosition()); + static blankRange() { + return new Range(Position.blankPosition(), Position.blankPosition()); } /** @@ -59,6 +59,10 @@ export default class Range { this.tail.isEqual(other.tail); } + get isBlank() { + return this.head.isBlank && this.tail.isBlank; + } + // "legacy" APIs get headSection() { return this.head.section; diff --git a/tests/unit/editor/post-test.js b/tests/unit/editor/post-test.js index 27d7f689a..f31300a72 100644 --- a/tests/unit/editor/post-test.js +++ b/tests/unit/editor/post-test.js @@ -7,6 +7,7 @@ import { DIRECTION } from 'mobiledoc-kit/utils/key'; import PostNodeBuilder from 'mobiledoc-kit/models/post-node-builder'; import Range from 'mobiledoc-kit/utils/cursor/range'; import Position from 'mobiledoc-kit/utils/cursor/position'; +import { clearSelection } from 'mobiledoc-kit/utils/selection-utils'; const { FORWARD } = DIRECTION; @@ -1006,6 +1007,53 @@ test('#toggleSection skips over non-markerable sections', (assert) => { assert.positionIsEqual(renderedRange.head, post.sections.head.headPosition()); }); +test('#toggleSection when cursor is in non-markerable section changes nothing', (assert) => { + let post = Helpers.postAbstract.build( + ({post, markupSection, marker, cardSection}) => { + return post([ + cardSection('my-card') + ]); + }); + + const mockEditor = renderBuiltAbstract(post); + const range = new Range(post.sections.head.headPosition()); + + postEditor = new PostEditor(mockEditor); + postEditor.toggleSection('blockquote', range); + postEditor.complete(); + + assert.ok(post.sections.head.isCardSection, 'card section not changed'); + assert.positionIsEqual(renderedRange.head, post.sections.head.headPosition()); +}); + +test('#toggleSection when editor has no cursor does nothing', (assert) => { + editor = buildEditorWithMobiledoc( + ({post, markupSection, marker, cardSection}) => { + return post([ + cardSection('my-card') + ]); + }); + let expected = Helpers.postAbstract.build( + ({post, markupSection, marker, cardSection}) => { + return post([ + cardSection('my-card') + ]); + }); + + editorElement.blur(); + clearSelection(); + + postEditor = new PostEditor(editor); + postEditor.toggleSection('blockquote'); + postEditor.complete(); + + assert.postIsSimilar(editor.post, expected); + assert.ok(document.activeElement !== editorElement, + 'editor element is not active'); + assert.ok(renderedRange.isBlank, 'rendered range is blank'); + assert.equal(window.getSelection().rangeCount, 0, 'nothing selected'); +}); + test('#toggleSection toggle single p -> list item', (assert) => { let post = Helpers.postAbstract.build( ({post, markupSection, marker, markup}) => { @@ -1387,7 +1435,8 @@ test('#toggleSection toggle when cursor on card section is no-op', (assert) => { assert.equal(post.sections.length, 1, '1 section'); assert.ok(post.sections.head.isCardSection, 'still card section'); - assert.ok(!renderedRange, 'cursor position not changed'); + assert.positionIsEqual(renderedRange.head, range.head, 'range head is set to same'); + assert.positionIsEqual(renderedRange.tail, range.tail, 'range tail is set to same'); }); test('#toggleSection joins contiguous list items', (assert) => { @@ -1415,6 +1464,134 @@ test('#toggleSection joins contiguous list items', (assert) => { ['abc', '123', 'def']); }); +test('#toggleMarkup when cursor is in non-markerable does nothing', (assert) => { + editor = buildEditorWithMobiledoc( + ({post, markupSection, marker, cardSection}) => { + return post([ + cardSection('my-card') + ]); + }); + + const range = new Range(editor.post.sections.head.headPosition()); + postEditor = new PostEditor(editor); + postEditor.toggleMarkup('b', range); + postEditor.complete(); + + assert.ok(editor.post.sections.head.isCardSection); + assert.positionIsEqual(renderedRange.head, + editor.post.sections.head.headPosition()); +}); + +test('#toggleMarkup when cursor surrounds non-markerable does nothing', (assert) => { + editor = buildEditorWithMobiledoc( + ({post, markupSection, marker, cardSection}) => { + return post([ + cardSection('my-card') + ]); + }); + + const range = Range.fromSection(editor.post.sections.head); + postEditor = new PostEditor(editor); + postEditor.toggleMarkup('b', range); + postEditor.complete(); + + assert.ok(editor.post.sections.head.isCardSection); + assert.positionIsEqual(renderedRange.head, + editor.post.sections.head.headPosition()); +}); + +test('#toggleMarkup when range has the markup removes it', (assert) => { + editor = buildEditorWithMobiledoc( + ({post, markupSection, marker, markup}) => { + return post([markupSection('p', [marker('abc', [markup('b')])])]); + }); + let expected = Helpers.postAbstract.build( + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc')])]); + }); + + const range = Range.fromSection(editor.post.sections.head); + postEditor = new PostEditor(editor); + postEditor.toggleMarkup('b', range); + postEditor.complete(); + + assert.positionIsEqual(renderedRange.head, editor.post.sections.head.headPosition()); + assert.positionIsEqual(renderedRange.tail, editor.post.sections.head.tailPosition()); + assert.postIsSimilar(editor.post, expected); +}); + +test('#toggleMarkup when only some of the range has it removes it', (assert) => { + editor = buildEditorWithMobiledoc( + ({post, markupSection, marker, markup}) => { + return post([markupSection('p', [ + marker('a'), + marker('b', [markup('b')]), + marker('c') + ])]); + }); + let expected = Helpers.postAbstract.build( + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc')])]); + }); + + const range = Range.fromSection(editor.post.sections.head); + postEditor = new PostEditor(editor); + postEditor.toggleMarkup('b', range); + postEditor.complete(); + + assert.positionIsEqual(renderedRange.head, + editor.post.sections.head.headPosition()); + assert.positionIsEqual(renderedRange.tail, + editor.post.sections.head.tailPosition()); + assert.postIsSimilar(editor.post, expected); +}); + +test('#toggleMarkup when range does not have the markup adds it', (assert) => { + editor = buildEditorWithMobiledoc( + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc')])]); + }); + let expected = Helpers.postAbstract.build( + ({post, markupSection, marker, markup}) => { + return post([markupSection('p', [marker('abc', [markup('b')])])]); + }); + + const range = Range.fromSection(editor.post.sections.head); + postEditor = new PostEditor(editor); + postEditor.toggleMarkup('b', range); + postEditor.complete(); + + assert.positionIsEqual(renderedRange.head, + editor.post.sections.head.headPosition()); + assert.positionIsEqual(renderedRange.tail, + editor.post.sections.head.tailPosition()); + assert.postIsSimilar(editor.post, expected); +}); + +test('#toggleMarkup when the editor has no cursor', (assert) => { + editor = buildEditorWithMobiledoc( + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc')])]); + }); + let expected = Helpers.postAbstract.build( + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc')])]); + }); + + editorElement.blur(); + clearSelection(); + postEditor = new PostEditor(editor); + postEditor.toggleMarkup('b'); + postEditor.complete(); + + assert.postIsSimilar(editor.post, expected); + assert.equal(window.getSelection().rangeCount, 0, + 'nothing is selected'); + assert.ok(document.activeElement !== editorElement, + 'active element is not editor element'); + assert.ok(renderedRange.isBlank, 'rendered range is blank'); +}); + test('#insertMarkers inserts the markers in middle, merging markups', (assert) => { let toInsert, expected; Helpers.postAbstract.build(({post, markupSection, marker, markup}) => {