Skip to content

Commit

Permalink
refactor: remove some coupling between the "CSS" and "Style" API (#2825)
Browse files Browse the repository at this point in the history
Previously, when applying the "CSS" API, the implementation ensured that
the cell's original style was cached (for any subsequent reset) without
any additional CSS classes. In fact, this was not necessary. Storing
styles in the cache with additional CSS classes information posed no
problem. When the style is reset, the value retrieved from the cache is
updated with the CSS classes currently applied to the cell. This works
just as well if there are no CSS classes as if there are. This
additional check has therefore been removed.

The tests verifying interactions between the "CSS" and "Style" APIs have
been improved. They confirm that the removal of this coupling is
legitimate:
  - Introduce a dedicated test file for these tests.
- Previously, they were stored in the file testing the style update at
template level.
- This change provides an explicit way of identifying API interaction
tests and a better separation of concerns.
- Add a dedicated test to check the impact of resetting the style after
a CSS and style update. This proves the uselessness of the coupling
described above.

covers #2803
closes #2743

---------

Co-authored-by: Souchet Céline <[email protected]>
  • Loading branch information
tbouffard and csouchet authored Aug 28, 2023
1 parent 03005b7 commit 0f2daa3
Show file tree
Hide file tree
Showing 4 changed files with 194 additions and 91 deletions.
2 changes: 0 additions & 2 deletions src/component/mxgraph/GraphCellUpdater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ export default class GraphCellUpdater {
return;
}

this.styleManager.ensureStyleIsStored(cell);

let cellStyle = cell.getStyle();
cellStyle = setStyle(cellStyle, BpmnStyleIdentifier.EXTRA_CSS_CLASSES, cssClasses.join(','));
model.setStyle(cell, cellStyle);
Expand Down
191 changes: 191 additions & 0 deletions test/integration/cross.css.and.style.api.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
/*
Copyright 2023 Bonitasoft S.A.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import type { mxCell } from 'mxgraph';
import { ShapeBpmnElementKind, ShapeBpmnEventDefinitionKind } from '@lib/model/bpmn/internal';
import { readFileSync } from '@test/shared/file-helper';
import { initializeBpmnVisualizationWithContainerId } from './helpers/bpmn-visualization-initialization';
import { HtmlElementLookup } from './helpers/html-utils';
import type { ExpectedEdgeModelElement, ExpectedShapeModelElement, VerticalAlign } from './helpers/model-expect';
import { buildReceivedResolvedModelCellStyle, buildReceivedViewStateStyle } from './matchers/matcher-utils';
import { buildExpectedShapeCellStyle } from './matchers/toBeShape';
import { buildExpectedEdgeCellStyle } from './matchers/toBeEdge';

// Here, we are checking both the model and the DOM.
// This fully ensures that the usage of both APIs have no impact on the model update and that the repaints are correctly done.

// We cannot reuse the shared model expect functions here. They are using the shared `bpmnVisualization` instance that we cannot use here because we need a dedicated instance to be able to check the DOM.
// So, we use the basic expect model functions. We only need to check a part of the data, the rest is already checked in details in other tests.

// Create a dedicated instance with a DOM container as it is required by the CSS API.
const bv = initializeBpmnVisualizationWithContainerId('bpmn-container-style-css-cross-tests');
const bpmnElementsRegistry = bv.bpmnElementsRegistry;

const getCell = (bpmnElementId: string): mxCell => {
const graph = bv.graph;
const cell = graph.model.getCell(bpmnElementId);
if (!cell) {
throw new Error(`Unable to find cell in the model with id ${bpmnElementId}`);
}
return cell;
};

const htmlElementLookup = new HtmlElementLookup(bv);
const checkFlowNode = (bpmnElementId: string, expectedModel: ExpectedShapeModelElement): void => {
// check mxGraph model
expect(buildReceivedResolvedModelCellStyle(getCell(bpmnElementId), bv)).toEqual(buildExpectedShapeCellStyle(expectedModel));
expect(buildReceivedViewStateStyle(getCell(bpmnElementId), bv)).toEqual(buildExpectedShapeCellStyle(expectedModel));
// check DOM
const checks = { label: expectedModel.label, additionalClasses: expectedModel.extraCssClasses };
const kind = expectedModel.kind;
if (kind == ShapeBpmnElementKind.GATEWAY_EXCLUSIVE) {
htmlElementLookup.expectExclusiveGateway(bpmnElementId, checks);
} else if (kind == ShapeBpmnElementKind.TASK_USER) {
htmlElementLookup.expectUserTask(bpmnElementId, checks);
} else {
htmlElementLookup.expectEndEvent(bpmnElementId, ShapeBpmnEventDefinitionKind.MESSAGE, checks);
}
};

const checkSequenceFlow = (bpmnElementId: string, expectedModel: ExpectedEdgeModelElement): void => {
expectedModel.endArrow = 'blockThin';
expectedModel.verticalAlign = 'bottom'; // the element has a label
// check mxGraph model
expect(buildReceivedResolvedModelCellStyle(getCell(bpmnElementId), bv)).toEqual(buildExpectedEdgeCellStyle(expectedModel));
expect(buildReceivedViewStateStyle(getCell(bpmnElementId), bv)).toEqual(buildExpectedEdgeCellStyle(expectedModel));
// check DOM
htmlElementLookup.expectSequenceFlow(bpmnElementId, { label: expectedModel.label, additionalClasses: expectedModel.extraCssClasses });
};

describe('Verify interaction between the CSS and style APIs', () => {
beforeEach(() => {
bv.load(readFileSync('../fixtures/bpmn/registry/1-pool-3-lanes-message-start-end-intermediate-events.bpmn'));
});

describe('Both style API update and CSS class', () => {
it.each([true, false])('Apply style update first %s', (isStyleUpdateAppliedFirst: boolean) => {
const bpmnElementId = 'endEvent_message_1';
const strokeColor = 'pink';
const cssClassNames = ['class-1', 'class-2'];
if (isStyleUpdateAppliedFirst) {
bpmnElementsRegistry.updateStyle(bpmnElementId, { stroke: { color: strokeColor } });
bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassNames);
} else {
bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassNames);
bpmnElementsRegistry.updateStyle(bpmnElementId, { stroke: { color: strokeColor } });
}

checkFlowNode(bpmnElementId, {
extraCssClasses: ['class-1', 'class-2'],
kind: ShapeBpmnElementKind.EVENT_END,
label: 'message end 2',
stroke: { color: strokeColor },
verticalAlign: <VerticalAlign>'top', // the element has a label
} as ExpectedShapeModelElement);
});
});

describe('Style API Reset and CSS class update', () => {
it.each([true, false])('Reset style first %s', (isStyleResetFirst: boolean) => {
const bpmnElementId = 'endEvent_message_1';
const cssClassNames = ['class-1', 'class-2'];

bpmnElementsRegistry.updateStyle(bpmnElementId, { stroke: { color: 'pink' } });

if (isStyleResetFirst) {
bpmnElementsRegistry.resetStyle(bpmnElementId);
bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassNames);
} else {
bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassNames);
bpmnElementsRegistry.resetStyle(bpmnElementId);
}

// Check that the style has been reset to default values for each element
checkFlowNode(bpmnElementId, {
extraCssClasses: ['class-1', 'class-2'],
kind: ShapeBpmnElementKind.EVENT_END,
label: 'message end 2',
verticalAlign: <VerticalAlign>'top', // the element has a label
} as ExpectedShapeModelElement);
});
});

// Perform both "CSS classes" operations both before and after updating the style
// This validates all possible interactions between the 2 APIS
it('Style API update and reset + CSS class update', () => {
bpmnElementsRegistry.addCssClasses(
['gateway_01', 'userTask_0', 'userTask_2_2', 'endEvent_message_1', 'endEvent_terminate_1', 'sequenceFlow_lane_1_elt_1', 'sequenceFlow_lane_1_elt_6'],
['class-1', 'class-2'],
);
bpmnElementsRegistry.updateStyle(['gateway_01', 'userTask_2_2', 'endEvent_terminate_1', 'sequenceFlow_lane_1_elt_1'], { stroke: { color: 'pink' } });
bpmnElementsRegistry.addCssClasses(['gateway_01', 'userTask_0', 'endEvent_message_1', 'sequenceFlow_lane_1_elt_6'], 'class-11');
bpmnElementsRegistry.removeCssClasses(['userTask_2_2'], 'class-1');

// Check some elements
checkFlowNode('gateway_01', {
extraCssClasses: ['class-1', 'class-2', 'class-11'],
kind: ShapeBpmnElementKind.GATEWAY_EXCLUSIVE,
label: 'gateway 1',
verticalAlign: 'top', // the element has a label
stroke: { color: 'pink' },
} as ExpectedShapeModelElement);
checkSequenceFlow('sequenceFlow_lane_1_elt_1', {
extraCssClasses: ['class-1', 'class-2'],
stroke: { color: 'pink' },
} as ExpectedEdgeModelElement);

// Reset style of some elements and check them afterward
bpmnElementsRegistry.resetStyle(['gateway_01', 'sequenceFlow_lane_1_elt_1']);

// Check that the style has been reset to default values for each element
checkFlowNode('gateway_01', {
extraCssClasses: ['class-1', 'class-2', 'class-11'],
kind: ShapeBpmnElementKind.GATEWAY_EXCLUSIVE,
label: 'gateway 1',
verticalAlign: 'top', // the element has a label
} as ExpectedShapeModelElement);
checkSequenceFlow('sequenceFlow_lane_1_elt_1', {
extraCssClasses: ['class-1', 'class-2'],
} as ExpectedEdgeModelElement);

// Reset the style of remaining elements with 'reset all'
bpmnElementsRegistry.updateStyle(['endEvent_message_1'], { fill: { color: 'yellow' } });
bpmnElementsRegistry.resetStyle();

// Check that the style has been reset to default values for each element
checkFlowNode('userTask_2_2', {
extraCssClasses: ['class-2'],
kind: ShapeBpmnElementKind.TASK_USER,
label: 'User Task 2.2',
} as ExpectedShapeModelElement);
checkFlowNode('endEvent_message_1', {
extraCssClasses: ['class-1', 'class-2', 'class-11'],
kind: ShapeBpmnElementKind.EVENT_END,
label: 'message end 2',
verticalAlign: <VerticalAlign>'top', // the element has a label
} as ExpectedShapeModelElement);

// Check elements whose style were not updated prior calling 'reset all'
checkFlowNode('userTask_0', {
extraCssClasses: ['class-1', 'class-2', 'class-11'],
kind: ShapeBpmnElementKind.TASK_USER,
label: 'User Task 0',
} as ExpectedShapeModelElement);
checkSequenceFlow('sequenceFlow_lane_1_elt_6', {
extraCssClasses: ['class-1', 'class-2', 'class-11'],
} as ExpectedEdgeModelElement);
});
});
2 changes: 1 addition & 1 deletion test/integration/matchers/toBeEdge/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { mxConstants } from '@lib/component/mxgraph/initializer';
import MatcherContext = jest.MatcherContext;
import CustomMatcherResult = jest.CustomMatcherResult;

function buildExpectedEdgeCellStyle(expectedModel: ExpectedEdgeModelElement): BpmnCellStyle {
export function buildExpectedEdgeCellStyle(expectedModel: ExpectedEdgeModelElement): BpmnCellStyle {
const style = buildExpectedCellStyleWithCommonAttributes(expectedModel);
style.verticalAlign = expectedModel.verticalAlign ?? 'top';
style.align = 'center';
Expand Down
90 changes: 2 additions & 88 deletions test/integration/mxGraph.model.style.api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,39 +14,11 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { initializeBpmnVisualizationWithContainerId } from './helpers/bpmn-visualization-initialization';
import { HtmlElementLookup } from './helpers/html-utils';
import type { ExpectedDirection, ExpectedFill, ExpectedShapeModelElement, VerticalAlign } from './helpers/model-expect';
import type { ExpectedDirection, ExpectedFill } from './helpers/model-expect';
import { bpmnVisualization } from './helpers/model-expect';
import { buildReceivedResolvedModelCellStyle, buildReceivedViewStateStyle } from './matchers/matcher-utils';
import { buildExpectedShapeCellStyle } from './matchers/toBeShape';
import { readFileSync } from '@test/shared/file-helper';
import { MessageVisibleKind, ShapeBpmnElementKind, ShapeBpmnEventDefinitionKind } from '@lib/model/bpmn/internal';
import { MessageVisibleKind, ShapeBpmnEventDefinitionKind } from '@lib/model/bpmn/internal';
import type { EdgeStyleUpdate, Fill, Font, GradientDirection, Stroke, StyleUpdate } from '@lib/component/registry';
import type { mxCell } from 'mxgraph';

// Create a dedicated instance with a DOM container as it is required by the CSS API.
const bv = initializeBpmnVisualizationWithContainerId('bpmn-container-style-css-cross-tests');
const htmlElementLookup = new HtmlElementLookup(bv);

const getCell = (bpmnElementId: string): mxCell => {
const graph = bv.graph;
const cell = graph.model.getCell(bpmnElementId);
if (!cell) {
throw new Error(`Unable to find cell in the model with id ${bpmnElementId}`);
}
return cell;
};

// we cannot reuse the model expect functions here. They are using the shared bpmnVisualization that we cannot use here.
// So use the minimal expect function. We only need to check a part of the data, the rest is already checked in details in other tests.
const checkViewStateStyle = (bpmnElementId: string, expectedModel: ExpectedShapeModelElement): void => {
expect(buildReceivedViewStateStyle(getCell(bpmnElementId), bv)).toEqual(buildExpectedShapeCellStyle(expectedModel));
};

const checkModelStyle = (bpmnElementId: string, expectedModel: ExpectedShapeModelElement): void => {
expect(buildReceivedResolvedModelCellStyle(getCell(bpmnElementId), bv)).toEqual(buildExpectedShapeCellStyle(expectedModel));
};

describe('mxGraph model - update style', () => {
describe('Shapes', () => {
Expand Down Expand Up @@ -797,34 +769,6 @@ describe('mxGraph model - update style', () => {
});
});
});

// Check that there is no bad interactions between the two features
describe('Both style API update and CSS class', () => {
it.each([true, false])('Apply style update first %s', (isStyleUpdateAppliedFirst: boolean) => {
bv.load(readFileSync('../fixtures/bpmn/registry/1-pool-3-lanes-message-start-end-intermediate-events.bpmn'));

const bpmnElementId = 'endEvent_message_1';
const strokeColor = 'pink';
const cssClassName = ['class-1', 'class-2'];
if (isStyleUpdateAppliedFirst) {
bv.bpmnElementsRegistry.updateStyle(bpmnElementId, { stroke: { color: strokeColor } });
bv.bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassName);
} else {
bv.bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassName);
bv.bpmnElementsRegistry.updateStyle(bpmnElementId, { stroke: { color: strokeColor } });
}

const expectedModel = {
extraCssClasses: ['class-1', 'class-2'],
kind: ShapeBpmnElementKind.EVENT_END,
stroke: { color: strokeColor },
verticalAlign: <VerticalAlign>'top', // when events have a label
};
checkModelStyle(bpmnElementId, expectedModel);
checkViewStateStyle(bpmnElementId, expectedModel);
htmlElementLookup.expectEndEvent(bpmnElementId, ShapeBpmnEventDefinitionKind.MESSAGE, { label: 'message end 2', additionalClasses: ['class-1', 'class-2'] });
});
});
});

describe('mxGraph model - reset style', () => {
Expand Down Expand Up @@ -1183,34 +1127,4 @@ describe('mxGraph model - reset style', () => {
});
});
});

// Check that there is no bad interactions between the two features
describe('Style API Reset and CSS class update', () => {
it.each([true, false])('Reset style first %s', (isStyleResetFirst: boolean) => {
bv.load(readFileSync('../fixtures/bpmn/registry/1-pool-3-lanes-message-start-end-intermediate-events.bpmn'));

const bpmnElementId = 'endEvent_message_1';
const cssClassName = ['class-1', 'class-2'];

bv.bpmnElementsRegistry.updateStyle(bpmnElementId, { stroke: { color: 'pink' } });

if (isStyleResetFirst) {
bv.bpmnElementsRegistry.resetStyle(bpmnElementId);
bv.bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassName);
} else {
bv.bpmnElementsRegistry.addCssClasses(bpmnElementId, cssClassName);
bv.bpmnElementsRegistry.resetStyle(bpmnElementId);
}

// Check that the style has been reset to default values for each element
const expectedModel = {
extraCssClasses: ['class-1', 'class-2'],
kind: ShapeBpmnElementKind.EVENT_END,
verticalAlign: 'top' as VerticalAlign, // when events have a label
};
checkModelStyle(bpmnElementId, expectedModel);
checkViewStateStyle(bpmnElementId, expectedModel);
htmlElementLookup.expectEndEvent(bpmnElementId, ShapeBpmnEventDefinitionKind.MESSAGE, { label: 'message end 2', additionalClasses: ['class-1', 'class-2'] });
});
});
});

0 comments on commit 0f2daa3

Please sign in to comment.