diff --git a/src/js/commands/link.js b/src/js/commands/link.js index 54ab18fb3..0d3e66bf2 100644 --- a/src/js/commands/link.js +++ b/src/js/commands/link.js @@ -9,9 +9,9 @@ export default class LinkCommand extends TextFormatCommand { }); } - exec(url) { + exec(href) { this.editor.run(postEditor => { - const markup = postEditor.builder.createMarkup('a', ['href', url]); + const markup = postEditor.builder.createMarkup('a', {href}); this.editor.run(postEditor => postEditor.toggleMarkup(markup)); }); } diff --git a/src/js/models/markup.js b/src/js/models/markup.js index 8bfe65e23..ad9b5ab4e 100644 --- a/src/js/models/markup.js +++ b/src/js/models/markup.js @@ -12,10 +12,13 @@ export const VALID_MARKUP_TAGNAMES = [ class Markup { /* - * @param {attributes} array flat array of key1,value1,key2,value2,... + * @param {Object} attributes key-values */ - constructor(tagName, attributes=[]) { + constructor(tagName, attributes={}) { this.tagName = normalizeTagName(tagName); + if (Array.isArray(attributes)) { + throw new Error('Must use attributes object param (not array) to Markup'); + } this.attributes = attributes; this.type = MARKUP_TYPE; @@ -25,12 +28,11 @@ class Markup { } hasTag(tagName) { - tagName = normalizeTagName(tagName); - return this.tagName === tagName; + return this.tagName === normalizeTagName(tagName); } static isValidElement(element) { - let tagName = normalizeTagName(element.tagName); + const tagName = normalizeTagName(element.tagName); return VALID_MARKUP_TAGNAMES.indexOf(tagName) !== -1; } } diff --git a/src/js/models/post-node-builder.js b/src/js/models/post-node-builder.js index 25b0cabfc..7abb97304 100644 --- a/src/js/models/post-node-builder.js +++ b/src/js/models/post-node-builder.js @@ -7,6 +7,7 @@ import Marker from '../models/marker'; import Markup from '../models/markup'; import Card from '../models/card'; import { normalizeTagName } from '../utils/dom-utils'; +import { objectToSortedKVArray } from '../utils/array-utils'; import { DEFAULT_TAG_NAME as DEFAULT_MARKUP_SECTION_TAG_NAME } from '../models/markup-section'; @@ -15,6 +16,19 @@ import { DEFAULT_TAG_NAME as DEFAULT_LIST_SECTION_TAG_NAME } from '../models/list-section'; +function cacheKey(tagName, attributes) { + return `${normalizeTagName(tagName)}-${objectToSortedKVArray(attributes).join('-')}`; +} + +function addMarkupToCache(cache, markup) { + cache[cacheKey(markup.tagName, markup.attributes)] = markup; +} + +function findMarkupInCache(cache, tagName, attributes) { + const key = cacheKey(tagName, attributes); + return cache[key]; +} + export default class PostNodeBuilder { constructor() { this.markupCache = {}; @@ -75,25 +89,19 @@ export default class PostNodeBuilder { return marker; } - // Attributes is an array of [key1, value1, key2, value2, ...] - createMarkup(tagName, attributes=[]) { + /** + * @param {Object} attributes {key:value} + */ + createMarkup(tagName, attributes={}) { tagName = normalizeTagName(tagName); - let markup; - - if (attributes.length) { - // FIXME: This could also be cached + let markup = findMarkupInCache(this.markupCache, tagName, attributes); + if (!markup) { markup = new Markup(tagName, attributes); - } else { - markup = this.markupCache[tagName]; - - if (!markup) { - markup = new Markup(tagName, attributes); - this.markupCache[tagName] = markup; - } + markup.builder = this; + addMarkupToCache(this.markupCache, markup); } - markup.builder = this; return markup; } } diff --git a/src/js/parsers/dom.js b/src/js/parsers/dom.js index 0db868ea0..2c781318c 100644 --- a/src/js/parsers/dom.js +++ b/src/js/parsers/dom.js @@ -1,7 +1,7 @@ import { trim } from 'content-kit-utils'; import { VALID_MARKUP_SECTION_TAGNAMES } from '../models/markup-section'; import { VALID_MARKUP_TAGNAMES } from '../models/markup'; -import { normalizeTagName } from '../utils/dom-utils'; +import { getAttributes, normalizeTagName } from '../utils/dom-utils'; const ELEMENT_NODE = 1; const TEXT_NODE = 3; @@ -12,48 +12,6 @@ function isEmptyTextNode(node) { return node.nodeType === TEXT_NODE && trim(node.textContent) === ''; } -// FIXME we need sorted attributes for deterministic tests. This is not -// a particularly elegant method, since it loops at least 3 times. -function sortAttributes(attributes) { - let keyValueAttributes = []; - let currentKey; - attributes.forEach((keyOrValue, index) => { - if (index % 2 === 0) { - currentKey = keyOrValue; - } else { - keyValueAttributes.push({key:currentKey, value:keyOrValue}); - } - }); - keyValueAttributes.sort((a,b) => { - return a.key === b.key ? 0 : - a.key > b.key ? 1 : - 1; - }); - let sortedAttributes = []; - keyValueAttributes.forEach(({key, value}) => { - sortedAttributes.push(key, value); - }); - return sortedAttributes; -} - -/** - * @return {array} attributes as key1,value1,key2,value2,etc - */ -function readAttributes(node) { - var attributes = []; - - if (node.hasAttributes()) { - var i, l; - for (i=0,l=node.attributes.length;i this.parseMarkerType(markerType)); } - parseMarkerType([tagName, attributes]) { - return this.builder.createMarkup(tagName, attributes); + parseMarkerType([tagName, attributesArray]) { + const attributesObject = kvArrayToObject(attributesArray || []); + return this.builder.createMarkup(tagName, attributesObject); } parseSections(sections, post) { diff --git a/src/js/parsers/post.js b/src/js/parsers/post.js index e04279252..dbb412229 100644 --- a/src/js/parsers/post.js +++ b/src/js/parsers/post.js @@ -6,7 +6,7 @@ import { import SectionParser from 'content-kit-editor/parsers/section'; import { forEach } from 'content-kit-editor/utils/array-utils'; -import { getAttributesArray, walkTextNodes } from '../utils/dom-utils'; +import { getAttributes, walkTextNodes } from '../utils/dom-utils'; import Markup from 'content-kit-editor/models/markup'; export default class PostParser { @@ -51,9 +51,8 @@ export default class PostParser { // Turn an element node into a markup markupFromNode(node) { if (Markup.isValidElement(node)) { - let tagName = node.tagName; - let attributes = getAttributesArray(node); - + const tagName = node.tagName; + const attributes = getAttributes(node); return this.builder.createMarkup(tagName, attributes); } } diff --git a/src/js/parsers/section.js b/src/js/parsers/section.js index 1440bb2d6..4d1bc646c 100644 --- a/src/js/parsers/section.js +++ b/src/js/parsers/section.js @@ -93,7 +93,6 @@ export default class SectionParser { markupFromElement(element) { const tagName = normalizeTagName(element.tagName); if (VALID_MARKUP_TAGNAMES.indexOf(tagName) === -1) { return null; } - return this.builder.createMarkup(tagName, getAttributes(element)); } diff --git a/src/js/renderers/editor-dom.js b/src/js/renderers/editor-dom.js index 91c7663ca..5f9d591b4 100644 --- a/src/js/renderers/editor-dom.js +++ b/src/js/renderers/editor-dom.js @@ -17,11 +17,9 @@ export const SPACE = ' '; function createElementFromMarkup(doc, markup) { var element = doc.createElement(markup.tagName); - if (markup.attributes) { - for (var i=0, l=markup.attributes.length;i { + element.setAttribute(k, markup.attributes[k]); + }); return element; } diff --git a/src/js/renderers/mobiledoc.js b/src/js/renderers/mobiledoc.js index f33f2523c..f27e537b8 100644 --- a/src/js/renderers/mobiledoc.js +++ b/src/js/renderers/mobiledoc.js @@ -1,4 +1,5 @@ import {visit, visitArray, compile} from '../utils/compiler'; +import { objectToSortedKVArray } from '../utils/array-utils'; import { POST_TYPE, MARKUP_SECTION_TYPE, @@ -44,7 +45,7 @@ const visitor = { visitArray(visitor, node.openedMarkups, opcodes); }, [MARKUP_TYPE](node, opcodes) { - opcodes.push(['openMarkup', node.tagName, node.attributes]); + opcodes.push(['openMarkup', node.tagName, objectToSortedKVArray(node.attributes)]); } }; @@ -84,21 +85,24 @@ const postOpcodeCompiler = { }; }, openMarkup(tagName, attributes) { - if (!this._seenMarkerTypes) { - this._seenMarkerTypes = {}; - } - let index; - if (attributes.length) { - this.markerTypes.push([tagName, attributes]); - index = this.markerTypes.length - 1; - } else { - index = this._seenMarkerTypes[tagName]; - if (index === undefined) { - this.markerTypes.push([tagName]); - this._seenMarkerTypes[tagName] = index = this.markerTypes.length-1; - } - } + const index = this._findOrAddMarkerTypeIndex(tagName, attributes); this.markupMarkerIds.push(index); + }, + _findOrAddMarkerTypeIndex(tagName, attributesArray) { + if (!this._markerTypeCache) { this._markerTypeCache = {}; } + const key = `${tagName}-${attributesArray.join('-')}`; + + let index = this._markerTypeCache[key]; + if (index === undefined) { + let markerType = [tagName]; + if (attributesArray.length) { markerType.push(attributesArray); } + this.markerTypes.push(markerType); + + index = this.markerTypes.length - 1; + this._markerTypeCache[key] = index; + } + + return index; } }; diff --git a/src/js/utils/array-utils.js b/src/js/utils/array-utils.js index 35e8e10e1..f26051b1f 100644 --- a/src/js/utils/array-utils.js +++ b/src/js/utils/array-utils.js @@ -69,6 +69,37 @@ function reduce(enumerable, callback, initialValue) { return previousValue; } +/** + * @param {Array} array of key1,value1,key2,value2,... + * @return {Object} {key1:value1, key2:value2, ...} + */ +function kvArrayToObject(array) { + const obj = {}; + for (let i = 0; i < array.length; i+=2) { + let [key, value] = [array[i], array[i+1]]; + obj[key] = value; + } + return obj; +} + +function objectToSortedKVArray(obj) { + const keys = Object.keys(obj).sort(); + const result = []; + keys.forEach(k => { + result.push(k); + result.push(obj[k]); + }); + return result; +} + +function isArrayEqual(array1, array2) { + if (array1.length !== array2.length) { return false; } + for (let i=0; i < array1.length; i++) { + if (array1[i] !== array2[i]) { return false; } + } + return true; +} + export { detect, forEach, @@ -76,5 +107,8 @@ export { filter, commonItemLength, compact, - reduce + reduce, + isArrayEqual, + objectToSortedKVArray, + kvArrayToObject }; diff --git a/src/js/utils/dom-utils.js b/src/js/utils/dom-utils.js index 3aed59729..5ce1affc7 100644 --- a/src/js/utils/dom-utils.js +++ b/src/js/utils/dom-utils.js @@ -84,14 +84,23 @@ function containsNode(parentNode, childNode) { /** * converts the element's NamedNodeMap of attrs into * an object with key-value pairs - * FIXME should add a whitelist as a second arg + * @param {DOMNode} element + * @param {Array} whitelist optional, an array of attributes to constrain to. + * If not passed (or empty), all attributes are allowed. + * @return {Object} key-value pairs */ -function getAttributes(element) { - let result = {}; +function getAttributes(element, whitelist=[]) { + const allowed = attrName => { + return whitelist.length === 0 ? true : whitelist.indexOf(attrName) !== -1; + }; + const result = {}; if (element.hasAttributes()) { - let attributes = element.attributes; - - forEach(attributes, ({name,value}) => result[name] = value); + const attributes = element.attributes; + forEach(attributes, ({name,value}) => { + if (allowed(name)) { + result[name] = value; + } + }); } return result; } diff --git a/tests/unit/models/markup-section-test.js b/tests/unit/models/markup-section-test.js index 8b72d8cd8..da47dbbc8 100644 --- a/tests/unit/models/markup-section-test.js +++ b/tests/unit/models/markup-section-test.js @@ -20,13 +20,14 @@ test('a section can append a marker', (assert) => { assert.equal(s1.markers.length, 1); }); +// FIXME move to unit postnode builder test test('postNodeBuilder#createMarkup returns a singleton object when it has no attributes', (assert) => { let markup = builder.createMarkup('b'); let others = [ builder.createMarkup('B'), - builder.createMarkup('B', []), - builder.createMarkup('b', []) + builder.createMarkup('B', {}), + builder.createMarkup('b', {}) ]; others.forEach(other => { diff --git a/tests/unit/models/post-node-builder-test.js b/tests/unit/models/post-node-builder-test.js new file mode 100644 index 000000000..bef7e4ed1 --- /dev/null +++ b/tests/unit/models/post-node-builder-test.js @@ -0,0 +1,30 @@ +import Helpers from '../../test-helpers'; +import PostNodeBuilder from 'content-kit-editor/models/post-node-builder'; + +const {module, test} = Helpers; + +module('Unit: PostNodeBuilder'); + +test('#createMarkup returns singleton markup', (assert) => { + const builder = new PostNodeBuilder(); + const m1 = builder.createMarkup('strong'); + const m2 = builder.createMarkup('strong'); + + assert.ok(m1 === m2, 'markups are singletons'); +}); + +test('#createMarkup returns singleton markup when has equal attributes', (assert) => { + const builder = new PostNodeBuilder(); + const m1 = builder.createMarkup('a', {href:'bustle.com'}); + const m2 = builder.createMarkup('a', {href:'bustle.com'}); + + assert.ok(m1 === m2, 'markups with attributes are singletons'); +}); + +test('#createMarkup returns differents markups when has different attributes', (assert) => { + const builder = new PostNodeBuilder(); + const m1 = builder.createMarkup('a', {href:'bustle.com'}); + const m2 = builder.createMarkup('a', {href:'other.com'}); + + assert.ok(m1 !== m2, 'markups with different attributes are different'); +}); diff --git a/tests/unit/models/post-test.js b/tests/unit/models/post-test.js index 54d06a1dc..2a0b634d3 100644 --- a/tests/unit/models/post-test.js +++ b/tests/unit/models/post-test.js @@ -104,8 +104,8 @@ test('#markupsInRange returns all markups', (assert) => { b = markup('strong'); i = markup('em'); - a1 = markup('a', ['href', 'example.com']); - a2 = markup('a', ['href', 'other-example.com']); + a1 = markup('a', {href:'example.com'}); + a2 = markup('a', {href:'other-example.com'}); return post([ markupSection('p', [ diff --git a/tests/unit/parsers/dom-test.js b/tests/unit/parsers/dom-test.js index b6c7f2091..80210a2d3 100644 --- a/tests/unit/parsers/dom-test.js +++ b/tests/unit/parsers/dom-test.js @@ -149,7 +149,7 @@ test('a tag (stray markup) without a block should create a block', (assert) => { let expectedFirst = builder.createMarkupSection('P', [], true); expectedFirst.markers.append(builder.createMarker('text', [ - builder.createMarkup('A', ['href', url]) + builder.createMarkup('A', {href:url}) ])); expectedPost.sections.append(expectedFirst); @@ -352,7 +352,7 @@ test('attributes', (assert) => { let expectedFirst = builder.createMarkupSection('P'); expectedFirst.markers.append(builder.createMarker('Link to google.com', [ - builder.createMarkup('A', ['href', href, 'rel', rel]) + builder.createMarkup('A', {href, rel}) ])); expectedPost.sections.append(expectedFirst); diff --git a/tests/unit/parsers/mobiledoc-test.js b/tests/unit/parsers/mobiledoc-test.js index c5546bcd2..f58543cd3 100644 --- a/tests/unit/parsers/mobiledoc-test.js +++ b/tests/unit/parsers/mobiledoc-test.js @@ -107,7 +107,7 @@ test('#parse doc with marker type', (assert) => { const parsed = parser.parse(mobiledoc); let section = builder.createMarkupSection('P', [], false); - let aMarkerType = builder.createMarkup('A', ['href', 'google.com']); + let aMarkerType = builder.createMarkup('A', {href:'google.com'}); let bMarkerType = builder.createMarkup('B'); let markers = [ diff --git a/tests/unit/renderers/mobiledoc-test.js b/tests/unit/renderers/mobiledoc-test.js index 5c3acab4c..d148b425d 100644 --- a/tests/unit/renderers/mobiledoc-test.js +++ b/tests/unit/renderers/mobiledoc-test.js @@ -4,8 +4,9 @@ import { } from 'content-kit-editor/renderers/mobiledoc'; import PostNodeBuilder from 'content-kit-editor/models/post-node-builder'; import { normalizeTagName } from 'content-kit-editor/utils/dom-utils'; +import Helpers from '../../test-helpers'; -const { module, test } = window.QUnit; +const { module, test } = Helpers; const render = MobiledocRenderer.render; let builder; @@ -25,62 +26,79 @@ test('renders a blank post', (assert) => { }); test('renders a post with marker', (assert) => { - let post = builder.createPost(); - let section = builder.createMarkupSection('P'); - post.sections.append(section); - section.markers.append( - builder.createMarker('Hi', [ - builder.createMarkup('STRONG') - ]) - ); - let mobiledoc = render(post); + const post = Helpers.postAbstract.build(({post, markupSection, marker, markup}) => { + return post([ + markupSection('p', [marker('Hi', [markup('strong')])]) + ]); + }); + const mobiledoc = render(post); assert.deepEqual(mobiledoc, { version: MOBILEDOC_VERSION, sections: [ + [['strong']], [ - ['strong'] - ], + [1, normalizeTagName('P'), [[[0], 1, 'Hi']]] + ] + ] + }); +}); + +test('renders a post section with markers sharing a markup', (assert) => { + const post = Helpers.postAbstract.build(({post, markupSection, marker, markup}) => { + const strong = markup('strong'); + return post([ + markupSection('p', [marker('Hi', [strong]), marker(' Guy', [strong])]) + ]); + }); + let mobiledoc = render(post); + assert.deepEqual(mobiledoc, { + version: MOBILEDOC_VERSION, + sections: [ + [['strong']], [ [1, normalizeTagName('P'), [ - [[0], 1, 'Hi'] + [[0], 0, 'Hi'], + [[], 1, ' Guy'] ]] ] ] }); }); -test('renders a post section with markers sharing a markup', (assert) => { - let post = builder.createPost(); - let section = builder.createMarkupSection('P'); - post.sections.append(section); - let markup = builder.createMarkup('STRONG'); - section.markers.append( - builder.createMarker('Hi', [ - markup - ]) - ); - section.markers.append( - builder.createMarker(' Guy', [ - markup - ]) - ); +test('renders a post with markers with markers with complex attributes', (assert) => { + let link1,link2; + const post = Helpers.postAbstract.build(({post, markupSection, marker, markup}) => { + link1 = markup('a', {href:'bustle.com'}); + link2 = markup('a', {href:'other.com'}); + return post([ + markupSection('p', [ + marker('Hi', [link1]), + marker(' Guy', [link2]), + marker(' other guy', [link1]) + ]) + ]); + }); let mobiledoc = render(post); assert.deepEqual(mobiledoc, { version: MOBILEDOC_VERSION, sections: [ [ - ['strong'] + ['a', ['href', 'bustle.com']], + ['a', ['href', 'other.com']] ], [ [1, normalizeTagName('P'), [ - [[0], 0, 'Hi'], - [[], 1, ' Guy'] + [[0], 1, 'Hi'], + [[1], 1, ' Guy'], + [[0], 1, ' other guy'] ]] ] ] }); + }); + test('renders a post with image', (assert) => { let url = "data:image/gif;base64,R0lGODlhAQABAIAAAP///wAAACwAAAAAAQABAAACAkQBADs="; let post = builder.createPost(); diff --git a/tests/unit/utils/array-utils-test.js b/tests/unit/utils/array-utils-test.js new file mode 100644 index 000000000..245e43aaa --- /dev/null +++ b/tests/unit/utils/array-utils-test.js @@ -0,0 +1,12 @@ +import Helpers from '../../test-helpers'; +import { objectToSortedKVArray } from 'content-kit-editor/utils/array-utils'; + +const {module, test} = Helpers; + +module('Unit: Utils: Array Utils'); + +test('#objectToSortedKVArray gives proper results', (assert) => { + assert.deepEqual(objectToSortedKVArray({a: 1, b:2}), ['a', 1, 'b', 2]); + assert.deepEqual(objectToSortedKVArray({b: 1, a:2}), ['a', 2, 'b', 1]); + assert.deepEqual(objectToSortedKVArray({}), []); +});