Merge branch 'IhorKaleniuk666-fix/id-conflict' into dev

Author: artf

packages/core/src/dom_components/config/config.ts

@@ -60,6 +60,14 @@ export interface DomComponentsConfig {
    * work properly (eg. Web Components).
    */
   useFrameDoc?: boolean;
+
+  /**
+   * Experimental!
+   * By default, the editor ensures unique attribute IDs for all components inside the project, no matter of the page.
+   * With this option enabled, the editor will keep same attributes IDs for components across different pages.
+   * This allows multiple components cross pages (eg. <footer id="footer"/>) to share the same ID (eg. to keep the same styling).
+   */
+  keepAttributeIdsCrossPages?: boolean;
 }
 
 const config: () => DomComponentsConfig = () => ({

packages/core/src/dom_components/model/Component.ts

@@ -72,6 +72,11 @@ import {
 export interface IComponent extends ExtractMethods<Component> {}
 export interface SetAttrOptions extends SetOptions, UpdateStyleOptions, DataWatchersOptions {}
 export interface ComponentSetOptions extends SetOptions, DataWatchersOptions {}
+export interface CheckIdOptions {
+  keepIds?: string[];
+  idMap?: PrevToNewIdMap;
+  updatedIds?: Record<string, ComponentDefinitionDefined[]>;
+}
 
 const escapeRegExp = (str: string) => {
   return str.replace(/[|\\{}()[\]^$+*?.]/g, '\\$&');
@@ -316,7 +321,7 @@ export default class Component extends StyleableModel<ComponentProperties> {
     };
     const attrs = this.dataResolverWatchers.getValueOrResolver('attributes', defaultAttrs);
     this.setAttributes(attrs);
-    this.ccid = Component.createId(this, opt);
+    this.ccid = Component.createId(this, opt as any);
     this.preInit();
     this.initClasses();
     this.initComponents();
@@ -2062,43 +2067,77 @@ export default class Component extends StyleableModel<ComponentProperties> {
 
   static ensureInList(model: Component) {
     const list = Component.getList(model);
-    const id = model.getId();
+    const propId = model.id as string | undefined;
+    const id = propId || model.getId();
     const current = list[id];
 
     if (!current) {
-      // Insert in list
       list[id] = model;
     } else if (current !== model) {
-      // Create new ID
+      const keepIdsCrossPages = model.em?.Components.config.keepAttributeIdsCrossPages;
+      const currentPage = current.page;
+      const modelPage = model.page;
+      const samePage = !!currentPage && !!modelPage && currentPage === modelPage;
       const nextId = Component.getIncrementId(id, list);
-      model.setId(nextId);
+
+      if (samePage || !keepIdsCrossPages) {
+        model.setId(nextId);
+      } else {
+        model.set({ id: nextId });
+      }
+
       list[nextId] = model;
     }
 
     model.components().forEach((i) => Component.ensureInList(i));
   }
 
-  static createId(model: Component, opts: any = {}) {
+  static createId(model: Component, opts: CheckIdOptions = {}) {
     const list = Component.getList(model);
+    const keepIdsCrossPages = model.em?.Components.config.keepAttributeIdsCrossPages;
     const { idMap = {} } = opts;
-    let { id } = model.get('attributes')!;
-    let nextId;
+    const attrs = model.get('attributes') || {};
+    const attrId = attrs.id as string | undefined;
+    const propId = model.id as string | undefined;
+    const currentId = propId ?? attrId;
+    let nextId: string;
+
+    if (propId) {
+      nextId = Component.getIncrementId(propId, list, opts);
+      if (nextId !== propId) {
+        model.set({ id: nextId });
+      }
+    } else if (attrId) {
+      const existing = list[attrId] as Component | undefined;
 
-    if (id) {
-      nextId = Component.getIncrementId(id, list, opts);
-      model.setId(nextId);
-      if (id !== nextId) idMap[id] = nextId;
+      if (!existing || existing === model) {
+        nextId = attrId;
+      } else {
+        const existingPage = existing.page;
+        const newPage = model.page;
+        const samePage = !!existingPage && !!newPage && existingPage === newPage;
+        nextId = Component.getIncrementId(attrId, list, opts);
+
+        if (samePage || !keepIdsCrossPages) {
+          model.setId(nextId);
+        } else {
+          model.set({ id: nextId });
+        }
+      }
     } else {
       nextId = Component.getNewId(list);
     }
 
+    if (!!currentId && currentId !== nextId) {
+      idMap[currentId] = nextId;
+    }
+
     list[nextId] = model;
     return nextId;
   }
 
   static getNewId(list: ObjectAny) {
     const count = Object.keys(list).length;
-    // Testing 1000000 components with `+ 2` returns 0 collisions
     const ilen = count.toString().length + 2;
     const uid = (Math.random() + 1.1).toString(36).slice(-ilen);
     let newId = `i${uid}`;
@@ -2126,20 +2165,14 @@ export default class Component extends StyleableModel<ComponentProperties> {
   }
 
   static getList(model: Component) {
-    const { em } = model;
-    const dm = em?.Components;
-    return dm?.componentsById ?? {};
+    return model.em?.Components?.componentsById ?? {};
   }
 
   static checkId(
     components: ComponentDefinitionDefined | ComponentDefinitionDefined[],
     styles: CssRuleJSON[] = [],
     list: ObjectAny = {},
-    opts: {
-      keepIds?: string[];
-      idMap?: PrevToNewIdMap;
-      updatedIds?: Record<string, ComponentDefinitionDefined[]>;
-    } = {},
+    opts: CheckIdOptions = {},
   ) {
     opts.updatedIds = opts.updatedIds || {};
     const comps = isArray(components) ? components : [components];

packages/core/test/specs/pages/pages-same-id-on-different-pages.ts

@@ -0,0 +1,158 @@
+import Editor from '../../../src/editor';
+import EditorModel from '../../../src/editor/model/Editor';
+import ComponentWrapper from '../../../src/dom_components/model/ComponentWrapper';
+import { setupTestEditor } from '../../common';
+
+describe('Pages with same component ids across pages', () => {
+  let editor: Editor;
+  let em: EditorModel;
+  let pm: Editor['Pages'];
+  let domc: Editor['Components'];
+  const rootDefaultProps = {
+    type: 'wrapper',
+    head: { type: 'head' },
+    docEl: { tagName: 'html' },
+    stylable: [
+      'background',
+      'background-color',
+      'background-image',
+      'background-repeat',
+      'background-attachment',
+      'background-position',
+      'background-size',
+    ],
+  };
+
+  const getTitle = (wrapper: ComponentWrapper) => wrapper.components().at(0);
+
+  const getRootComponent = ({ idBody = 'body', idTitle = 'main-title', contentTitle = 'A' } = {}) => ({
+    type: 'wrapper',
+    attributes: { id: idBody },
+    components: [
+      {
+        tagName: 'h1',
+        type: 'text',
+        attributes: { id: idTitle },
+        components: [{ type: 'textnode', content: contentTitle }],
+      },
+    ],
+  });
+
+  beforeEach(() => {
+    ({ editor } = setupTestEditor());
+    em = editor.getModel();
+    pm = em.Pages;
+    domc = em.Components;
+  });
+
+  afterEach(() => {
+    editor.destroy();
+  });
+
+  test('Default behavior with pages having components with same ids are incremented', () => {
+    editor.Pages.add({
+      id: 'page1',
+      frames: [{ component: getRootComponent() }],
+    });
+    editor.Pages.add({
+      id: 'page2',
+      frames: [{ component: getRootComponent({ contentTitle: 'B' }) }],
+    });
+
+    const root1 = pm.get('page1')!.getMainComponent();
+    const root2 = pm.get('page2')!.getMainComponent();
+
+    expect(editor.getHtml({ component: root1 })).toBe('<body id="body"><h1 id="main-title">A</h1></body>');
+    expect(editor.getHtml({ component: root2 })).toBe('<body id="body-2"><h1 id="main-title-2">B</h1></body>');
+
+    expect(JSON.parse(JSON.stringify(root1))).toEqual({
+      ...rootDefaultProps,
+      attributes: { id: 'body' },
+      components: [
+        {
+          tagName: 'h1',
+          type: 'text',
+          attributes: { id: 'main-title' },
+          components: [{ type: 'textnode', content: 'A' }],
+        },
+      ],
+    });
+
+    expect(JSON.parse(JSON.stringify(root2))).toEqual({
+      ...rootDefaultProps,
+      attributes: { id: 'body-2' },
+      components: [
+        {
+          tagName: 'h1',
+          type: 'text',
+          attributes: { id: 'main-title-2' },
+          components: [{ type: 'textnode', content: 'B' }],
+        },
+      ],
+    });
+  });
+
+  test('Handles pages with components having the same id across pages', () => {
+    editor.Components.config.keepAttributeIdsCrossPages = true;
+    editor.Pages.add({
+      id: 'page1',
+      frames: [{ component: getRootComponent() }],
+    });
+    editor.Pages.add({
+      id: 'page2',
+      frames: [{ component: getRootComponent({ contentTitle: 'B' }) }],
+    });
+    const page1 = pm.get('page1')!;
+    const page2 = pm.get('page2')!;
+    const root1 = page1.getMainComponent();
+    const root2 = page2.getMainComponent();
+
+    expect(root1.getId()).toBe('body');
+    expect(root2.getId()).toBe('body');
+
+    const title1 = getTitle(root1);
+    const title2 = getTitle(root2);
+
+    // IDs should be preserved per page but stored uniquely in the shared map
+    expect(title1.getId()).toBe('main-title');
+    expect(title2.getId()).toBe('main-title');
+
+    const all = domc.allById();
+
+    expect(all['body']).toBe(root1);
+    expect(all['body-2']).toBe(root2);
+    expect(all['main-title']).toBe(title1);
+    expect(all['main-title-2']).toBe(title2);
+
+    expect(editor.getHtml({ component: root1 })).toBe('<body id="body"><h1 id="main-title">A</h1></body>');
+    expect(editor.getHtml({ component: root2 })).toBe('<body id="body"><h1 id="main-title">B</h1></body>');
+
+    expect(JSON.parse(JSON.stringify(root1))).toEqual({
+      ...rootDefaultProps,
+      attributes: { id: 'body' },
+      components: [
+        {
+          tagName: 'h1',
+          type: 'text',
+          attributes: { id: 'main-title' },
+          components: [{ type: 'textnode', content: 'A' }],
+        },
+      ],
+    });
+
+    expect(JSON.parse(JSON.stringify(root2))).toEqual({
+      ...rootDefaultProps,
+      id: 'body-2',
+      attributes: { id: 'body' },
+      components: [
+        {
+          id: 'main-title-2',
+          tagName: 'h1',
+          type: 'text',
+          attributes: { id: 'main-title' },
+          components: [{ type: 'textnode', content: 'B' }],
+        },
+      ],
+    });
+  });
+});