From af5ae91300abcfd57645b45a3a1d3ec3ce44f491 Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Tue, 26 Jan 2016 13:57:02 -0500 Subject: [PATCH] Refactor cursor positioning It was previously possible to create a position with a null section or an invalid section (like a list section). A position must have a section that the cursor can be rendered at; this PR adds an assertion to that fact. Before this change, if the window.getSelection() pointed at the editor's element, Position#fromNode would naively assume that the post's first section at offset 0 was the position, but this creates an invalid position when the first section is a list section. * add Post#headPosition and #tailPosition * add Section#isSection property * Refactor Position#fromNode * tests for Position#fromNode for text nodes, element nodes, card-related nodes * Position constructor asserts it has a valid leaft section and offset --- src/js/models/_section.js | 2 + src/js/models/list-section.js | 1 + src/js/models/post.js | 17 +++ src/js/renderers/editor-dom.js | 7 +- src/js/utils/cursor/position.js | 54 ++++----- tests/helpers/assertions.js | 4 +- tests/helpers/mobiledoc.js | 13 +- tests/unit/models/markup-section-test.js | 5 + tests/unit/models/post-test.js | 28 +++++ tests/unit/renderers/editor-dom-test.js | 4 +- tests/unit/utils/cursor-position-test.js | 145 ++++++++++++++++++++++- 11 files changed, 241 insertions(+), 39 deletions(-) diff --git a/src/js/models/_section.js b/src/js/models/_section.js index d4644fff0..aa057bc90 100644 --- a/src/js/models/_section.js +++ b/src/js/models/_section.js @@ -14,6 +14,8 @@ export default class Section extends LinkedItem { this.type = type; this.isMarkerable = false; this.isNested = false; + this.isSection = true; + this.isLeafSection = true; } set tagName(val) { diff --git a/src/js/models/list-section.js b/src/js/models/list-section.js index 122abd6a5..bd0426c83 100644 --- a/src/js/models/list-section.js +++ b/src/js/models/list-section.js @@ -16,6 +16,7 @@ export default class ListSection extends Section { super(LIST_SECTION_TYPE); this.tagName = tagName; this.isListSection = true; + this.isLeafSection = false; this.items = new LinkedList({ adoptItem: i => { diff --git a/src/js/models/post.js b/src/js/models/post.js index b97639fb4..6968f8b02 100644 --- a/src/js/models/post.js +++ b/src/js/models/post.js @@ -4,6 +4,7 @@ import { forEach, compact } from 'mobiledoc-kit/utils/array-utils'; import Set from 'mobiledoc-kit/utils/set'; import mobiledocRenderers from 'mobiledoc-kit/renderers/mobiledoc'; import Range from 'mobiledoc-kit/utils/cursor/range'; +import Position from 'mobiledoc-kit/utils/cursor/position'; export default class Post { constructor() { @@ -14,6 +15,22 @@ export default class Post { }); } + headPosition() { + if (this.isBlank) { + return Position.blankPosition(); + } else { + return this.sections.head.headPosition(); + } + } + + tailPosition() { + if (this.isBlank) { + return Position.blankPosition(); + } else { + return this.sections.tail.tailPosition(); + } + } + get isBlank() { return this.sections.isEmpty; } diff --git a/src/js/renderers/editor-dom.js b/src/js/renderers/editor-dom.js index f30534b54..c66168eb8 100644 --- a/src/js/renderers/editor-dom.js +++ b/src/js/renderers/editor-dom.js @@ -15,10 +15,11 @@ import { MARKUP_SECTION_ELEMENT_NAMES } from '../models/markup-section'; import assert from '../utils/assert'; import { TAB } from 'mobiledoc-kit/utils/characters'; -const CARD_ELEMENT_CLASS_NAME = '__mobiledoc-card'; +export const CARD_ELEMENT_CLASS_NAME = '__mobiledoc-card'; export const NO_BREAK_SPACE = '\u00A0'; export const TAB_CHARACTER = '\u2003'; export const SPACE = ' '; +export const ZWNJ = '\u200c'; function createElementFromMarkup(doc, markup) { var element = doc.createElement(markup.tagName); @@ -102,9 +103,9 @@ function renderCard() { let cardElement = document.createElement('div'); cardElement.contentEditable = false; addClassName(cardElement, CARD_ELEMENT_CLASS_NAME); - wrapper.appendChild(document.createTextNode('\u200c')); + wrapper.appendChild(document.createTextNode(ZWNJ)); wrapper.appendChild(cardElement); - wrapper.appendChild(document.createTextNode('\u200c')); + wrapper.appendChild(document.createTextNode(ZWNJ)); return { wrapper, cardElement }; } diff --git a/src/js/utils/cursor/position.js b/src/js/utils/cursor/position.js index afc7e3685..05222047c 100644 --- a/src/js/utils/cursor/position.js +++ b/src/js/utils/cursor/position.js @@ -4,37 +4,38 @@ import { import { DIRECTION } from 'mobiledoc-kit/utils/key'; import assert from 'mobiledoc-kit/utils/assert'; -function isSection(postNode) { - if (!(postNode && postNode.type)) { return false; } - return postNode.isMarkerable || postNode.isCardSection; -} - function findParentSectionFromNode(renderTree, node) { let renderNode = renderTree.findRenderNodeFromElement( node, - (renderNode) => isSection(renderNode.postNode) + (renderNode) => renderNode.postNode.isSection ); return renderNode && renderNode.postNode; } function findOffsetInSection(section, node, offset) { - if (!section.isCardSection) { - return findOffsetInElement(section.renderNode.element, - node, offset); - } - - // Only the card case - let wrapperNode = section.renderNode.element; - let endTextNode = wrapperNode.lastChild; - if (node === endTextNode) { - return 1; + if (section.isMarkerable) { + return findOffsetInElement(section.renderNode.element, node, offset); + } else { + assert('findOffsetInSection must be called with markerable or card section', + section.isCardSection); + + let wrapperNode = section.renderNode.element; + let endTextNode = wrapperNode.lastChild; + if (node === endTextNode) { + return 1; + } + return 0; } - return 0; } const Position = class Position { constructor(section, offset=0) { + assert('Position must have a section that is addressable by the cursor', + (section && section.isLeafSection)); + assert('Position must have numeric offset', + (offset !== null && offset !== undefined)); + this.section = section; this.offset = offset; this.isBlank = false; @@ -155,22 +156,16 @@ const Position = class Position { } static fromElementNode(renderTree, elementNode, offset) { - let section, offsetInSection; + let position; // The browser may change the reported selection to equal the editor's root // element if the user clicks an element that is immediately removed, // which can happen when clicking to remove a card. if (elementNode === renderTree.rootElement) { let post = renderTree.rootNode.postNode; - if (offset === 0) { - section = post.sections.head; - offsetInSection = 0; - } else { - section = post.sections.tail; - offsetInSection = section.length; - } + position = offset === 0 ? post.headPosition() : post.tailPosition(); } else { - section = findParentSectionFromNode(renderTree, elementNode); + let section = findParentSectionFromNode(renderTree, elementNode); assert('Could not find parent section from element node', !!section); if (section.isCardSection) { @@ -179,14 +174,15 @@ const Position = class Position { // selecting the card's wrapper div. If the offset is 2 we've selected // the final zwnj and should consider the cursor at the end of the card (offset 1). Otherwise, // the cursor is at the start of the card - offsetInSection = offset < 2 ? 0 : 1; + position = offset < 2 ? section.headPosition() : section.tailPosition(); } else { // The offset is 0 if the cursor is on an element node (e.g., a
tag in // a blank markup section) - offsetInSection = 0; + position = section.headPosition(); } } - return new Position(section, offsetInSection); + + return position; } /** diff --git a/tests/helpers/assertions.js b/tests/helpers/assertions.js index 5093dd905..1e2c6fd96 100644 --- a/tests/helpers/assertions.js +++ b/tests/helpers/assertions.js @@ -187,10 +187,10 @@ export default function registerAssertions() { this.push(false, `${position.section.type}:${position.section.tagName}`, `${expected.section.type}:${expected.section.tagName}`, - `incorrect position section`); + `incorrect position section (${message})`); } else if (position.offset !== expected.offset) { this.push(false, position.offset, expected.offset, - `incorrect position offset`); + `incorrect position offset (${message})`); } else { this.push(true, position, expected, message); } diff --git a/tests/helpers/mobiledoc.js b/tests/helpers/mobiledoc.js index 540f0f845..485ee5558 100644 --- a/tests/helpers/mobiledoc.js +++ b/tests/helpers/mobiledoc.js @@ -1,6 +1,8 @@ import PostAbstractHelpers from './post-abstract'; import mobiledocRenderers from 'mobiledoc-kit/renderers/mobiledoc'; import MobiledocRenderer_0_2, { MOBILEDOC_VERSION } from 'mobiledoc-kit/renderers/mobiledoc/0-2'; +import Editor from 'mobiledoc-kit/editor/editor'; +import { mergeWithOptions } from 'mobiledoc-kit/utils/merge'; /* * usage: @@ -25,6 +27,15 @@ function build(treeFn, version) { } } +function renderInto(element, treeFn, editorOptions={}) { + let mobiledoc = build(treeFn); + mergeWithOptions(editorOptions, {mobiledoc}); + let editor = new Editor(editorOptions); + editor.render(element); + return editor; +} + export default { - build + build, + renderInto }; diff --git a/tests/unit/models/markup-section-test.js b/tests/unit/models/markup-section-test.js index b493a3c4a..bd3baab92 100644 --- a/tests/unit/models/markup-section-test.js +++ b/tests/unit/models/markup-section-test.js @@ -253,3 +253,8 @@ test('splitMarkerAtOffset splits a marker deep in the middle', (assert) => { assert.deepEqual(section.markers.map(m => m.value), ['a', 'bc', 'de', 'f', 'ghi']); }); + +test('a section has property `isSection`', (assert) => { + let section = builder.createMarkupSection(); + assert.ok(section.isSection, 'section.isSection'); +}); diff --git a/tests/unit/models/post-test.js b/tests/unit/models/post-test.js index f49df005f..4208784f7 100644 --- a/tests/unit/models/post-test.js +++ b/tests/unit/models/post-test.js @@ -1,5 +1,6 @@ import Helpers from '../../test-helpers'; import Range from 'mobiledoc-kit/utils/cursor/range'; +import Position from 'mobiledoc-kit/utils/cursor/position'; const {module, test} = Helpers; @@ -539,3 +540,30 @@ test('#cloneRange when range contains multiple list items and more sections', (a assert.deepEqual(mobiledoc, expected); }); + +test('#headPosition and #tailPosition returns head and tail', (assert) => { + let post = Helpers.postAbstract.build(({post, markupSection, marker}) => { + return post([ + markupSection('p', [marker('abc')]), + markupSection('p', [marker('123')]) + ]); + }); + + let head = post.headPosition(); + let tail = post.tailPosition(); + + assert.positionIsEqual(head, post.sections.head.headPosition(), 'head pos'); + assert.positionIsEqual(tail, post.sections.tail.tailPosition(), 'tail pos'); +}); + +test('#headPosition and #tailPosition when post is blank return blank', (assert) => { + let post = Helpers.postAbstract.build(({post}) => { + return post(); + }); + + let head = post.headPosition(); + let tail = post.tailPosition(); + + assert.positionIsEqual(head, Position.blankPosition(), 'head pos'); + assert.positionIsEqual(tail, Position.blankPosition(), 'tail pos'); +}); diff --git a/tests/unit/renderers/editor-dom-test.js b/tests/unit/renderers/editor-dom-test.js index 2dc4e19f5..ec06595da 100644 --- a/tests/unit/renderers/editor-dom-test.js +++ b/tests/unit/renderers/editor-dom-test.js @@ -2,12 +2,10 @@ import PostNodeBuilder from 'mobiledoc-kit/models/post-node-builder'; import Renderer from 'mobiledoc-kit/renderers/editor-dom'; import RenderTree from 'mobiledoc-kit/models/render-tree'; import Helpers from '../../test-helpers'; -import { NO_BREAK_SPACE } from 'mobiledoc-kit/renderers/editor-dom'; +import { NO_BREAK_SPACE, ZWNJ } from 'mobiledoc-kit/renderers/editor-dom'; import { TAB } from 'mobiledoc-kit/utils/characters'; const { module, test } = Helpers; -const ZWNJ = '\u200c'; - import placeholderImageSrc from 'mobiledoc-kit/utils/placeholder-image-src'; let builder; diff --git a/tests/unit/utils/cursor-position-test.js b/tests/unit/utils/cursor-position-test.js index 2660a4c59..59455531e 100644 --- a/tests/unit/utils/cursor-position-test.js +++ b/tests/unit/utils/cursor-position-test.js @@ -1,9 +1,21 @@ import Helpers from '../../test-helpers'; import Position from 'mobiledoc-kit/utils/cursor/position'; +import { CARD_ELEMENT_CLASS_NAME, ZWNJ } from 'mobiledoc-kit/renderers/editor-dom'; const {module, test} = Helpers; -module('Unit: Utils: Position'); +let editor, editorElement; + +module('Unit: Utils: Position', { + beforeEach() { + editorElement = $('#editor')[0]; + }, + afterEach() { + if (editor) { + editor.destroy(); + } + } +}); test('#move moves forward and backward in markup section', (assert) => { let post = Helpers.postAbstract.build(({post, markupSection, marker}) => { @@ -118,3 +130,134 @@ test('#move across and beyond card section into list section', (assert) => { assert.positionIsEqual(midHead.moveLeft(), aTail, 'left to prev section'); assert.positionIsEqual(midTail.moveRight(), cHead, 'right to next section'); }); + +test('#fromNode when node is marker text node', (assert) => { + editor = Helpers.mobiledoc.renderInto(editorElement, + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc'), marker('123')])]); + }); + + let textNode = editorElement.firstChild // p + .lastChild; // textNode + + assert.equal(textNode.textContent, '123', 'precond - correct text node'); + + let renderTree = editor._renderTree; + let position = Position.fromNode(renderTree, textNode, 2); + + let section = editor.post.sections.head; + assert.positionIsEqual(position, new Position(section, 'abc'.length + 2)); +}); + +test('#fromNode when node is section node with offset', (assert) => { + editor = Helpers.mobiledoc.renderInto(editorElement, + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc'), marker('123')])]); + }); + + let pNode = editorElement.firstChild; + assert.equal(pNode.tagName.toLowerCase(), 'p', 'precond - correct node'); + + let renderTree = editor._renderTree; + let position = Position.fromNode(renderTree, pNode, 0); + + assert.positionIsEqual(position, editor.post.sections.head.headPosition()); +}); + +test('#fromNode when node is root element and offset is 0', (assert) => { + editor = Helpers.mobiledoc.renderInto(editorElement, + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc'), marker('123')])]); + }); + + let renderTree = editor._renderTree; + let position = Position.fromNode(renderTree, editorElement, 0); + + assert.positionIsEqual(position, editor.post.headPosition()); +}); + +test('#fromNode when node is root element and offset is > 0', (assert) => { + editor = Helpers.mobiledoc.renderInto(editorElement, + ({post, markupSection, marker}) => { + return post([markupSection('p', [marker('abc'), marker('123')])]); + }); + + let renderTree = editor._renderTree; + let position = Position.fromNode(renderTree, editorElement, 1); + + assert.positionIsEqual(position, editor.post.tailPosition()); +}); + +test('#fromNode when node is card section element or next to it', (assert) => { + let editorOptions = { cards: [{ + name: 'some-card', + type: 'dom', + render() { + return $('
this is the card
')[0]; + } + }]}; + editor = Helpers.mobiledoc.renderInto(editorElement, + ({post, cardSection}) => { + return post([cardSection('some-card')]); + }, editorOptions); + + let nodes = { + wrapper: editorElement.firstChild, + leftCursor: editorElement.firstChild.firstChild, + rightCursor: editorElement.firstChild.lastChild, + cardDiv: editorElement.firstChild.childNodes[1] + }; + + assert.ok(nodes.wrapper && nodes.leftCursor && nodes.rightCursor && + nodes.cardDiv, + 'precond - nodes'); + + assert.equal(nodes.wrapper.tagName.toLowerCase(), 'div', 'precond - wrapper'); + assert.equal(nodes.leftCursor.textContent, ZWNJ, 'precond - left cursor'); + assert.equal(nodes.rightCursor.textContent, ZWNJ, 'precond - right cursor'); + assert.ok(nodes.cardDiv.className.indexOf(CARD_ELEMENT_CLASS_NAME) !== -1, + 'precond -card div'); + + let renderTree = editor._renderTree; + let cardSection = editor.post.sections.head; + + let leftPos = cardSection.headPosition(); + let rightPos = cardSection.tailPosition(); + + assert.positionIsEqual(Position.fromNode(renderTree, nodes.wrapper, 0), + leftPos, 'wrapper offset 0'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.wrapper, 1), + leftPos, 'wrapper offset 1'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.wrapper, 2), + rightPos, 'wrapper offset 2'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.leftCursor, 0), + leftPos, 'left cursor offset 0'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.leftCursor, 1), + leftPos, 'left cursor offset 1'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.rightCursor, 0), + rightPos, 'right cursor offset 0'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.rightCursor, 1), + rightPos, 'right cursor offset 1'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.cardDiv, 0), + leftPos, 'card div offset 0'); + assert.positionIsEqual(Position.fromNode(renderTree, nodes.cardDiv, 1), + leftPos, 'card div offset 1'); +}); + +test('Position cannot be on list section', (assert) => { + let post = Helpers.postAbstract.build(({post, listSection, listItem}) => { + return post([listSection('ul', [listItem()])]); + }); + + let listSection = post.sections.head; + let listItem = listSection.items.head; + + let position; + assert.throws(() => { + position = new Position(listSection, 0); + }, /addressable by the cursor/); + + position = new Position(listItem, 0); + assert.ok(position, 'position with list item is ok'); +}); +