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

Call Activity Propagation data marker does not update #3815

Closed
marstamm opened this issue Aug 30, 2023 · 6 comments · Fixed by #3897
Closed

Call Activity Propagation data marker does not update #3815

marstamm opened this issue Aug 30, 2023 · 6 comments · Fixed by #3897
Assignees
Labels
bug Something isn't working
Milestone

Comments

@marstamm
Copy link
Member

Describe the bug

Recording 2023-08-30 at 11 14 06 (1)

Steps to reproduce

  1. Create a Call activity in C8
  2. Toggle on Input or Output propagation
  3. Toggle off again
  4. The Group data marker is still there

Expected behavior

The data marker is removed when I toggle propagation off.

Environment

Camunda Modeler system information

  • Version: 5.15.0-dev
  • Operating System: Linux x86_64 amd64
  • Execution Platform: BPMN - Camunda Platform 8

#3814

Additional context

The data marker is updated when another property in the zeebe:calledElement is updated, e.g. other Propagation or called element ID

@marstamm marstamm added the bug Something isn't working label Aug 30, 2023
@nikku nikku added the ready Ready to be worked on label Aug 30, 2023
@smbea
Copy link
Contributor

smbea commented Sep 13, 2023

The useEffect that checks for entry changes is triggered before the checkbox is actually checked. If we check the toggle value a few milliseconds later, then we would obtain the true value. This is also why the value is corrected after a re-render (caused by other entries).

I'm still not sure why it's not behaving as expected. What I can imagine is the useEffect is triggered by the setValue and getting the new value from the DOM before the actual re-render has happened:

Toggle level: user input -> set checkbox value -> checkbox is rendered accordingly
Group level:                                   ->`useEffect` is triggered -> get value via `domQuery`

@nikku nikku added this to the M70 milestone Sep 22, 2023
@smbea
Copy link
Contributor

smbea commented Sep 22, 2023

Findings from looking at this with @barmac:

  • The marker is updated based on the DOM at the time of the entries changed
  • The DOM of the toggle switch is changed after useEffect has run
  • isEdited doesn't depend on DOM entries but variable entries
Screenshot 2023-09-22 at 12 10 15

@barmac
Copy link
Collaborator

barmac commented Sep 22, 2023

One idea: use MutationObserver to dispatch marker updates instead of useEffect.

@barmac
Copy link
Collaborator

barmac commented Sep 22, 2023

We cannot use :has because it's not supported in FireFox.

@barmac
Copy link
Collaborator

barmac commented Sep 22, 2023

Workaround sketch: bpmn-io/properties-panel@3f9f598

@smbea
Copy link
Contributor

smbea commented Sep 25, 2023

I propose we go with the workaround for now. In the future (or if more issues like this one come up), we should consider refactoring the way we check/update if entries are edited.

This could include making entries more aware and having their own isEdited method, as opposed to it being a separate utility function. This way, it always checks the actual input value:

class ToggleSwitch {
  isEdited() {
    return !!this.state.checked
  }
}

smbea pushed a commit to bpmn-io/properties-panel that referenced this issue Sep 25, 2023
smbea pushed a commit to bpmn-io/properties-panel that referenced this issue Sep 25, 2023
barmac added a commit to bpmn-io/properties-panel that referenced this issue Sep 25, 2023
Related to camunda/camunda-modeler#3815

Co-authored-by: Beatriz Mendes <bea.smendes98@gmail.com>
smbea added a commit to bpmn-io/properties-panel that referenced this issue Sep 25, 2023
Related to camunda/camunda-modeler#3815

Co-authored-by: Beatriz Mendes <bea.smendes98@gmail.com>
@smbea smbea added the fixed upstream Requires integration of upstream change label Sep 25, 2023 — with bpmn-io-tasks
@smbea smbea removed the ready Ready to be worked on label Sep 25, 2023
@nikku nikku modified the milestones: M70, M69 Sep 27, 2023
@nikku nikku mentioned this issue Sep 27, 2023
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Sep 27, 2023
@bpmn-io-tasks bpmn-io-tasks bot removed the fixed upstream Requires integration of upstream change label Sep 27, 2023
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants