search

bpmn-io / dmn-js

View and edit DMN diagrams in the browser.
https://bpmn.io/toolkit/dmn-js/
Other
289 stars 137 forks source link

dmn-js-decision-table does not work properly in web components #631

Closed holkwen closed 1 month ago

holkwen commented 3 years ago

Describe the Bug

There are many things that do not work in the decision table when using inside a web component:

  1. "View DRD" and "+" buttons don't work (nothing happens on click).
  2. Double click on a table head cell does not work.
  3. Resizing columns does not work (borders cannot be dragged).
  4. Context menu (right-click menu) is displayed, but nothing happens when an item from the menu is clicked.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Build the dmn-js editor inside a web component.
  2. Create a DMN diagram with a decision table, or open an existing one.
  3. Click on elements / select context menu items.

Expected Behavior

Clicks on the buttons and context menu items should produce the expected behavior.

Possible solutions

  1. The problem with clicks is related to the known (and fixed) bug of inferno js: https://github.com/infernojs/inferno/issues/1430 We have tried a number of inferno js versions, and clicks start to work correctly starting with 7.3.x without changes to dmn-js-decision-table. So, the solution could be to upgrade to inferno js 7.3.x or newer.

  2. The problem with double clicks on the table's head cells is related to Shadow DOM. The event's target 'bubbles' to the parent element in the light dom, and consequently domClosest cannot find the correct element. This can be fixed by replacing 'event.target' with 'event.currentTarget' in InputEditingProvider.js and OutputEditingProvider.js.

  3. The problem with resizing columns is the same as 2 and can be fixed by replacing 'event.target' with 'event.currentTarget' in ResizeColumn.js.

  4. The problem with context menu goes into the table-js library. If the ContextMenuComponent there registers an event listener for the 'mousedown' event and the handler for this event calls _this.setState, no subsequent 'click' event is fired. As a result, the context menu just closes without triggering any action. This might be an issue of inferno js (we did not investigate further). Our solution was to replace 'mousedown' with 'click' in document.addEventListener('mousedown', this.onGlobalMouseDown), but it's just a solution 'to make it work'.

Environment

nikku commented 3 years ago

Needs backport of https://github.com/infernojs/inferno/commit/2753856cb01220f6339aa86fe48b89b170aa35d9 to inferno@5.x and integration of a fixed inferno version.

arjangeertsema commented 3 months ago

@holkwen thank you for this great analysis, it is a bit disappointing to see that your suggestions were not picked-up, especially because the changes and impact are very limited.

@nikku what is the way forward? Upgrading inferno to a newer supported version or to change the code of unsupported versions?

Second question @nikku: I also created a pull request which fixed webcomponent related issues in the properties panel (https://github.com/bpmn-io/properties-panel/pull/318). What is the overal webcomponent strategy of bpmn-js and dmn-js?

nikku commented 3 months ago

@arjangeertsema I think web components are here to stay and shall be fully supported, some day, maybe.

You could help us by pushing for a backport of this change in inferno@5 => https://github.com/bpmn-io/dmn-js/issues/631#issuecomment-849578345.

You could help us by contributing improvements in dmn-js, including relevant test coverage.

You could help us by bootstrapping general shadow DOM test coverage so we account for this use case.

Your call.

arjangeertsema commented 3 months ago

Hey @nikku thank you for reacting. I understand the opensource dilemma, and I am grateful for the work.

I have some node-js knowledge, but I am not an expert and it is not my daily work. I do have budget for these improvements. I am looking in my network for specialised knowledge to adres these issues.

You can help me, @nikku , to solve these problems by linking me to independent specialists in your network who can help improve bpmn.js and dmn.js on a commercial basis.

Thanks.

arjangeertsema commented 1 month ago

The backport fix PR has been created. The changes have been added and the new test scenarios are passing. Other build steps are failing, probably due to legacy test scripts which are not supported by the current test runner version. These test are untouched by the PR changes. @nikku can you help bring this forward?

arjangeertsema commented 1 month ago

@nikku can you answer Havunens question? https://github.com/infernojs/inferno/pull/1669#issuecomment-2237239684

barmac commented 1 month ago

I see this request is outdated since the change has already been released. We'll update inferno, and hopefully the issue is resolved.