Toggle state for list items is persisted even for different elements

[MaxTru]

Describe the Bug

Toggle state for list items is persisted even for different elements.

Steps to Reproduce

1. Have a process with process variables (see attached sample diagram)
2. In the process, have a sub-process with process variables (see attached sample diagram)
3. Click on the process
4. Open the Process Variables group
5. Toggle one process variable list item
6. Now click on the sub process
7. => Notice that the toggle state is the same (ie. if in the process the second list item is uncollapsed, also in the sub-process the second list item will be uncollapsed)

(Notice the same thing can be created with other list components, e.g., Input Parameter)

Expected Behavior

• Toggle state of list items is not persisted between different elements
• Toggle state of list items is persisted for the same element ==> follow up #90

Environment

• Host (Browser/Node version), if applicable: Chrome
• OS: Arch Linux
• Library version: 0.2.0

Example BPMN

<?xml version="1.0" encoding="UTF-8"?>
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" xmlns:di="http://www.omg.org/spec/DD/20100524/DI" xmlns:camunda="http://camunda.org/schema/1.0/bpmn" id="Definitions_0vshbc8" targetNamespace="http://bpmn.io/schema/bpmn" exporter="Camunda Modeler" exporterVersion="4.9.0">
  <bpmn:process id="Process_1" isExecutable="true">
    <bpmn:startEvent id="StartEvent_1">
      <bpmn:outgoing>Flow_0x3xt3q</bpmn:outgoing>
    </bpmn:startEvent>
    <bpmn:task id="Task_1" name="Task 1">
      <bpmn:extensionElements>
        <camunda:inputOutput>
          <camunda:outputParameter name="variable1">200</camunda:outputParameter>
          <camunda:outputParameter name="variable2">${myLocalVar + 20}</camunda:outputParameter>
        </camunda:inputOutput>
      </bpmn:extensionElements>
      <bpmn:incoming>Flow_0x3xt3q</bpmn:incoming>
      <bpmn:outgoing>Flow_07gumap</bpmn:outgoing>
    </bpmn:task>
    <bpmn:sequenceFlow id="Flow_0x3xt3q" sourceRef="StartEvent_1" targetRef="Task_1" />
    <bpmn:endEvent id="Event_177sc47">
      <bpmn:incoming>Flow_106422d</bpmn:incoming>
    </bpmn:endEvent>
    <bpmn:sequenceFlow id="Flow_07gumap" sourceRef="Task_1" targetRef="Task_3" />
    <bpmn:subProcess id="SubProcess_1" name="sub process">
      <bpmn:extensionElements>
        <camunda:inputOutput>
          <camunda:inputParameter name="variable4">200</camunda:inputParameter>
        </camunda:inputOutput>
      </bpmn:extensionElements>
      <bpmn:incoming>Flow_07jlmj4</bpmn:incoming>
      <bpmn:outgoing>Flow_106422d</bpmn:outgoing>
      <bpmn:startEvent id="Event_0juijw9">
        <bpmn:outgoing>Flow_08rdoxi</bpmn:outgoing>
      </bpmn:startEvent>
      <bpmn:task id="Task_2" name="Task 2">
        <bpmn:extensionElements>
          <camunda:inputOutput>
            <camunda:outputParameter name="variable4">foo</camunda:outputParameter>
          </camunda:inputOutput>
        </bpmn:extensionElements>
        <bpmn:incoming>Flow_08rdoxi</bpmn:incoming>
        <bpmn:outgoing>Flow_0q3gl8i</bpmn:outgoing>
      </bpmn:task>
      <bpmn:sequenceFlow id="Flow_08rdoxi" sourceRef="Event_0juijw9" targetRef="Task_2" />
      <bpmn:endEvent id="Event_0z6jiz0">
        <bpmn:incoming>Flow_0q3gl8i</bpmn:incoming>
      </bpmn:endEvent>
      <bpmn:sequenceFlow id="Flow_0q3gl8i" sourceRef="Task_2" targetRef="Event_0z6jiz0" />
      <bpmn:textAnnotation id="TextAnnotation_197masl">
        <bpmn:text>Output: variable4</bpmn:text>
      </bpmn:textAnnotation>
      <bpmn:association id="Association_0b61rwc" sourceRef="Task_2" targetRef="TextAnnotation_197masl" />
    </bpmn:subProcess>
    <bpmn:sequenceFlow id="Flow_106422d" sourceRef="SubProcess_1" targetRef="Event_177sc47" />
    <bpmn:task id="Task_3">
      <bpmn:extensionElements>
        <camunda:inputOutput>
          <camunda:outputParameter name="variable3" />
        </camunda:inputOutput>
      </bpmn:extensionElements>
      <bpmn:incoming>Flow_07gumap</bpmn:incoming>
      <bpmn:outgoing>Flow_07jlmj4</bpmn:outgoing>
    </bpmn:task>
    <bpmn:sequenceFlow id="Flow_07jlmj4" sourceRef="Task_3" targetRef="SubProcess_1" />
    <bpmn:textAnnotation id="TextAnnotation_14l4lam">
      <bpmn:text>Input: variable4</bpmn:text>
    </bpmn:textAnnotation>
    <bpmn:association id="Association_1drcf6y" sourceRef="SubProcess_1" targetRef="TextAnnotation_14l4lam" />
    <bpmn:textAnnotation id="TextAnnotation_1ixo74k">
      <bpmn:text>Output: variable1, variable2</bpmn:text>
    </bpmn:textAnnotation>
    <bpmn:association id="Association_0m3wc12" sourceRef="Task_1" targetRef="TextAnnotation_1ixo74k" />
    <bpmn:textAnnotation id="TextAnnotation_0017htw">
      <bpmn:text>Output: 
variable3</bpmn:text>
    </bpmn:textAnnotation>
    <bpmn:association id="Association_1mcx4ho" sourceRef="Task_3" targetRef="TextAnnotation_0017htw" />
  </bpmn:process>
  <bpmndi:BPMNDiagram id="BPMNDiagram_1">
    <bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_1">
      <bpmndi:BPMNEdge id="Flow_07jlmj4_di" bpmnElement="Flow_07jlmj4">
        <di:waypoint x="520" y="287" />
        <di:waypoint x="570" y="287" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="Flow_106422d_di" bpmnElement="Flow_106422d">
        <di:waypoint x="920" y="287" />
        <di:waypoint x="972" y="287" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="Flow_07gumap_di" bpmnElement="Flow_07gumap">
        <di:waypoint x="370" y="287" />
        <di:waypoint x="420" y="287" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="Flow_0x3xt3q_di" bpmnElement="Flow_0x3xt3q">
        <di:waypoint x="215" y="287" />
        <di:waypoint x="270" y="287" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNShape id="_BPMNShape_StartEvent_2" bpmnElement="StartEvent_1">
        <dc:Bounds x="179" y="269" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Activity_1utdf9m_di" bpmnElement="Task_1">
        <dc:Bounds x="270" y="247" width="100" height="80" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Event_177sc47_di" bpmnElement="Event_177sc47">
        <dc:Bounds x="972" y="269" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Activity_071xtw7_di" bpmnElement="SubProcess_1" isExpanded="true">
        <dc:Bounds x="570" y="170" width="350" height="217" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNEdge id="Flow_0q3gl8i_di" bpmnElement="Flow_0q3gl8i">
        <di:waypoint x="800" y="287" />
        <di:waypoint x="862" y="287" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="Flow_08rdoxi_di" bpmnElement="Flow_08rdoxi">
        <di:waypoint x="646" y="287" />
        <di:waypoint x="700" y="287" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNShape id="Event_0juijw9_di" bpmnElement="Event_0juijw9">
        <dc:Bounds x="610" y="269" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Activity_0c3fm0y_di" bpmnElement="Task_2">
        <dc:Bounds x="700" y="247" width="100" height="80" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Event_0z6jiz0_di" bpmnElement="Event_0z6jiz0">
        <dc:Bounds x="862" y="269" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="TextAnnotation_197masl_di" bpmnElement="TextAnnotation_197masl">
        <dc:Bounds x="780" y="190" width="100" height="40" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNEdge id="Association_0b61rwc_di" bpmnElement="Association_0b61rwc">
        <di:waypoint x="789" y="247" />
        <di:waypoint x="806" y="230" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNShape id="Activity_0a1objl_di" bpmnElement="Task_3">
        <dc:Bounds x="420" y="247" width="100" height="80" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="TextAnnotation_14l4lam_di" bpmnElement="TextAnnotation_14l4lam">
        <dc:Bounds x="620" y="70" width="120" height="30" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="TextAnnotation_1ixo74k_di" bpmnElement="TextAnnotation_1ixo74k">
        <dc:Bounds x="290" y="170" width="100" height="54" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="TextAnnotation_0017htw_di" bpmnElement="TextAnnotation_0017htw">
        <dc:Bounds x="460" y="140" width="100" height="40" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNEdge id="Association_1drcf6y_di" bpmnElement="Association_1drcf6y">
        <di:waypoint x="731" y="170" />
        <di:waypoint x="722" y="100" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="Association_0m3wc12_di" bpmnElement="Association_0m3wc12">
        <di:waypoint x="328" y="247" />
        <di:waypoint x="332" y="224" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="Association_1mcx4ho_di" bpmnElement="Association_1mcx4ho">
        <di:waypoint x="482" y="247" />
        <di:waypoint x="502" y="180" />
      </bpmndi:BPMNEdge>
    </bpmndi:BPMNPlane>
  </bpmndi:BPMNDiagram>
</bpmn:definitions>

[MaxTru]

Discovered when implementing bpmn-io/bpmn-properties-panel#31

[pinussilvestrus]

I think we have to things here

Toggle state of list items is not persisted between different elements

That's probably to the fact we don't use unique ids per element. The variable-3 item will be the same for element A and for element B. We can solve this by using unique ids here, e.g. adding a prefix indicating the element. That would need a fix inside bpmn-io/bpmn-properties-panel I'd assume. I wouldn't add such a prefix inside the core, it's the duty of the implementor to define the items.

Toggle state of list items is persisted for the same element

That's a bit tougher. Giving we add uniqueness to each item, they will be rendered which every element selection change. Persisting the collapsed state would mean we would have to persist the state per element somewhere. I don't think we have a proper way of doing it as of now. What we could try out is to use the LayoutContext (which was meant for exactly this purpose).

[andreasgeier]

I agree with what Max described as expected behavior for sections and list items.

We have to pay attention to one thing in addition: there are more properties across selections than just sections and list items. One example is the textarea (for documentation and others). If the user resizes it for one element, would it be expected to be also resized on all other elements? Maybe it was just a single reason because of a long text or script. But it could also be that someone just wants to focus on documentation and wants to have it open and large for all elements.

I have no strong opinion on that yet and am open to suggestions. As fallback, we could start without persistence.

[pinussilvestrus]

I added a simple fix for the first part ("Toggle state of list items should not be persisted between different elements") via #89. Since the second part is more difficult, I'd tackle it separately.

[volkergersabeck]

I could imagine that we start without persisting the list states. For sections I see the value but for lists I'm not sure how much value this actually brings, given the effort it takes to implement this.

[pinussilvestrus]

One step at a time 👍

[andreasgeier]

I disagree with @volkergersabeck statement and am not sure if the consequences were understood. Currently, we have just wrong behavior and keep states of list items in sync that have nothing to do with each other. Why should variable 3 on one element be kept in sync with variable 3 on all other elements? This should be avoided.

But I agree with doing that step by step, of course. However, it should be part of the initial scope.

[pinussilvestrus]

To clarify

Why should variable 3 on one element be kept in sync with variable 3 on all other elements?

that's the current behavior, which is a bug for sure. We can fix this by making all list items unique, which is solved by bpmn-io/bpmn-properties-panel#63 + #89.

However, this will keep all items unsynced and not persisting. Therefore, when a new list item is rendered for another element, it will be closed, because it was not there before. Persisting the state is way more difficult and has to be solved individually.

[andreasgeier]

Ok, if this can be handled independent from each other, I'm fine with it.

Let's (a) fix the behavior that I mentioned right now and (b) enable the persistence within a list later.

[pinussilvestrus]

I opened another issue to track the b) part: #90