ESC closes the preferences dialog when editing table. #14106

VishakhaMathur · 2025-10-19T08:59:52Z

Issue

The issue was that, earlier when we opened the citation key generator in the preferences and pressing esc did not close the dialog when we were inside the table of key patterns. For example, clicking on Article and then pressing esc didn't close the preference dialog. This was a bug.

Fix

What we tried to do was implement consistent functionality of esc across the table.
If we the table in preference dialog is being edited, and somebody wants to escape in between, pressing first esc clears the changes and second esc closes the dialog view. We used JavaFX and created event filter on the dialog scene (installGlobalCloseBehavior). It checks whether any table or list is being edited, if it is so, then first esc cancels the changes, otherwise close the dialog.

Steps to test

Test plan: Preferences dialog close behaviour (ESC vs edit)
Note: The below assumes ESC is bound to the Close dialog keyboard shortcut
A. Baseline: ESC closes when not editing

Open Preferences.
In the left sidebar, go to Citation key generator.
Scroll to Key patterns.
Single-click a table cell (e.g., Default pattern) so it’s focused but not editing.
Press the Close shortcut (default ESC).
Expected: The Preferences dialog closes.

B. Two-step ESC while editing (table)

Open Preferences → Citation key generator → Key patterns.
Double-click the Default pattern cell to enter edit mode (you should see a text editor in the cell).
Press ESC once.
Expected: Edit is cancelled; dialog stays open and the cell exits edit mode.
Press ESC again.
Expected: The Preferences dialog closes.

C. Other affected tabs (similar issue)
Repeat B in tabs (such as ‘Protected terms files’ and ‘Entry types) had the same incorrect close behaviour with TableView elements:
• Protected terms files → Select a cell in the table: start editing a row → ESC once cancels edit, ESC twice closes dialog.
• Entry types → Select a Entry Type on the left (e.g., ‘Article’) → Select a cell in the Required and optional fields table: start editing a row → ESC once cancels edit, ESC twice closes dialog.

Mandatory checks

I own the copyright of the code submitted and I license it under the [MIT license] (https://github.com/JabRef/jabref/blob/main/LICENSE)
I manually tested my changes in running JabRef (always required)
I added JUnit tests for changes (if applicable)
I added screenshots in the PR description (if change is visible to the user)
I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
I checked the user documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

…by cell-based controls (ListView/TableView/TreeView/TreeTableView)

koppor · 2025-10-19T12:26:41Z

Steps to test

Since, our OS is windows, we tried to run the app and found that the code works and aligns with the expectation. Everything worked smoothly, so we pushed all the changes.

I am not sure about your aims. Just get a PR merged (which is OK!) or to learn something about SE?

In case of the latter, how can a tester follow these steps? How precise is "aligns with the expectations"?

VishakhaMathur · 2025-10-19T14:07:46Z

Steps to test

Since, our OS is windows, we tried to run the app and found that the code works and aligns with the expectation. Everything worked smoothly, so we pushed all the changes.

I am not sure about your aims. Just get a PR merged (which is OK!) or to learn something about SE?

In case of the latter, how can a tester follow these steps? How precise is "aligns with the expectations"?

Hi Koppor, I am sorry for not mentioning test steps clearly. I have made changes in that part of description. I am new to OSS, I have never done something like this before. And I'll take care of all these things in the future projects.

VishakhaMathur · 2025-10-19T14:11:23Z

Steps to test

Since, our OS is windows, we tried to run the app and found that the code works and aligns with the expectation. Everything worked smoothly, so we pushed all the changes.

I am not sure about your aims. Just get a PR merged (which is OK!) or to learn something about SE?

In case of the latter, how can a tester follow these steps? How precise is "aligns with the expectations"?

Also, can you please help me with the check that is failing because I am not understanding why it is happening? I tried to follow the documentation and the jabref-machine comment above but it didn't fix it.

subhramit · 2025-10-19T18:01:36Z

Also, can you please help me with the check that is failing because I am not understanding why it is happening?

You just need to link the issue you fixed to this PR.
E.g. see the first line of the description of the merged PR #14102
Use Closes <link>

VishakhaMathur · 2025-10-20T00:24:46Z

Also, can you please help me with the check that is failing because I am not understanding why it is happening?

You just need to link the issue you fixed to this PR. E.g. see the first line of the description of the merged PR #14102 Use Closes <link>

Thanks for the reply, Subhramit. I followed what you suggested, but its still same. Is there anything else you think that is causing issues?

subhramit · 2025-10-20T07:09:01Z

Thanks for the reply, Subhramit. I followed what you suggested, but its still same. Is there anything else you think that is causing issues?

Look at the example in the PR template (this is the text that appeared in the PR description when you first opened the PR, which you had to replace):

<!-- LINK THE ISSUE WITH THE "Closes" KEYWORD. Example: Closes https://github.com/JabRef/jabref/issues/13109 OR Closes #13109 -->

Or the examples in the jabref-machine bot message:

Examples
✅ Fixes #xyz links pull-request to issue. Merging the PR will close the issue.
✅ Fixes https://github.com/JabRef/jabref/issues/xyz links pull-request to issue. Merging the PR will close the issue.

It says either link using #number OR the link to the issue.

Or what I said in my comment above:

Use Closes <link>

Instead of Closes <link>, you used Closes [#number](link).

Hope this is clear now.

P.S. Furthermore, the link that you provided in the brackets is not even a proper link to the issue, but a link to a comment on the issue.

VishakhaMathur · 2025-10-20T08:06:46Z

Thanks for the reply, Subhramit. I followed what you suggested, but its still same. Is there anything else you think that is causing issues?

Look at the example in the PR template (this is the text that appeared in the PR description when you first opened the PR, which you had to replace):

Or the examples in the jabref-machine bot message:

Examples
✅ Fixes #xyz links pull-request to issue. Merging the PR will close the issue.
✅ Fixes https://github.com/JabRef/jabref/issues/xyz links pull-request to issue. Merging the PR will close the issue.

It says either link using #number OR the link to the issue.

Or what I said in my comment above:

Use Closes <link>

Instead of Closes <link>, you used Closes [#number](link).

Hope this is clear now.

P.S. Furthermore, the link that you provided in the brackets is not even a proper link to the issue, but a link to a comment on the issue.

Thanks, it makes sense to me now. Sorry to bother you again but I have tried everything now. I added Closes but it is still failing. I also tried Fixes xyz, Fixes and Closes xyz, but it didn't work.

Siedlerchr · 2025-10-20T08:35:03Z

ignore the PR issue number, I think it sometimes doesn't work when you modify the original pr text

VishakhaMathur · 2025-10-20T09:42:06Z

ignore the PR issue number, I think it sometimes doesn't work when you modify the original pr text

Will our PR still be accepted and merged?

calixtus · 2025-10-20T10:03:24Z

Your pr will be reviewed and tested. Takes time, as we all are working full time and do JabRef in the spare time. Please be patient.

subhramit · 2025-10-20T13:12:47Z

Thanks, it makes sense to me now. Sorry to bother you again but I have tried everything now. I added Closes but it is still failing. I also tried Fixes xyz, Fixes and Closes xyz, but it didn't work

Hi Vishakha, no issues, thanks for pointing this out. There was an issue with our workflow that it didn't re-run on editing the PR description. Should be fixed by #14121.

VishakhaMathur · 2025-10-25T11:50:26Z

Hi there, I am just trying to follow up on this PR, since all the checks are passing now and all the previous issues have been fixed. Can you please let me know if there is any feedback or further changes we need to make?
Thanks

subhramit · 2025-10-25T12:52:50Z

Please go through the mandatory checks and act on what is applicable.
For once, your code cannot be accepted till you license it under the standard MIT licence.

subhramit

Some feedback on your changes.
You can address them.
However, since the code appears to be heavily written by AI, other maintainers will take a call whether to move forward with this PR.

subhramit · 2025-10-25T12:55:52Z

jabgui/src/main/java/org/jabref/gui/preferences/PreferencesDialogView.java

+        var root = getDialogPane();
+
+        for (var n : root.lookupAll(".list-view")) {
+            if (n instanceof javafx.scene.control.ListView<?> lv && lv.getEditingIndex() != -1) {


Use imports instead of raw library references.

We also avoid abbreviations

subhramit · 2025-10-25T12:56:20Z

jabgui/src/main/java/org/jabref/gui/preferences/PreferencesDialogView.java

+    }
+
+    private void installGlobalCloseBehavior() {
+        var scene = getDialogPane().getScene();


We avoid var and use explicit types

subhramit · 2025-10-25T12:57:14Z

jabgui/src/main/java/org/jabref/gui/preferences/PreferencesDialogView.java

+        for (var n : root.lookupAll(".table-view")) {
+            if (n instanceof javafx.scene.control.TableView<?> tv && tv.getEditingCell() != null) {
+                return true;
+            }
+        }
+
+        for (var n : root.lookupAll(".tree-view")) {
+            if (n instanceof javafx.scene.control.TreeView<?> trv && trv.getEditingItem() != null) {
+                return true;
+            }
+        }
+
+        for (var n : root.lookupAll(".tree-table-view")) {
+            if (n instanceof javafx.scene.control.TreeTableView<?> ttv && ttv.getEditingCell() != null) {
+                return true;


Similar comments applicable here

subhramit · 2025-10-25T12:57:37Z

jabgui/src/main/java/org/jabref/gui/util/BaseDialog.java

+// BaseDialog.java
+


Rationale behind this comment? Why was this file touched?

Rationale behind this comment? Why was this file touched?

In the beginning, when we were trying to understand what triggered the bug, we thought that BaseDialog.java might have some role to play because the ESC was not closing a dialog, although we were wrong, as it was in the Preferences tab. But, we didn't make any changes in the code of this file.

So, shall we change our implementation? Will changing the code make a difference?

No, but why was this comment added ^

I am sorry, I think the comment was added by mistake because I am not able to recall the reason behind adding it. It has now been removed in the new commit. I apologize for the oversight.

…r/jabref into fix-for-issue-8888

VishakhaMathur · 2025-10-25T14:24:57Z

Some feedback on your changes. You can address them. However, since the code appears to be heavily written by AI, other maintainers will take a call whether to move forward with this PR.

We got rid of the previous implementation and added a new one.

Callum-Higginson · 2025-10-25T14:56:32Z

Some feedback on your changes. You can address them. However, since the code appears to be heavily written by AI, other maintainers will take a call whether to move forward with this PR.

Apologies for the sloppy code in the initial fix, we should have better familiarised ourselves with the proper coding style. And yes, AI was used in the original code - we were not aware that was against JabRef's contribution policy, apologies again. Our new code hopefully remedies both these concerns as AI wasn't used this time around and I believe we have used proper coding style. Thanks for your review in advance.

Callum-Higginson and others added 2 commits October 19, 2025 13:32

Change preferences dialog behaviour so esc key press is not consumed …

ae51634

…by cell-based controls (ListView/TableView/TreeView/TreeTableView)

WIP: local changes before rebase

2a244a3

JabRef deleted a comment from jabref-machine Oct 20, 2025

subhramit mentioned this pull request Oct 20, 2025

Separate issue linking workflows, make them run on edited #14121

Merged

1 task

Merge branch 'main' into fix-for-issue-8888

53bdb65

subhramit requested changes Oct 25, 2025

View reviewed changes

u7663368 added 3 commits October 26, 2025 01:11

Changes in the fix

026b018

Merge branch 'fix-for-issue-8888' of https://github.com/VishakhaMathu…

287680d

…r/jabref into fix-for-issue-8888

added changes in the code

ff149a1

Callum-Higginson added 2 commits October 26, 2025 01:40

Merge remote-tracking branch 'origin/main' into fix-for-issue-8888

c36c778

Fix submodules

5b3eb1f

subhramit requested a review from Siedlerchr October 25, 2025 17:38

Uh oh!

ESC closes the preferences dialog when editing table. #14106

Are you sure you want to change the base?

ESC closes the preferences dialog when editing table. #14106

Uh oh!

Conversation

VishakhaMathur commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Fix

Steps to test

Mandatory checks

Uh oh!

koppor commented Oct 19, 2025

Steps to test

Uh oh!

VishakhaMathur commented Oct 19, 2025

Steps to test

Uh oh!

VishakhaMathur commented Oct 19, 2025

Steps to test

Uh oh!

subhramit commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VishakhaMathur commented Oct 20, 2025

Uh oh!

subhramit commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VishakhaMathur commented Oct 20, 2025

Uh oh!

Siedlerchr commented Oct 20, 2025

Uh oh!

VishakhaMathur commented Oct 20, 2025

Uh oh!

calixtus commented Oct 20, 2025

Uh oh!

subhramit commented Oct 20, 2025

Uh oh!

VishakhaMathur commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subhramit commented Oct 25, 2025

Uh oh!

subhramit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subhramit Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subhramit Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

VishakhaMathur Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

VishakhaMathur Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

VishakhaMathur Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VishakhaMathur commented Oct 25, 2025

Uh oh!

VishakhaMathur commented Oct 19, 2025 •

edited

Loading

subhramit commented Oct 19, 2025 •

edited

Loading

subhramit commented Oct 20, 2025 •

edited

Loading

VishakhaMathur commented Oct 25, 2025 •

edited

Loading

subhramit left a comment •

edited

Loading

subhramit Oct 25, 2025 •

edited

Loading

VishakhaMathur Oct 25, 2025 •

edited

Loading