Repository: maxGraph/maxGraph
Pull request #383 (2024-04-09 12:05:45)

fix: correctly compute the style when setting undefined/none
Previously, when the value of a property was defined as `undefined`, it was also defined as `undefined` in the calculated style. This was a change from the mxGraph implementation and was not consistent with the style without the same property. 
This has now been corrected: when a value is set to `undefined`, the value calculated is that of the default style or a "base" style.

To match `mxGraph`'s behavior, the same logic applies when the value is set to "none".



Patch:
diff --git a/.github/ISSUE_TEMPLATE/01-bug-report.md b/.github/ISSUE_TEMPLATE/01-bug-report.md
new file mode 100644
index 000000000..191e8af66
--- /dev/null
+++ b/.github/ISSUE_TEMPLATE/01-bug-report.md
@@ -0,0 +1,44 @@
+---
+name: Bug report
+about: Create a report to help us improve
+title: "[BUG]"
+labels: bug, triage
+assignees: ''
+
+---
+
+### Describe the bug
+A clear and concise description of what the bug is.
+
+Please create a single issue for each bug. If you have multiple bugs, please create multiple issues.
+This will help us to track and manage the issues more effectively.
+
+
+### To Reproduce
+Steps to reproduce the behavior:
+1. Go to '...'
+2. Click on '....'
+3. Scroll down to '....'
+4. See error (if the error is taking from the browser console, please provide the error message in the text form, no image)
+
+If possible provide a repository to reproduce the issue. At minimum, provide the code (in text form, no image) that produces the issue.
+
+### Expected behavior
+A clear and concise description of what you expected to happen.
+
+
+### Screenshots
+If applicable, add screenshots or videos to help explain your problem.
+
+
+### Environment
+- Desktop or mobile:
+- OS and version: [e.g. iOS 14.1]
+- Browser and version: [e.g. chrome 118, safari 15]
+- Node/npm version (if applicable):
+- Used frameworks (if applicable):
+
+
+
+### Additional context
+Add any other context about the problem here.
diff --git a/.github/ISSUE_TEMPLATE/02-feature-request.md b/.github/ISSUE_TEMPLATE/02-feature-request.md
new file mode 100644
index 000000000..212c9e30f
--- /dev/null
+++ b/.github/ISSUE_TEMPLATE/02-feature-request.md
@@ -0,0 +1,24 @@
+---
+name: Feature request
+about: Suggest an idea for this project
+title: "[FEAT]"
+labels: enhancement, triage
+assignees: ''
+
+---
+
+### Is your feature request related to a problem? Please describe.
+A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
+
+Please create a single issue for each request. If you have multiple requests, please create multiple issues.
+This will help us to track and manage the issues more effectively.
+
+
+### Describe the solution you'd like
+A clear and concise description of what you want to happen.
+
+### Describe alternatives you've considered
+A clear and concise description of any alternative solutions or features you've considered.
+
+### Additional context
+Add any other context or screenshots about the feature request here.
diff --git a/.github/ISSUE_TEMPLATE/03-technical-request.md b/.github/ISSUE_TEMPLATE/03-technical-request.md
new file mode 100644
index 000000000..b19522984
--- /dev/null
+++ b/.github/ISSUE_TEMPLATE/03-technical-request.md
@@ -0,0 +1,22 @@
+---
+name: Technical request
+about: Suggest an refactoring, technical change, ... for this project
+title: "[CHORE]"
+labels: chore, triage
+assignees: ''
+
+---
+
+**Please first consider** starting discussion in the [development discussions](https://github.com/maxGraph/maxGraph/discussions/categories/development) prior submitting technical requests.
+
+### Is your technical request related to a problem? Please describe.
+A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
+
+### Describe the solution you'd like
+A clear and concise description of what you want to happen.
+
+### Describe alternatives you've considered
+A clear and concise description of any alternative solutions or features you've considered.
+
+### Additional context
+Add any other context or screenshots about the feature request here.
diff --git a/.github/ISSUE_TEMPLATE/README.md b/.github/ISSUE_TEMPLATE/README.md
new file mode 100644
index 000000000..91434d5e0
--- /dev/null
+++ b/.github/ISSUE_TEMPLATE/README.md
@@ -0,0 +1,16 @@
+# Issue Templates
+
+## Documentation
+
+[Syntax for issue forms (the top level properties)](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms)
+
+[Syntax for GitHub's form schema (the form body)](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-githubs-form-schema)
+
+[Configuring the template chooser (config.yml)](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/configuring-issue-templates-for-your-repository#configuring-the-template-chooser)
+
+https://help.github.com/en/github/building-a-strong-community/configuring-issue-templates-for-your-repository#configuring-the-template-chooser
+
+
+## Template Chooser Sorting
+
+The templates are sorted in the template chooser based on the filename, not on the form name. This is why we have prefixed the filenames with numbers.
diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml
new file mode 100644
index 000000000..b60680ceb
--- /dev/null
+++ b/.github/ISSUE_TEMPLATE/config.yml
@@ -0,0 +1,8 @@
+blank_issues_enabled: false
+contact_links:
+  - name: Community Support for usage questions
+    url: https://github.com/maxGraph/maxGraph/discussions/categories/development
+    about: Please ask and answer questions about maxGraph usage here.
+  - name: Development Support
+    url: https://github.com/maxGraph/maxGraph/discussions/categories/development
+    about: Please ask questions about maxGraph development here.
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
deleted file mode 100644
index 4f4f35948..000000000
--- a/.github/PULL_REQUEST_TEMPLATE.md
+++ /dev/null
@@ -1,23 +0,0 @@
-**Summary**
-<!--
-What existing issue does the pull request solve?
-Please provide enough information so that others can review your pull request
--->
-
-**Description for the changelog**
-<!--
-A short (one line) summary that describes the changes in this
-pull request for inclusion in the change log
-If this is a bug fix, your description should include "fixes #xxxx", or
-"closes #xxxx", where #xxxx is the issue number
--->
-
-**Other info**
-<!--
-Thanks for submitting a pull request!
-
-All contributions to this project are done under the terms of the Apache 2.0 license as stated in https://www.apache.org/licenses/LICENSE-2.0
-
-Please make sure you read github's contributing guidelines;
-https://docs.github.com/en/desktop/installing-and-configuring-github-desktop/getting-started-with-github-desktop#part-3-contributing-to-projects-with-github-desktop
--->
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
new file mode 100644
index 000000000..4bc7212e8
--- /dev/null
+++ b/.github/pull_request_template.md
@@ -0,0 +1,51 @@
+<!--
+👋 Hi, thanks for sending a Pull Request to maxGraph! 💖
+Please fill out all fields below and make sure each item is true and [x] checked.
+Otherwise we may not be able to review your PR.
+
+All contributions to this project are done under the terms of the Apache 2.0 license as stated in the LICENSE file located at the root of this repository.
+
+
+Note: core contributors are not required to use this template.
+-->
+
+## PR Checklist
+
+- [ ] Addresses an existing open issue: closes #<the_issue_number_here>. If not, explain why (minor changes, etc.).
+- [ ] You have discussed this issue with the maintainers of `maxGraph`, and you are assigned to the issue.
+- [ ] The scope of the PR is small enough to review in a single session. PR targeting several issues should be split into separate PRs. Don't create large PR like https://github.com/maxGraph/maxGraph/pull/88.
+- [ ] The PR title follows the ["Conventional Commits"](https://www.conventionalcommits.org/en/v1.0.0/) guidelines.
+
+<!--
+The PR title must look like `<type>[optional scope]: <lower case description>`
+
+*type* can be (see existing Pull Request for more elements):
+- chore
+- docs
+- feat
+- fix
+- refactor
+  ...
+
+If defined, the _optional scope_ must be put in parentheses.
+
+Note: The title is used as proposal for the maintainer merging the Pull Request.
+-->
+
+
+## Overview
+
+<!-- Description of what and why is changed, and how the code change does that. -->
+- Explain the **details** for making this change: What existing problem does the Pull Request solve? Why is this feature beneficial?
+- If the Pull Request includes breaking changes, it must explicitly follow the syntax proposed by [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#specification) (in the PR title and with a quote at this end of this overview).
+- Includes keywords to reference the issue, e.g., "fixes #xxxx", "closes #xxxx", ... (where #xxxx is the issue number) and automatically closes it when the Pull Request is merged.
+For more details about linking issues in Pull Requests, see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue.
+- Also include a short description of the changes in this Pull Request to be used as merge commit when merging the PR.
+-->
+
+
+## Notes
+
+<!-- Use this paragraph to provide the reviewer with any additional information. Remove if not applicable -->
+
+
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index d3f0bc68b..a544f9f76 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -37,7 +37,7 @@ jobs:
       # we want to run the full build on all os: don't cancel running jobs even if one fails
       fail-fast: false
       matrix:
-        os: ['ubuntu-22.04', 'macos-13', 'windows-2022']
+        os: ['ubuntu-22.04', 'macos-14', 'windows-2022']
     steps:
       - uses: actions/checkout@v4
       - name: Build Setup
diff --git a/packages/core/src/serialization/ModelXmlSerializer.ts b/packages/core/src/serialization/ModelXmlSerializer.ts
index 770a1b9ec..135e0eea6 100644
--- a/packages/core/src/serialization/ModelXmlSerializer.ts
+++ b/packages/core/src/serialization/ModelXmlSerializer.ts
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { registerCoreCodecs } from './register';
+import { registerModelCodecs } from './register';
 import { getPrettyXml, parseXml } from '../util/xmlUtils';
 import { Codec } from '../index';
 import type GraphDataModel from '../view/GraphDataModel';
@@ -68,6 +68,6 @@ export class ModelXmlSerializer {
    * Hook for replacing codecs registered by default (core codecs).
    */
   protected registerCodecs(): void {
-    registerCoreCodecs();
+    registerModelCodecs();
   }
 }
diff --git a/packages/core/src/serialization/register.ts b/packages/core/src/serialization/register.ts
index 9dd7b4c8a..ab342345a 100644
--- a/packages/core/src/serialization/register.ts
+++ b/packages/core/src/serialization/register.ts
@@ -69,25 +69,18 @@ const createObjectCodec = (template: any, name: string): ObjectCodec => {
   return objectCodec;
 };
 
-let isCoreCodecsRegistered = false;
-
+let isModelCodecsRegistered = false;
 /**
- * Register core editor i.e. codecs that don't relate to editor.
+ * Register model codecs i.e. codecs used to import/export the Graph Model, see {@link GraphDataModel}.
  *
  * @param force if `true` register the codecs even if they were already registered. If false, only register them
  *              if they have never been registered before.
- * @since 0.6.0
+ * @since 0.10.0
  */
-export const registerCoreCodecs = (force = false) => {
-  if (!isCoreCodecsRegistered || force) {
+export const registerModelCodecs = (force = false) => {
+  if (!isModelCodecsRegistered || force) {
     CodecRegistry.register(new CellCodec());
-    CodecRegistry.register(new ChildChangeCodec());
-    CodecRegistry.register(new GraphViewCodec());
     CodecRegistry.register(new ModelCodec());
-    CodecRegistry.register(new RootChangeCodec());
-    CodecRegistry.register(new StylesheetCodec());
-    CodecRegistry.register(new TerminalChangeCodec());
-    registerGenericChangeCodecs();
 
     // To support decode/import executed before encode/export (see https://github.com/maxGraph/maxGraph/issues/178)
     // Codecs are currently only registered automatically during encode/export
@@ -108,6 +101,29 @@ export const registerCoreCodecs = (force = false) => {
     CodecRegistry.register(new mxCellCodec(), false);
     CodecRegistry.register(new mxGeometryCodec(), false);
 
+    isModelCodecsRegistered = true;
+  }
+};
+
+let isCoreCodecsRegistered = false;
+/**
+ * Register core codecs i.e. codecs that don't relate to editor. This includes model codecs that can be registered individually with {@link registerModelCodecs}.
+ *
+ * @param force if `true` register the codecs even if they were already registered. If false, only register them
+ *              if they have never been registered before.
+ * @since 0.6.0
+ */
+export const registerCoreCodecs = (force = false) => {
+  if (!isCoreCodecsRegistered || force) {
+    CodecRegistry.register(new ChildChangeCodec());
+    CodecRegistry.register(new GraphViewCodec());
+    CodecRegistry.register(new RootChangeCodec());
+    CodecRegistry.register(new StylesheetCodec());
+    CodecRegistry.register(new TerminalChangeCodec());
+    registerGenericChangeCodecs();
+
+    registerModelCodecs(force);
+
     isCoreCodecsRegistered = true;
   }
 };
@@ -131,7 +147,7 @@ export const registerEditorCodecs = (force = false) => {
 };
 
 /**
- * Register all editors i.e. core codecs (as done by {@link registerCoreCodecs}) and editor codecs (as done by {@link registerEditorCodecs}).
+ * Register all codecs i.e. core codecs (as done by {@link registerCoreCodecs}) and editor codecs (as done by {@link registerEditorCodecs}).
  *
  * @param force if `true` register the codecs even if they were already registered. If false, only register them
  *              if they have never been registered before.
diff --git a/packages/core/src/view/GraphDataModel.ts b/packages/core/src/view/GraphDataModel.ts
index ce87146e6..d0bd77f29 100644
--- a/packages/core/src/view/GraphDataModel.ts
+++ b/packages/core/src/view/GraphDataModel.ts
@@ -396,9 +396,9 @@ export class GraphDataModel extends EventSource {
   /**
    * Returns true if {@link isRoot} returns true for the parent of the given cell.
    *
-   * @param {Cell} cell  that represents the possible layer.
+   * @param cell  that represents the possible layer.
    */
-  isLayer(cell: Cell) {
+  isLayer(cell: Cell | null) {
     return cell ? this.isRoot(cell.getParent()) : false;
   }
 
diff --git a/packages/core/src/view/mixins/SwimlaneMixin.ts b/packages/core/src/view/mixins/SwimlaneMixin.ts
index 100421fcb..77431687b 100644
--- a/packages/core/src/view/mixins/SwimlaneMixin.ts
+++ b/packages/core/src/view/mixins/SwimlaneMixin.ts
@@ -379,7 +379,7 @@ const SwimlaneMixin: PartialType = {
       }
     }
 
-    return !this.getDataModel().isLayer(<Cell>cell) && !parent ? cell : null;
+    return !this.getDataModel().isLayer(cell) && !parent ? cell : null;
   },
 
   /**
diff --git a/packages/core/src/view/style/Stylesheet.ts b/packages/core/src/view/style/Stylesheet.ts
index b416aacc0..01f01e877 100644
--- a/packages/core/src/view/style/Stylesheet.ts
+++ b/packages/core/src/view/style/Stylesheet.ts
@@ -16,7 +16,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { ALIGN, ARROW, SHAPE } from '../../util/Constants';
+import { ALIGN, ARROW, NONE, SHAPE } from '../../util/Constants';
 import { clone } from '../../util/cloneUtils';
 import type { CellStateStyle, CellStyle } from '../../types';
 
@@ -186,9 +186,14 @@ export class Stylesheet {
     }
 
     // Merges cellStyle into style
+    const filteredCellStyle = clone(cellStyle); // Clones the cellStyle to avoid modifying the original object pass as parameter
+    for (const key of Object.keys(filteredCellStyle)) {
+      (filteredCellStyle[key] === undefined || filteredCellStyle[key] == NONE) &&
+        delete filteredCellStyle[key];
+    }
     style = {
       ...style,
-      ...cellStyle,
+      ...filteredCellStyle,
     };
 
     // Remove the 'baseStyleNames' that may have been copied from the cellStyle parameter to match the method signature


Test patch:
diff --git a/packages/core/__tests__/GraphDataModel.test.ts b/packages/core/__tests__/GraphDataModel.test.ts
index 56832a27b..5527f4b48 100644
--- a/packages/core/__tests__/GraphDataModel.test.ts
+++ b/packages/core/__tests__/GraphDataModel.test.ts
@@ -23,18 +23,15 @@ describe('isLayer', () => {
   dm.setRoot(root);
 
   test('Child is null', () => {
-    // @ts-ignore
-    const child: Cell = null;
-    expect(dm.isLayer(child)).toBe(false);
+    expect(dm.isLayer(null)).toBe(false);
   });
 
   test('Child is not null and is not layer', () => {
-    const child: Cell = new Cell();
-    expect(dm.isLayer(child)).toBe(false);
+    expect(dm.isLayer(new Cell())).toBe(false);
   });
 
   test('Child is not null and is layer', () => {
-    const child: Cell = new Cell();
+    const child = new Cell();
     root.children.push(child);
     child.setParent(root);
     expect(dm.isLayer(child)).toBe(true);
diff --git a/packages/core/__tests__/view/style/StyleSheet.test.ts b/packages/core/__tests__/view/style/StyleSheet.test.ts
index 3666fb071..e1604219d 100644
--- a/packages/core/__tests__/view/style/StyleSheet.test.ts
+++ b/packages/core/__tests__/view/style/StyleSheet.test.ts
@@ -16,6 +16,7 @@ limitations under the License.
 
 import { describe, expect, test } from '@jest/globals';
 import { CellStateStyle, CellStyle, Stylesheet } from '../../../src';
+import { NONE } from '../../../src/util/Constants';
 
 /**
  * Additional properties to test extension points by extending `CellStyle` and `CustomCellStateStyle`.
@@ -136,6 +137,111 @@ describe('getCellStyle', () => {
     });
   });
 
+  test('Setting undefined properties, there are filtered in the computed "getStyle"', () => {
+    const stylesheet = new Stylesheet();
+    const cellStyle = stylesheet.getCellStyle(
+      {
+        align: undefined,
+        shape: 'cloud',
+        strokeColor: undefined,
+      },
+      { arcSize: 5 }
+    );
+    expect(cellStyle).toStrictEqual({
+      arcSize: 5, // from default
+      shape: 'cloud',
+    });
+  });
+
+  // This was the mxGraph behaviour
+  // Setting a null/undefined value with mxUtils.setStyle remove the key from the cell style (in the string form). As it was not present in the style, it felt back to values from the default or base style.
+  // See https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/util/mxUtils.js#L3465-L3478
+  test('Setting undefined properties fallback to property value of the default style', () => {
+    const stylesheet = new Stylesheet();
+    const cellStyle = stylesheet.getCellStyle(
+      {
+        align: undefined,
+        shape: 'cloud',
+        strokeColor: undefined,
+      },
+      { strokeColor: 'red', align: 'left' }
+    );
+    expect(cellStyle).toStrictEqual({
+      align: 'left',
+      shape: 'cloud',
+      strokeColor: 'red',
+    });
+  });
+
+  // This was the mxGraph behaviour (same as fallback to value in the default style)
+  test('Setting undefined properties fallback to property value of the "base" styles', () => {
+    const stylesheet = new Stylesheet();
+    stylesheet.putCellStyle('style-1', {
+      shape: 'triangle',
+      fillColor: 'blue',
+      fillOpacity: 80,
+    });
+
+    const cellStyle = stylesheet.getCellStyle(
+      {
+        baseStyleNames: ['style-1'],
+        fillColor: undefined,
+        fillOpacity: undefined,
+        shape: 'cloud',
+      },
+      {}
+    );
+    expect(cellStyle).toStrictEqual({
+      fillColor: 'blue',
+      fillOpacity: 80,
+      shape: 'cloud',
+    });
+  });
+
+  // This was the mxGraph behaviour, see https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/view/mxStylesheet.js#L236-L239
+  test('Setting properties with "none" value fallback to property value of the default and "base" styles', () => {
+    const stylesheet = new Stylesheet();
+    stylesheet.putCellStyle('style-1', {
+      fillColor: 'orange',
+    });
+
+    const cellStyle = stylesheet.getCellStyle(
+      {
+        baseStyleNames: ['style-1'],
+        fillColor: NONE,
+        shape: 'cloud',
+        strokeColor: NONE,
+      },
+      { strokeColor: 'pink' }
+    );
+    expect(cellStyle).toStrictEqual({
+      fillColor: 'orange',
+      shape: 'cloud',
+      strokeColor: 'pink',
+    });
+  });
+
+  // This was the mxGraph behaviour
+  test('Setting undefined properties in "base" style, set the property to undefined in the style', () => {
+    const stylesheet = new Stylesheet();
+    stylesheet.putCellStyle('style-1', {
+      fillColor: undefined,
+      fillOpacity: 80,
+    });
+
+    const cellStyle = stylesheet.getCellStyle(
+      {
+        baseStyleNames: ['style-1'],
+      },
+      { fillColor: 'yellow', shape: 'cloud' }
+    );
+    expect(cellStyle).toStrictEqual({
+      fillColor: undefined,
+      fillOpacity: 80,
+      shape: 'cloud',
+    });
+  });
+
   test('Custom CellStyle type - baseStyleNames set and related styles are registered', () => {
     const stylesheet = new Stylesheet();
     stylesheet.putCellStyle('style-1', <CustomCellStyle>{