Pressing Enter in the Annotations column to navigate through a Decision Table creates unwanted rows

[azeghers]

Describe the Bug

In the decision table, pressing enter focuses the row below the currently selected cell, and if it's the last row, creates a new one and focuses it.
But in the "Annotations" column, pressing enter from any selected row will just add a new one at the bottom and not change focus

Steps to Reproduce

Steps to reproduce the behavior:

1. Open a DMN Decision Table
2. Click on any cell of the last column (Annotations) as if to input text
3. Press enter

Expected Behavior

The cursor falls to the next cell below, and if there isn't one, a new row is added and focused.

Actual Behavior

The cursor stays in the selected cell and a new row is systematically appended at the end of the table.

Environment

• Browser: Chrome 86.0.4240.75
• OS: Ubuntu 20.04
• Library version: v9.3.2

[azeghers]

Blocks #602

[pinussilvestrus]

Good finding, I was able to reproduce it on current master 👍

[barmac]

Blocks #602

We discovered that #602 is not blocked entirely by this issue. We can tackle this separately.

[azeghers]

This issue seems to stem from the fact that the annotations column and its cells aren't treated like the rest of the table cells.
For instance, its coordinates are stored as "2:annotation" instead of "2:4".
This results in helper functions returning NaN, or selecting the wrong cell because multiple cells share the same id.
My suggestions would be the following :
-Since the context menu needs to be reachable from any cell, give rows a rowId. It would make more sense than feeding it singular cell ids that are shared throughout the row.
-Treat the annotations column like the rest of the table, with a single, distinct id per cell and valid coordinates.
-The root objects shouldn't have collections of cells by type (input, output), but instead store whole rows and cols, and have a type attribute. This would make the visualization of the neighbor cells more intuitive.

[pinussilvestrus]

Is this something that needs to be tackled earlier aka adding the "ready" label? @nikku

[nikku]

@azeghers mentions an important thing that we should talk about it in our weekly tomorrow.

The quirk is, that extensions can plug-in rows (and columns) and we need to find a way to handle that situation.

To evaluate the urgency on this bug, I'd look back and understand when it was introduced. Is it an old one or did we just recently break this behavior?

[barmac]

The bug was introduced via my context menu PR. Camunda Modeler 4.2 does not include it yet.

[azeghers]

This issue will sit in the backlog until the bug is reported by users.

Findings :

• Because annotations are nothing more than a description of a rule in the business logic, its cells don't have a distinct id from the row. This causes the navigation logic to identify all cells that possess this row id, namely the index cell and the annotation cell. The queried cell is incidentally the former, the index cell.
• This causes the navigation to try and select a cell that doesn't have coordinates, which isn't possible. A side effect of the impossibility to change focus is that a new rule is added in the process.
• A very quick hack that was not accepted at this time would be to always select the latter of the 2 cells with the same id, since the only possible case for this situation to happen is selecting an annotation cell. This snippet would have been added to the Cell Selection utility file

export function getSelectableNodeById(elementId, container) {
  const cells = queryAll(`[data-element-id="${ cssEscape(elementId) }"]`, container);
  return cells[cells.length - 1];
}

• The current implementation makes it difficult to solve this issue without refactoring a bigger chunk of its navigation system.
• Possible alternatives :
  • Refactor the table logic to be completely separate from the DMN logic behind
  • Use dom manipulation instead of ids and coordinates.