From 12897993bd7e4d58fa5769a5257ab44bae8c2d6d Mon Sep 17 00:00:00 2001 From: Artur Arseniev Date: Thu, 31 Jan 2019 23:50:38 +0100 Subject: [PATCH 1/3] Avoid duplicate IDs --- src/dom_components/model/Component.js | 64 ++++++++++++++++++-- src/dom_components/model/Components.js | 3 + test/specs/dom_components/model/Component.js | 53 ++++++++++++++++ test/specs/grapesjs/index.js | 3 +- 4 files changed, 118 insertions(+), 5 deletions(-) diff --git a/src/dom_components/model/Component.js b/src/dom_components/model/Component.js index 03cb3b082..41bfdc84c 100644 --- a/src/dom_components/model/Component.js +++ b/src/dom_components/model/Component.js @@ -169,11 +169,11 @@ const Component = Backbone.Model.extend(Styleable).extend( this.opt = opt; this.em = em; this.config = opt.config || {}; - this.ccid = Component.createId(this); this.set('attributes', { ...(this.defaults.attributes || {}), ...(this.get('attributes') || {}) }); + this.ccid = Component.createId(this); this.initClasses(); this.initTraits(); this.initComponents(); @@ -1037,17 +1037,73 @@ const Component = Backbone.Model.extend(Styleable).extend( * @private */ createId(model) { + if (window.stoop) debugger; + let { id } = model.get('attributes'); + let nextId; + + if (id) { + nextId = Component.getIncrementId(id); + model.setId(nextId); + } else { + nextId = Component.getNewId(); + } + + componentList[nextId] = model; + return nextId; + }, + + getNewId() { componentIndex++; // Testing 1000000 components with `+ 2` returns 0 collisions const ilen = componentIndex.toString().length + 2; const uid = (Math.random() + 1.1).toString(36).slice(-ilen); - const nextId = 'i' + uid; - componentList[nextId] = model; - return nextId; + let newId = `i${uid}`; + while (componentList[newId]) newId = Component.getNewId(); + + return newId; + }, + + getIncrementId(id) { + let counter = 1; + let newId = id; + + while (componentList[newId]) { + counter++; + newId = `${id}-${counter}`; + } + + return newId; }, getList() { return componentList; + }, + + /** + * This method checks, for each parsed component and style object + * (are not Components/CSSRules yet), for duplicated id and fixes them + * + */ + checkId(components, styles = []) { + const comps = isArray(components) ? components : [components]; + comps.forEach(comp => { + const { attributes = {} } = comp; + const { id } = attributes; + + // Check if we have collisions with current components + if (id && componentList[id]) { + const newId = Component.getIncrementId(id); + attributes.id = newId; + // Update passed styles + styles && + styles.forEach(style => { + const { selectors } = style; + selectors.forEach((sel, idx) => { + if (sel === `#${id}`) selectors[idx] = `#${newId}`; + }); + }); + } + }); } } ); diff --git a/src/dom_components/model/Components.js b/src/dom_components/model/Components.js index 92943fb23..d1d732fac 100644 --- a/src/dom_components/model/Components.js +++ b/src/dom_components/model/Components.js @@ -1,6 +1,7 @@ import { isEmpty, isArray, isString } from 'underscore'; const Backbone = require('backbone'); +let Component; module.exports = Backbone.Collection.extend({ initialize(models, opt = {}) { @@ -36,6 +37,8 @@ module.exports = Backbone.Collection.extend({ const { em } = this; const cssc = em.get('CssComposer'); const parsed = em.get('Parser').parseHtml(value); + if (!Component) Component = require('./Component'); + Component.checkId(parsed.html, parsed.css); if (parsed.css && cssc && !opt.temporary) { cssc.addCollection(parsed.css, { diff --git a/test/specs/dom_components/model/Component.js b/test/specs/dom_components/model/Component.js index 7bb5331fb..cae379676 100644 --- a/test/specs/dom_components/model/Component.js +++ b/test/specs/dom_components/model/Component.js @@ -526,6 +526,7 @@ module.exports = { describe('Components', () => { beforeEach(() => { + em = new Editor({}); dcomp = new DomComponents(); compOpts = { componentTypes: dcomp.componentTypes @@ -549,6 +550,58 @@ module.exports = { var m = c.add({ type: 'text' }); expect(m instanceof ComponentText).toEqual(true); }); + + test('Avoid conflicting components with the same ID', () => { + const em = new Editor({}); + dcomp = new DomComponents(); + dcomp.init({ em }); + const id = 'myid'; + const block = ` +
Test
+ + `; + const added = dcomp.addComponent(block); + expect(dcomp.getComponents().length).toBe(1); + expect(added.getId()).toBe(id); + const cc = em.get('CssComposer'); + expect(cc.getAll().length).toBe(2); + expect( + cc + .getAll() + .at(0) + .selectorsToString() + ).toBe(`#${id}`); + expect( + cc + .getAll() + .at(1) + .selectorsToString() + ).toBe(`#${id}:hover`); + const added2 = dcomp.addComponent(block); + const id2 = added2.getId(); + const newId = `${id}-2`; + expect(id2).toBe(newId); + expect(cc.getAll().length).toBe(4); + expect( + cc + .getAll() + .at(2) + .selectorsToString() + ).toBe(`#${newId}`); + expect( + cc + .getAll() + .at(3) + .selectorsToString() + ).toBe(`#${newId}:hover`); + }); }); } }; diff --git a/test/specs/grapesjs/index.js b/test/specs/grapesjs/index.js index 48825694e..7517fe690 100644 --- a/test/specs/grapesjs/index.js +++ b/test/specs/grapesjs/index.js @@ -450,7 +450,7 @@ describe('GrapesJS', () => { expect(css).toEqual(`${protCss}.test2{color:red;}.test3{color:blue;}`); }); - describe('Component selection', () => { + describe.only('Component selection', () => { let editor, wrapper, el1, el2, el3; beforeEach(() => { @@ -465,6 +465,7 @@ describe('GrapesJS', () => { el1 = wrapper.find('#el1')[0]; el2 = wrapper.find('#el2')[0]; el3 = wrapper.find('#el3')[0]; + // console.log('wrapper', wrapper.getEl().innerHTML); }); test('Select a single component', () => { From 60cb2846031ef38cc45aaca66b91fd1d677b400d Mon Sep 17 00:00:00 2001 From: Artur Arseniev Date: Fri, 1 Feb 2019 00:38:56 +0100 Subject: [PATCH 2/3] Change the container of components by IDs --- src/dom_components/index.js | 6 ++- src/dom_components/model/Component.js | 41 ++++++++++++-------- src/dom_components/model/Components.js | 5 ++- test/specs/dom_components/model/Component.js | 3 +- test/specs/grapesjs/index.js | 2 +- 5 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/dom_components/index.js b/src/dom_components/index.js index b4732b541..c13b18763 100644 --- a/src/dom_components/index.js +++ b/src/dom_components/index.js @@ -38,6 +38,7 @@ module.exports = () => { const ComponentView = require('./view/ComponentView'); const Components = require('./model/Components'); const ComponentsView = require('./view/ComponentsView'); + const componentsById = {}; var component, componentView; var componentTypes = [ @@ -137,6 +138,8 @@ module.exports = () => { componentTypes, + componentsById, + /** * Name of the module * @type {String} @@ -231,7 +234,8 @@ module.exports = () => { component = new Component(wrapper, { em, config: c, - componentTypes + componentTypes, + domc: this }); component.set({ attributes: { id: 'wrapper' } }); diff --git a/src/dom_components/model/Component.js b/src/dom_components/model/Component.js index 41bfdc84c..994610f43 100644 --- a/src/dom_components/model/Component.js +++ b/src/dom_components/model/Component.js @@ -1037,37 +1037,40 @@ const Component = Backbone.Model.extend(Styleable).extend( * @private */ createId(model) { - if (window.stoop) debugger; + const list = Component.getList(model); let { id } = model.get('attributes'); let nextId; if (id) { - nextId = Component.getIncrementId(id); + nextId = Component.getIncrementId(id, list); model.setId(nextId); } else { - nextId = Component.getNewId(); + nextId = Component.getNewId(list); } - componentList[nextId] = model; + list[nextId] = model; return nextId; }, - getNewId() { - componentIndex++; + getNewId(list) { + const count = Object.keys(list).length; // Testing 1000000 components with `+ 2` returns 0 collisions - const ilen = componentIndex.toString().length + 2; + const ilen = count.toString().length + 2; const uid = (Math.random() + 1.1).toString(36).slice(-ilen); let newId = `i${uid}`; - while (componentList[newId]) newId = Component.getNewId(); + + while (list[newId]) { + newId = Component.getNewId(list); + } return newId; }, - getIncrementId(id) { + getIncrementId(id, list) { let counter = 1; let newId = id; - while (componentList[newId]) { + while (list[newId]) { counter++; newId = `${id}-${counter}`; } @@ -1075,8 +1078,14 @@ const Component = Backbone.Model.extend(Styleable).extend( return newId; }, - getList() { - return componentList; + /** + * The list of components is taken from the Components module. + * Initially, the list, was set statically on the Component object but it was + * not ok, as it was shared between multiple editor instances + */ + getList(model) { + const domc = model.opt && model.opt.domc; + return domc ? domc.componentsById : {}; }, /** @@ -1084,18 +1093,18 @@ const Component = Backbone.Model.extend(Styleable).extend( * (are not Components/CSSRules yet), for duplicated id and fixes them * */ - checkId(components, styles = []) { + checkId(components, styles = [], list = {}) { const comps = isArray(components) ? components : [components]; comps.forEach(comp => { const { attributes = {} } = comp; const { id } = attributes; // Check if we have collisions with current components - if (id && componentList[id]) { - const newId = Component.getIncrementId(id); + if (id && list[id]) { + const newId = Component.getIncrementId(id, list); attributes.id = newId; // Update passed styles - styles && + isArray(styles) && styles.forEach(style => { const { selectors } = style; selectors.forEach((sel, idx) => { diff --git a/src/dom_components/model/Components.js b/src/dom_components/model/Components.js index d1d732fac..1a873be8a 100644 --- a/src/dom_components/model/Components.js +++ b/src/dom_components/model/Components.js @@ -5,6 +5,7 @@ let Component; module.exports = Backbone.Collection.extend({ initialize(models, opt = {}) { + this.opt = opt; this.listenTo(this, 'add', this.onAdd); this.config = opt.config; this.em = opt.em; @@ -15,6 +16,7 @@ module.exports = Backbone.Collection.extend({ options.em = opt.em; options.config = opt.config; options.componentTypes = df; + options.domc = opt.domc; for (var it = 0; it < df.length; it++) { var dfId = df[it].id; @@ -37,8 +39,9 @@ module.exports = Backbone.Collection.extend({ const { em } = this; const cssc = em.get('CssComposer'); const parsed = em.get('Parser').parseHtml(value); + // We need this to avoid duplicate IDs if (!Component) Component = require('./Component'); - Component.checkId(parsed.html, parsed.css); + Component.checkId(parsed.html, parsed.css, this.opt.domc.componentsById); if (parsed.css && cssc && !opt.temporary) { cssc.addCollection(parsed.css, { diff --git a/test/specs/dom_components/model/Component.js b/test/specs/dom_components/model/Component.js index cae379676..c92c57f94 100644 --- a/test/specs/dom_components/model/Component.js +++ b/test/specs/dom_components/model/Component.js @@ -24,7 +24,8 @@ module.exports = { dcomp = new DomComponents(); compOpts = { em, - componentTypes: dcomp.componentTypes + componentTypes: dcomp.componentTypes, + domc: dcomp }; obj = new Component({}, compOpts); }); diff --git a/test/specs/grapesjs/index.js b/test/specs/grapesjs/index.js index 7517fe690..4ff32d891 100644 --- a/test/specs/grapesjs/index.js +++ b/test/specs/grapesjs/index.js @@ -450,7 +450,7 @@ describe('GrapesJS', () => { expect(css).toEqual(`${protCss}.test2{color:red;}.test3{color:blue;}`); }); - describe.only('Component selection', () => { + describe('Component selection', () => { let editor, wrapper, el1, el2, el3; beforeEach(() => { From a4c2a50345aa3286b48ca7c03c0f33a6ae05efb8 Mon Sep 17 00:00:00 2001 From: Artur Arseniev Date: Fri, 1 Feb 2019 01:05:35 +0100 Subject: [PATCH 3/3] Avoid nested duplicate IDs --- src/dom_components/model/Component.js | 6 ++- test/specs/dom_components/model/Component.js | 45 +++++++++++++++++--- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/dom_components/model/Component.js b/src/dom_components/model/Component.js index 994610f43..997c69f6a 100644 --- a/src/dom_components/model/Component.js +++ b/src/dom_components/model/Component.js @@ -1091,12 +1091,12 @@ const Component = Backbone.Model.extend(Styleable).extend( /** * This method checks, for each parsed component and style object * (are not Components/CSSRules yet), for duplicated id and fixes them - * + * This method is used in Components.js just after the parsing */ checkId(components, styles = [], list = {}) { const comps = isArray(components) ? components : [components]; comps.forEach(comp => { - const { attributes = {} } = comp; + const { attributes = {}, components } = comp; const { id } = attributes; // Check if we have collisions with current components @@ -1112,6 +1112,8 @@ const Component = Backbone.Model.extend(Styleable).extend( }); }); } + + components && Component.checkId(components, styles, list); }); } } diff --git a/test/specs/dom_components/model/Component.js b/test/specs/dom_components/model/Component.js index c92c57f94..1483ef842 100644 --- a/test/specs/dom_components/model/Component.js +++ b/test/specs/dom_components/model/Component.js @@ -557,8 +557,11 @@ module.exports = { dcomp = new DomComponents(); dcomp.init({ em }); const id = 'myid'; + const idB = 'myid2'; const block = ` -
Test
+
+
+
`; const added = dcomp.addComponent(block); - expect(dcomp.getComponents().length).toBe(1); + // Let's check if everthing is working as expected + expect(Object.keys(dcomp.componentsById).length).toBe(3); // + 1 wrapper expect(added.getId()).toBe(id); + expect( + added + .components() + .at(0) + .getId() + ).toBe(idB); const cc = em.get('CssComposer'); - expect(cc.getAll().length).toBe(2); + expect(cc.getAll().length).toBe(3); expect( cc .getAll() @@ -585,23 +598,43 @@ module.exports = { .at(1) .selectorsToString() ).toBe(`#${id}:hover`); + expect( + cc + .getAll() + .at(2) + .selectorsToString() + ).toBe(`#${idB}`); + // Now let's add the same block const added2 = dcomp.addComponent(block); const id2 = added2.getId(); const newId = `${id}-2`; + const newIdB = `${idB}-2`; expect(id2).toBe(newId); - expect(cc.getAll().length).toBe(4); + expect( + added2 + .components() + .at(0) + .getId() + ).toBe(newIdB); + expect(cc.getAll().length).toBe(6); expect( cc .getAll() - .at(2) + .at(3) .selectorsToString() ).toBe(`#${newId}`); expect( cc .getAll() - .at(3) + .at(4) .selectorsToString() ).toBe(`#${newId}:hover`); + expect( + cc + .getAll() + .at(5) + .selectorsToString() + ).toBe(`#${newIdB}`); }); }); }