Skip to content

Conversation

@dididy
Copy link
Contributor

@dididy dididy commented Aug 30, 2025

What is this PR for?

Description:
Cursor behavior in the New UI’s paragraph needs to be fixed for several cursor related actions, including double-clicking, running all above/below, adding(clone), and removing paragraphs.

When cloneParagraph() is called, it internally calls addParagraph(), which has already been tested. The same addParagraph-related code is also applied in #5044. If either PR is merged first, I will rebase accordingly. I have also confirmed that cloneParagraph() works correctly through #5044.

Due to timing issues, I used setTimeout for removeParagraph() and doubleClickParagraph(). Since this is in the UI area, it likely won’t have major side effects, but I will look into it further.

Expected:

  • When doubleClickParagraph() is executed, the cursor should move to the end of the paragraph.
  • When runAllAbove() or runAllBelow() is executed, the current cursor position should be remembered, and after execution, focus should return to the previous cursor position.
  • When addParagraph() is executed, the newly added paragraph should receive focus.
  • When removeParagraph() is executed, focus should move to the paragraph that takes the deleted paragraph’s place.

Actual (New UI):

  • When doubleClickParagraph() is executed, the cursor moves to the beginning instead of the end.
  • After runAllAbove() or runAllBelow(), focus is lost completely.
  • When addParagraph() is executed, the new paragraph does not automatically receive focus.
  • After removeParagraph(), focus may not move to the correct paragraph.

[Appropriate action - Classic UI]

2025-08-30.12.17.17.mp4

Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph

[AS-IS]

2025-08-30.12.24.21.mp4

Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph

[TO-BE]

2025-08-30.12.18.22.mp4

Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? N
  • Is there breaking changes for older versions? N
  • Does this needs documentation? N

Copy link
Contributor

@tbonelee tbonelee left a comment

Choose a reason for hiding this comment

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

I've verified that it behaves as you intended. I've added some comment questions on a few parts of the code to better understand your intent.

Comment on lines 134 to 139
setTimeout(() => {
const adjustedCursorIndex =
paragraphIndex === definedNote.paragraphs.length ? paragraphIndex - 1 : paragraphIndex + 1;
this.listOfNotebookParagraphComponent.find((_, index) => index === adjustedCursorIndex)?.focusEditor();
this.cdr.markForCheck();
}, 250);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the setTimeout value of 250 selected as a heuristic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to ensure that the removed paragraph in definedNote.paragraphs is reflected in this.listOfNotebookParagraphComponent?

Copy link
Contributor Author

@dididy dididy Sep 4, 2025

Choose a reason for hiding this comment

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

f49113f

After investigating from multiple angles, I found that when receiving PARAGRAPH_REMOVED and executing focusEditor on the target paragraph, blur events such as onEditorBlur and blurEditor inside the paragraph component were being triggered, causing an issue.

Unlike ADD_PARAGRAPH, which works without special handling, this problem seems to occur because nzModalService is involved.
(If messageService.paragraphRemove is executed directly inside the paragraph component’s removeParagraph function without nzModalService, the issue does not occur)

Since removing nzModalService is not an appropriate solution, I introduced an ignoreBlur variable to suppress the blur events and re-apply focus as a fix.

@dididy
Copy link
Contributor Author

dididy commented Sep 6, 2025

this.nzModalService
      .confirm({
        ...
      })
      .afterClose.pipe(takeUntil(this.destroy$))
      .subscribe(() => {
        // You can execute focus-related events (or other logic) at the point when the modal is closed.
      });

Other functions that trigger a modal (such as runAllAbove and runAllBelowAndCurrent) can subscribe to the modal’s close event to execute focus-related logic, and I actually resolved the issue that way before.

However, removeParagraph is different: it deletes the current paragraph, and we need to set focus on the paragraph that comes after it. If we handled it the same way as the others, we would end up trying to focus a paragraph that has already been deleted (or is about to be).

The root cause is that Ant Design’s modal steals focus from Monaco editor. This creates a timing issue: even if we try to focus the next paragraph after deletion, as long as the modal is not fully destroyed, a blur event will still be triggered. I found in the documentation that there’s an nzAutofocus option, but this only designates which button in the modal should receive initial focus. Since the paragraph we want to focus isn’t inside the modal, this option isn’t useful (I temporarily set it to null, but I’ll remove it if we agree it’s unnecessary).

Therefore, the approach I came up with is to wait until the modal is closed and then reapply focus after the last blur event is triggered.

4434380

Previously, I introduced a variable called ignoreBlur to work around this. But while looking deeper into the documentation, I discovered that we can check nzModalService.openModals.length to detect when the modal has closed. So I refactored the code to remove the ignoreBlur variable and use this method instead.

@tbonelee
Copy link
Contributor

tbonelee commented Sep 6, 2025

Thanks for digging into this with multiple approaches.

Do we agree that focus() should run only after the modal is fully closed?
If so, what do you think about triggering this.messageService.paragraphRemove(this.paragraph.id) after the modal closes?

I confirmed that the return value of nzOnOk() is emitted to afterClose subject. Using that, we can run the paragraph removal (including focus() to the appropriate paragraph) once the modal is definitely closed. This worked in my local testing(tested version: tbonelee@7a64b2e).

Could you review this approace and share your thoughts?

-        this.nzModalService.confirm({
-          nzTitle: 'Delete Paragraph',
-          nzContent: 'Do you want to delete this paragraph?',
-          nzAutofocus: null,
-          nzOnOk: () => {
-            this.messageService.paragraphRemove(this.paragraph.id);
-            this.cdr.markForCheck();
-          }
-        });
+        this.nzModalService
+          .confirm({
+            nzTitle: 'Delete Paragraph',
+            nzContent: 'Do you want to delete this paragraph?',
+            nzOnOk: () => true
+          })
+          .afterClose.pipe(takeUntil(this.destroy$))
+          .subscribe(result => {
+             // result is undefined if a user cancels
+            if (result) {
+              this.messageService.paragraphRemove(this.paragraph.id);
+              this.cdr.markForCheck();
+            }
+          });

@dididy
Copy link
Contributor Author

dididy commented Sep 7, 2025

4fc743d

I’ve confirmed that your suggested solution works well. It turns out that instead of invoking REMOVE_PARAGRAPH at the moment the OK button is clicked, it should simply be triggered once the modal has closed. I realize I was approaching it in a more complicated way than necessary. Thank you very much for taking the time to look into this issue with me.

tbonelee
tbonelee previously approved these changes Sep 7, 2025
Copy link
Contributor

@tbonelee tbonelee left a comment

Choose a reason for hiding this comment

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

Appreciate you fixing the repeated feedback. LGTM.

@dididy
Copy link
Contributor Author

dididy commented Sep 7, 2025

I rebased due to conflicts caused by merged PRs.

@tbonelee
Copy link
Contributor

tbonelee commented Sep 7, 2025

Could you please check the failing CI job?

@tbonelee tbonelee merged commit 4fbfaec into apache:master Sep 7, 2025
16 of 18 checks passed
tbonelee pushed a commit that referenced this pull request Sep 7, 2025
### What is this PR for?
**Description:**
Cursor behavior in the New UI’s paragraph needs to be fixed for several cursor related actions, including double-clicking, running all above/below, adding(clone), and removing paragraphs.

When **cloneParagraph()** is called, it internally calls **addParagraph()**, which has already been tested. The same addParagraph-related code is also applied in #5044. If either PR is merged first, I will rebase accordingly. I have also confirmed that **cloneParagraph()** works correctly through #5044.

Due to timing issues, I used `setTimeout` for **removeParagraph()** and **doubleClickParagraph()**. Since this is in the UI area, it likely won’t have major side effects, but I will look into it further.

**Expected:**
- When **doubleClickParagraph()** is executed, the cursor should move to the end of the paragraph.
- When **runAllAbove()** or **runAllBelow()** is executed, the current cursor position should be remembered, and after execution, focus should return to the previous cursor position.
- When **addParagraph()** is executed, the newly added paragraph should receive focus.
- When **removeParagraph()** is executed, focus should move to the paragraph that takes the deleted paragraph’s place.

**Actual (New UI):**
- When **doubleClickParagraph()** is executed, the cursor moves to the beginning instead of the end.
- After **runAllAbove()** or **runAllBelow()**, focus is lost completely.
- When **addParagraph()** is executed, the new paragraph does not automatically receive focus.
- After **removeParagraph()**, focus may not move to the correct paragraph.

**[Appropriate action - Classic UI]**

https://github.com/user-attachments/assets/fc0066f7-4e03-4e3b-9d5b-2f33df415ba7

Run all above -> Run all below -> Double click .md paragraph  -> Add paragraph -> Delete paragraph

**[AS-IS]**

https://github.com/user-attachments/assets/f699f788-cf29-4c4c-8c47-2ef34d7962f0

Run all above -> Run all below -> Double click .md paragraph  -> Add paragraph -> Delete paragraph

**[TO-BE]**

https://github.com/user-attachments/assets/1206c524-103f-4328-85ee-04408073b628

Run all above -> Run all below -> Double click .md paragraph  -> Add paragraph -> Delete paragraph

### What type of PR is it?
Bug Fix

### Todos

### What is the Jira issue?
* [[ZEPPELIN-6298](https://issues.apache.org/jira/browse/ZEPPELIN-6298)]

### How should this be tested?

### Screenshots (if appropriate)

### Questions:
* Does the license files need to update? N
* Is there breaking changes for older versions? N
* Does this needs documentation? N

Closes #5057 from dididy/fix/ZEPPELIN-6298.

Signed-off-by: ChanHo Lee <[email protected]>
(cherry picked from commit 4fbfaec)
Signed-off-by: ChanHo Lee <[email protected]>
@tbonelee
Copy link
Contributor

tbonelee commented Sep 7, 2025

Thanks, merged into master and branch-0.12

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.

2 participants