Skip to content

fix(label): improve editing position and 'label changed" event #567

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.

Sign up for GitHub

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

Merged
tbouffard merged 1 commit into from
Nov 21, 2024
Merged

fix(label): improve editing position and 'label changed" event #567

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/core/src/view/handler/CellEditorHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ class CellEditorHandler implements GraphPlugin {
this.textarea.style.minHeight = '1em';
}

this.textarea.style.position = 'relative';
this.textarea.style.position = 'absolute';

@tbouffard tbouffard Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Have you checked this change outside the StoryBook stories to see if there's any change in behavior? In the js-example or ts-example applications provided in this repository, for example?
In mxGraph, the position was calculated dynamically: https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/view/mxCellEditor.js#L281
It seems to have been removed during migration and I don't know why. Probably because of the name of the function and its comments, even though it seems to have had an impact on modern browsers (and not only in IE 8).

Here, we could at least introduce a way to set the value with a new property and let people decide what they want to use as a position.
I haven't thought this through any further, so feel free to suggest alternatives or contradict the proposal. 😸

@HalilFocic HalilFocic Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked right now, and on main I have this issue where if i just add cellsEditable and try to dobuleclick, the tooltip goes to bottom like this:

image

This image maybe doesn't show much detail, but I double click on the topleft blue rectangle and it scrolled the graph down and focused on textarea which is misplaced.

Switching to my branch and clicking on it seemed to work as intented.

I am pretty sure reason for this behaviour is that textarea is being added like this:
this.graph.container.appendChild(textarea);

If we put relative it will go to the bottom of the graph, then move but its still taking same space.

@tbouffard tbouffard Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, I also tested with ts-example and other standalone applications.
I reproduce the problem for both the vertices and the edges.

image

In the mxGraph code, a check was done on the SVG node in the container. Generally, no style is applied to it so isLegacyEditor returned true and the absolute position was then used for the "text edition div". See https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/view/mxCellEditor.js#L750
The relative position was applied to the "text edition div" when the SVG node had a position explicitly set to absolute.

I tested the main branch (064c349) by applying an absolute position to the SVG. Using the relative position correctly position the "text edition div" in this case. But using the absolute position works as well, so I guess we can always set the absolute position.

this.installListeners(this.textarea);
}

Expand Down Expand Up @@ -854,7 +854,6 @@ class CellEditorHandler implements GraphPlugin {
const initial = this.initialValue;
this.initialValue = null;
this.editingCell = null;
this.trigger = null;
this.bounds = null;
textarea.blur();
clearSelection();
Expand Down Expand Up @@ -882,7 +881,7 @@ class CellEditorHandler implements GraphPlugin {
}
});
}

this.trigger = null;

@tbouffard tbouffard Nov 12, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: do you know why this statement must be moved?
In mxGraph, it was at the former place and it worked: https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/view/mxCellEditor.js#L968

@HalilFocic HalilFocic Nov 12, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so this trigger is inside stopEditing function. We change the label inside the if statement if innerHTML is not equal to initial which is okay and we call the applyValue function which takes state and the value, which also seemed fine.
Problem arised because the applyValue function used this.trigger when calling labelChanged function and labelChanged always received null. This is because no matter what, we set the trigger to null even before checking if values has changed. I can look up logic inside the mxGraph for this aswell, but I since the editing of value doesn't work in the example, I would assume they had the bug too.

@tbouffard tbouffard Nov 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks for the information I got what you mean 😄

I didn't understand why this change was needed at a first glance, because most stories and examples work without this change.
The Editing story relies on the triggered event in the overridden Graph.labelChanged method. It needs the event to be correctly set to correctly assign the label (using the custom Cell.value which is an object and not a string, which stores the 2 label values). See https://github.com/maxGraph/maxGraph/blob/v0.13.1/packages/html/stories/Editing.stories.js#L116
Here is video showing the problem (i.e. the labels are not updated) without the change 👇🏿

PR_567_Editing_story_required_change_in_trigger_management.webm

The regular Graph.labelChanged method doesn't directly used the event (except to send another event which is not used in stories). That is why, most of the time, the existing code was working. 😸

ℹ️ Notice that such a fix has been proposed in the past in 4a63940#diff-470b5171fde7755241a36fb45a75aec1a323913b5088e08646d182cfda9dc86c

ℹ️ For the record, the mxGraph "Editing" example doesn't work: https://jgraph.github.io/mxgraph/javascript/examples/editing.html. It doesn't set the label correctly after edition, for all reasons explained in this thread.

// Forces new instance on next edit for undo history reset
if (this.textarea) InternalEvent.release(this.textarea);

Expand Down