6726690: SwingUtilities.replaceUI*Map() methods do not remove previously installed maps #28671

prsadhuk · 2025-12-05T03:01:05Z

SwingUtilities.replaceUIInputMap() and SwingUtilities.replaceUIActionMap() do not actually remove previously installed maps as their Javadoc indicates if null is passed as uiInputMap/uiActionMap

jdk/src/java.desktop/share/classes/javax/swing/SwingUtilities.java

Lines 1802 to 1803 in 7e91d34

    
                * to <code>uiInputMap</code>. If <code>uiInputMap</code> is {@code null}, 
        
                * this removes any previously installed UI InputMap.

jdk/src/java.desktop/share/classes/javax/swing/SwingUtilities.java

Lines 1827 to 1828 in 7e91d34

    
                * to <code>uiActionMap</code>. If <code>uiActionMap</code> is {@code null}, 
        
                * this removes any previously installed UI ActionMap.

If the passed uiInputMap/uiActionMap is null, JComponent actually doesn't create a fresh map and returns the previously installed map

jdk/src/java.desktop/share/classes/javax/swing/JComponent.java

Lines 2586 to 2595 in 7e91d34

    
           case WHEN_IN_FOCUSED_WINDOW: 
        
               if (getFlag(WIF_INPUTMAP_CREATED)) { 
        
                   return windowInputMap; 
        
               } 
        
               // Hasn't been created yet. 
        
               if (create) { 
        
                   ComponentInputMap km = new ComponentInputMap(this); 
        
                   setInputMap(condition, km); 
        
                   return km; 
        
               }

which is in contradiction to the replaceUI*Map spec so SwingUtilities needs to clear the previously installed map which is being done in this fix.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-6726690: SwingUtilities.replaceUI*Map() methods do not remove previously installed maps (Bug - P4)

Reviewers

Alexander Zvegintsev (@azvegint - Reviewer)
Tejesh R (@TejeshR13 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28671/head:pull/28671
$ git checkout pull/28671

Update a local copy of the PR:
$ git checkout pull/28671
$ git pull https://git.openjdk.org/jdk.git pull/28671/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28671

View PR using the GUI difftool:
$ git pr show -t 28671

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28671.diff

Using Webrev

Link to Webrev Comment

…sly installed maps

bridgekeeper · 2025-12-05T03:02:21Z

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-12-05T03:02:49Z

@prsadhuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

6726690: SwingUtilities.replaceUI*Map() methods do not remove previously installed maps

Reviewed-by: azvegint, tr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 526 new commits pushed to the master branch:

a5968f9: 8371968: RISC-V: implement AES CBC intrinsics
d36a234: 8368701: CDS VerifierTest_1A.java failed on machines with 512 GB RAM
b6732d6: 8371603: C2: Missing Ideal optimizations for load and store vectors on SVE
... and 523 more: https://git.openjdk.org/jdk/compare/3f40f4c362f6ff4d1ec7d513b4690ed5fade3e2a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-12-05T03:03:34Z

@prsadhuk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-12-05T03:12:48Z

Webrevs

azvegint · 2025-12-08T11:00:14Z

src/java.desktop/share/classes/javax/swing/SwingUtilities.java

                return;
            }
            map = parent;
        }
+        if (map != null && uiInputMap == null) {


Line 1826 seems to be unreachable, as we have while (map != null) { before in the same method.

you are right..removed..

azvegint · 2025-12-08T14:50:26Z

test/jdk/javax/swing/SwingUtilities/UIMapTest.java

+        // Show the frame
+        JFrame frame = new JFrame("UIMapTest");
+        frame.add(button);


Do we really need this?

Suggested change

// Show the frame

JFrame frame = new JFrame("UIMapTest");

frame.add(button);

In its current state, the test will fail on headless systems because the test does not have the @key headful.

azvegint · 2025-12-08T14:52:33Z

test/jdk/javax/swing/SwingUtilities/UIMapTest.java

+        map.put(KeyStroke.getKeyStroke("released ENTER"), "released");
+
+        // Add the map
+        SwingUtilities.replaceUIInputMap(button, JComponent.WHEN_IN_FOCUSED_WINDOW, map);


Although the fix changes both replaceUIInputMap and replaceUIActionMap, only replaceUIInputMap is tested.

ActionMap test added

azvegint · 2025-12-08T14:53:03Z

src/java.desktop/share/classes/javax/swing/SwingUtilities.java

@@ -1815,6 +1815,9 @@ public static void replaceUIInputMap(JComponent component, int type,
            InputMap parent = map.getParent();
            if (parent == null || (parent instanceof UIResource)) {
                map.setParent(uiInputMap);
+                if (uiInputMap == null) {
+                    map.clear();
+                }


What if if (parent == null || (parent instanceof UIResource)) { is false and uiInputMap == null?
Should we also clear the map?

Addressed..

azvegint

I assume that testing looks good on all platforms.

azvegint · 2025-12-09T09:03:09Z

src/java.desktop/share/classes/javax/swing/SwingUtilities.java

Another approach would be to nullify the map.
In this case we don't modify the previously stored map. If a user stores a link to it, they can reuse it later.

Suggested change

if (uiActionMap == null) {

component.setActionMap(null);

return;

}

ActionMap map = component.getActionMap(true);

However, this will also require updating the JComponent code to reset the flags.

jdk/src/java.desktop/share/classes/javax/swing/JComponent.java

Lines 2524 to 2527 in cba09cd

public final void setActionMap(ActionMap am) {

actionMap = am;

setFlag(ACTIONMAP_CREATED, true);

}

It may be worth considering this option.

I had tried with nullify by using setInputMap(null) initially but there was some CI issue with some tests with that approach so I followed this and with this, the fix is contained in this class.

.JComponent's flag set/reset method is private so any change there possibly would have needed CSR so I guess this is good for this issue..

JComponent's flag set/reset method is private so any change there possibly would have needed CSR so I guess this is good for this issue..

or just change the implementation without changing the specification

TejeshR13

LGTM

prsadhuk · 2025-12-10T04:30:14Z

/integrate

openjdk · 2025-12-10T04:31:43Z

Going to push as commit 1bbbce7.
Since your change was applied there have been 526 commits pushed to the master branch:

a5968f9: 8371968: RISC-V: implement AES CBC intrinsics
d36a234: 8368701: CDS VerifierTest_1A.java failed on machines with 512 GB RAM
b6732d6: 8371603: C2: Missing Ideal optimizations for load and store vectors on SVE
... and 523 more: https://git.openjdk.org/jdk/compare/3f40f4c362f6ff4d1ec7d513b4690ed5fade3e2a...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-12-10T04:31:50Z

@prsadhuk Pushed as commit 1bbbce7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

prrace · 2025-12-10T17:56:22Z

This change worries me. How was it tested ?
I see MANY places (dozens and dozens) that Swing internally passes null to these calls.
The behaviour of these is now changed.
@prsadhuk please explain how this is a safe change and if you examined all of the usages of this with null ?

prsadhuk · 2025-12-11T04:23:41Z

This change worries me. How was it tested ? I see MANY places (dozens and dozens) that Swing internally passes null to these calls. The behaviour of these is now changed. @prsadhuk please explain how this is a safe change and if you examined all of the usages of this with null ?

In all cases, Swing passes null to these calls in uninstallKeyboardActions during uninstallUI that is, during L&F change and it doesn't check if the map is removed or not, even though spec mandates it should. It would have seen the issue if uninstall* APIs had tried to use the same map
but map is reloaded and replaced by calling these methods in installUI so it was never problematic.
I dont know why this spec violation is never caught, apart from this bug, even by JCK test.

I have tested with all clientlibs test in CI along with SwingSet2 with L&F change and keyboard navigation works ok.

prsadhuk added 2 commits December 5, 2025 02:44

6726690: SwingUtilities.replaceUI*Map() methods do not remove previou…

60856f5

…sly installed maps

Test fix

a2fe949

openjdk bot added the client client-libs-dev@openjdk.org label Dec 5, 2025

Test fix

63a5ac1

openjdk bot added the rfr Pull request is ready for review label Dec 5, 2025

azvegint reviewed Dec 8, 2025

View reviewed changes

prsadhuk added 2 commits December 8, 2025 13:10

Redundant code remove

002b018

Redundant code remove

847b6ba

azvegint reviewed Dec 8, 2025

View reviewed changes

Fix update...ActionMap test added

8502827

azvegint approved these changes Dec 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 9, 2025

TejeshR13 approved these changes Dec 10, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 10, 2025

openjdk bot closed this Dec 10, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 10, 2025

prsadhuk deleted the JDK-6726690 branch December 10, 2025 04:32

	* to <code>uiInputMap</code>. If <code>uiInputMap</code> is {@code null},
	* this removes any previously installed UI InputMap.

	* to <code>uiActionMap</code>. If <code>uiActionMap</code> is {@code null},
	* this removes any previously installed UI ActionMap.

	case WHEN_IN_FOCUSED_WINDOW:
	if (getFlag(WIF_INPUTMAP_CREATED)) {
	return windowInputMap;
	}
	// Hasn't been created yet.
	if (create) {
	ComponentInputMap km = new ComponentInputMap(this);
	setInputMap(condition, km);
	return km;
	}

	// Show the frame
	JFrame frame = new JFrame("UIMapTest");
	frame.add(button);

+if (uiActionMap == null) {
+    component.setActionMap(null);
+    return;
+}
+ActionMap map = component.getActionMap(true);

	public final void setActionMap(ActionMap am) {
	actionMap = am;
	setFlag(ACTIONMAP_CREATED, true);
	}

6726690: SwingUtilities.replaceUI*Map() methods do not remove previously installed maps #28671

6726690: SwingUtilities.replaceUI*Map() methods do not remove previously installed maps #28671

Uh oh!

Conversation

prsadhuk commented Dec 5, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Dec 5, 2025

Uh oh!

openjdk bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

azvegint Dec 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azvegint left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk Dec 9, 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!

Choose a reason for hiding this comment

Uh oh!

TejeshR13 left a comment

Choose a reason for hiding this comment

Uh oh!

prsadhuk commented Dec 10, 2025

Uh oh!

openjdk bot commented Dec 10, 2025

Uh oh!

openjdk bot commented Dec 10, 2025

Uh oh!

prrace commented Dec 10, 2025

Uh oh!

prsadhuk commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

prsadhuk commented Dec 5, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 5, 2025 •

edited

Loading

openjdk bot commented Dec 5, 2025 •

edited

Loading

mlbridge bot commented Dec 5, 2025 •

edited

Loading

azvegint Dec 8, 2025 •

edited

Loading

prsadhuk Dec 9, 2025 •

edited

Loading