Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting Color via addMarker on Sequence Flows does not color Arrowhead #4307

Closed
yT0n1 opened this issue May 23, 2024 · 3 comments · Fixed by #4324
Closed

Setting Color via addMarker on Sequence Flows does not color Arrowhead #4307

yT0n1 opened this issue May 23, 2024 · 3 comments · Fixed by #4324
Assignees
Labels
bug Something isn't working spring cleaning Could be cleaned up one day
Milestone

Comments

@yT0n1
Copy link
Member

yT0n1 commented May 23, 2024

Describe the bug

When styling a connection via addMarker (as described here), the arrowhead is not colored.

Steps to reproduce

  1. open https://github.com/yT0n1/bpmn-js-examples/blob/332480cb2334736b44e17ed7e91c32ae27d31794/colors/index.html#L96 in browser
  2. Look at the first sequence flow after Order Received message start event. The arrowhead is not colored green.

Expected behavior

I would expect addMarker to work also on sequence flows. Alternatively, I would wish for a convenience function for sequence flows to achieve the same behavior.

Environment

  • OS: MacOs 14.5

Additional context

Slack discussion: https://camunda.slack.com/archives/C032H77434N/p1716470720927639

@yT0n1 yT0n1 added the bug Something isn't working label May 23, 2024
@marstamm
Copy link
Member

Root Cause

Arrow heads (and some other SVG elements) are rendered using marker svg attributes, in this case marked-end.

All markers are shared and referenced by ID. The definitions are in the root of the SVG, not in the djs-visual of the element we are adding a marker to. Because of this, the Color can not be changed with SVG

Solution Sketch

To allow styling of connections with markers, instead of having a shared marker for all, we could create a marker for every connection and add it in the djs-visual subtree:

<g class="djs-visual">
  <defs>
    <marker id="uniqueID" >
       <path /*...*/ ></path>
    </marker>
  </defs>
  <path style="marker-end: url(&quot;#uniqueID&quot;);" /*...*/></path>
</g>

This way, css will also affect the marker:

.marked .djs-visual path {
    stroke: blue !important;
    fill: blue !important;
}

This could have performance implications, since we are adding more elements to the SVG, but I do not expect this to have a huge impact, as we are still rendering the same amount of markers

@philippfromme
Copy link
Contributor

philippfromme commented May 23, 2024

@marstamm Thanks for the analysis. The one global <defs> with reused markers approach was used because initially you couldn't change colors so there'd be exactly one marker for each connection type. When adding colors we slightly modified it to one marker for each connection type and color. Happy to see this changing.

@marstamm
Copy link
Member

marstamm commented May 23, 2024

I'd like to look into this in Spring Cleaning and evaluate if this can be achieved in a timely manner

@marstamm marstamm added the spring cleaning Could be cleaned up one day label May 23, 2024
@marstamm marstamm self-assigned this May 24, 2024
@marstamm marstamm added the in progress Currently worked on label May 24, 2024
marstamm added a commit to bpmn-io/bpmn-js that referenced this issue May 27, 2024
marstamm added a commit to bpmn-io/bpmn-js that referenced this issue May 28, 2024
marstamm added a commit to bpmn-io/bpmn-js that referenced this issue May 28, 2024
marstamm added a commit to bpmn-io/bpmn-js that referenced this issue May 28, 2024
@marstamm marstamm added the fixed upstream Requires integration of upstream change label May 28, 2024 — with bpmn-io-tasks
@marstamm marstamm removed the in progress Currently worked on label May 28, 2024
nikku added a commit that referenced this issue May 30, 2024
deps: update to camunda-bpmn-js@4.9.0

Closes #4317
Closes #4315
Closes #4307
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label May 30, 2024
@bpmn-io-tasks bpmn-io-tasks bot removed the fixed upstream Requires integration of upstream change label May 30, 2024
@nikku nikku closed this as completed in e526a4a May 30, 2024
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 30, 2024
@nikku nikku added this to the M77 milestone Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working spring cleaning Could be cleaned up one day
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants