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

fix: empty whitespaces label in group #2237

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

abdul99ahad
Copy link
Contributor

@abdul99ahad abdul99ahad commented Sep 18, 2024

Closes #2231

Proposed Changes

  • Enter whitespaces in group labels doesn't crash modeler
  • Whitespaces will be treated as no label

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@@ -251,7 +251,10 @@ export function setLabel(element, text) {
if (attr) {

if (attr === 'categoryValueRef') {
semantic['categoryValueRef'].value = text;
if (!semantic[attr]) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Watch for the contract of this function. If you early return, I propose you return element to not break it.

Alternative:

if (semantic[attr]) {
  // do change
}

My follow-up question is: Who calls this, and why, after the label has been removed? Will it ever be called with a non-empty text (in which case the above code would shadow a deeper issue?

Safe choice would be to be strict (ignore update only, if label is empty and categoryValueRef is unset).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree to early return the element.

The updateLabel event calls this method. Even if we remove the existing label, it is updated via this method. It is not called after the label is removed, but rather when we are updating the label during execute lifecycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the text is not empty, the label.create event will be fired and setCategoryValue function will be invoked in file GroupBehavior.js. So, in case the label is not empty, we will have categoryValueRef.

lib/util/LabelUtil.js Outdated Show resolved Hide resolved
@abdul99ahad abdul99ahad force-pushed the 2231-whitespaces-label-group branch 3 times, most recently from 9910997 to 0caf093 Compare September 23, 2024 18:24
@philippfromme philippfromme merged commit 303ca88 into develop Sep 25, 2024
12 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 25, 2024
@philippfromme philippfromme deleted the 2231-whitespaces-label-group branch September 25, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group name/label with whitespaces only crashes modeler
4 participants