8278620: properties installed by javax.swing.LookAndFeel installColors and installColorsAndFont are not uninstalled

[SWinxy]

Many installDefaults methods set the font, foreground, and background on objects but their inverse methods uninstallDefaults do not remove them. I've added an inverse method to remove the colors and font to call for the uninstallDefaults methods that install defaults.

AquaButtonUI can call its super since it would otherwise be repeated code. BasicComboBoxUI (weirdly) installs the properties again when it should be uninstalling them, so I changed.

I noticed that, in a few subclasses, only one of calls to the super of installDefaults and uninstallDefaults are made. That is, an overridden installDefaults may call its super while the overridden uninstallDefaults does not call its super (or vise versa). These classes are: AquaTabbedPaneUI, SynthMenuItemUI, SynthSplitPaneUI, and XTextAreaPeer.

Sorry I couldn't write a test; I wasn't sure how I should have accessed the protected variable aside from creating extending classes for each class that changed.

See also #6603, where this issue was discovered.

---

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
• Change requires CSR request JDK-8298350 to be approved

Issues

• JDK-8278620: properties installed by javax.swing.LookAndFeel installColors and installColorsAndFont are not uninstalled
• JDK-8298350: properties installed by javax.swing.LookAndFeel installColors and installColorsAndFont are not uninstalled (CSR)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10565

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back SWinxy! 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]

@SWinxy 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]

Webrevs

• 05: Full (ceb2503c)
• 04: Full - Incremental (7dfcaf25)
• 03: Full - Incremental (1cc42222)
• 02: Full - Incremental (1939190c)
• 01: Full - Incremental (009855eb)
• 00: Full (c5b9b48d)

[bridgekeeper]

@SWinxy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[aivanov-jdk]

I've started looking at this. It looks good, yet I just skimmed through the changes without thoroughly reviewing, I haven't run the tests yet.

[prrace]

Same as Alexey. It looks OK. I was not sure about new public API for the convenience method but
we aleady have precedent for these on LookAndFeel.
Any BACKPORT would have to be different.
I'd like to hear the results of Alexey's testing as the next step.

[prrace]

/csr

[openjdk]

@prrace has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@SWinxy please create a CSR request for issue JDK-8278620 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[SWinxy]

Can the CSR be skipped if the methods are made package-private?

[prrace]

Can the CSR be skipped if the methods are made package-private?

Well, yes, except you can't do that because they are called from other packages.

[prrace]

You could move the methods to some internal place like SwingUtilities2.java.
It would just seem very inconsistent with all the others.
But that's what you'd have to do for any backport that wanted the same "fix" to those L&Fs.

[bridgekeeper]

@SWinxy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[TheShermanTanker]

CSR at https://bugs.openjdk.org/browse/JDK-8298350, no worries

[prrace]

I have made a number of edits to the CSR.
There is some rewording which you need to copy back to here.
Also I didn't see the need to include the @see changes in other methods in the CSR. They are not spec-relevant and clutter it

[SWinxy]

I have made a number of edits to the CSR. There is some rewording which you need to copy back to here. Also I didn't see the need to include the @see changes in other methods in the CSR. They are not spec-relevant and clutter it

Done. The only thing I want is the oxford comma in the documentation to separate the background color and font wording.

[aivanov-jdk]

This change cause 23 test cases to fail across the platforms:

• sanity/client/SwingSet/src/SwingSet2DemoTest.java;
• javax/swing/GraphicsConfigNotifier/StalePreferredSize.java;
• two closed tests.

One JCK test fails; from a quick look, it's because the font is now uninstalled where it wasn't previously.

[prrace]

This change cause 23 test cases to fail across the platforms:

So I thought we'd got past the testing since a CSR was proposed. The CSR should have waited.

• sanity/client/SwingSet/src/SwingSet2DemoTest.java;
• javax/swing/GraphicsConfigNotifier/StalePreferredSize.java;
• two closed tests.

One JCK test fails; from a quick look, it's because the font is now uninstalled where it wasn't previously.

So all the test failures are blocker problem .. @aivanov-jdk, do any of the jtreg test failures characterise the JCK failure ?
Meaning something the submitter can use as a proxy to understand the problem
Do these failures suggest that there's a good reason why these fonts and colors are not installed ?
Or do we have 23 (!) tests which have an assumption they probably shouldn't have ?
Possible, but in that case, would apps have the same assumptions ?

[SWinxy]

So all the test failures are blocker problem

100%. I'll see what I can learn from the failed tests, but my hunch is that many fail due to incorrect assumptions.

[aivanov-jdk]

One JCK test fails; from a quick look, it's because the font is now uninstalled where it wasn't previously.

I confirmed it's a test problem. The JCK test needs to be updated. Details are in a confidential comment in JBS.

Do I need to submit a bug against JCK?

do any of the jtreg test failures characterise the JCK failure ?

The failure of javax/swing/GraphicsConfigNotifier/StalePreferredSize.java may be related. The test fails for JLabel.

The test calls SwingUtilities.updateComponentTreeUI(frame) and expects the preferred size doesn't change. Yet it does change.

The failure in sanity/client/SwingSet/src/SwingSet2DemoTest.java is also related. It fails because of timeout where a font on a component (JButton presumably) doesn't change to bold after selecting Theme → Font → Bold in the menu. Selecting this menu item results in SwingUtilities.updateComponentTreeUI(frame) being called to update the look and feel properties of all the components.

As for the closed test failure, it doesn't look related. A component being painted is null. It shouldn't happen.

[bridgekeeper]

@SWinxy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[SWinxy]

I'm really not sure how to fix these. I've tried digging into the tests. It looks like uninstalling the fonts from the components screws with other parts of Swing, specifically that delegates are calling getFont after they've been uninstalled? At least that's my guess. I'm unsure how to continue. @aivanov-jdk, could you please help me with this?

[SWinxy]

Found 👏 the 👏 problem! Removing the font from the JPanel causes a cascade of other components not getting the correct font objects when the component tree is updated. That is, the second time the font is installed, it is somehow different. In BasicPanelUI#uninstallDefaults, I've replaced uninstallColorsAndFont with uninstallColors for now. I've tested the two tests that Alexey said fail, and they both work again.
The components that don't receive the correct font are JLabel, JButton, JMenuItem, JList, JComboBox, JToolTip, and JSpinner. Figuring out what is causing that is not for almost-5AM-me. 🫠

[TheShermanTanker]

Heh, stuff like this happens much too often when you work on the JDK ;)

[SWinxy]

Figured it out. When setting a Component's font to null, it inherits the font from the component tree. (Some classes omit this fact from their respective documentations.) Window peers will check isFontSet when they are created, and will set it to a Font if no font is set. The problem is that installing the font checks if the font is critically a FontUIResource. This means that setFont(null) is called on FontUIResource fonts, the code goes up the hierarchy to non-null fonts, and finds a non-FontUIResource font, and sets the component to that font, which is originally set by the component peers on creation, and then the components can't install a default because it will replace a null or a FontUIResource font. Changing the fallback font type to a FontUIResource in the peers fixes this. I hope this doesn't break anything else lol. (It probably will.)

[prrace]

Figured it out. When setting a Component's font to null, it inherits the font from the component tree.

Good to know but ..

Changing the fallback font type to a FontUIResource in the peers fixes this.

Sorry, but I don't think we should do this.

FontUIResource is something devised by Swing, for Swing.
Making AWT components depend on it for the convenience of Swing is backwards.
And I'm quite surprised that WComponentPeer is EVER used by Swing.
AWT components on Windows are heavyweight - and that peer is for them and Swing is lightweight.

Did you actually try all 3 platforms ?

And if a default font prevents the FontUIResource from being installed, how does it get installed the in the first place ?
Perhaps that first time the component has no peer and it really is null ?

It seems you would have to explicitly track whether the app set the font omn the JComponent rather than using this mechanism .. and if it did not then ignore whatever the font is ??
But I'm guessing and it adds just another bit more risk+complexity to this change.

[SWinxy]

And if a default font prevents the FontUIResource from being installed, how does it get installed the in the first place ?
Perhaps that first time the component has no peer and it really is null ?

The font is null when it's created. Calling getFont causes it to go up the hierarchy, if it has a parent. The UI looks to be initialized (and thus a font is set) before getFont is called.

FontUIResource is something devised by Swing, for Swing.
Making AWT components depend on it for the convenience of Swing is backwards.

D'oh.

It seems you would have to explicitly track whether the app set the font omn the JComponent rather than using this mechanism .. and if it did not then ignore whatever the font is ??
But I'm guessing and it adds just another bit more risk+complexity to this change.

Sounds like the path would be to undo my last commit and just put a note in the code.

[prrace]

And if a default font prevents the FontUIResource from being installed, how does it get installed the in the first place ?
Perhaps that first time the component has no peer and it really is null ?

The font is null when it's created. Calling getFont causes it to go up the hierarchy, if it has a parent. The UI looks to be initialized (and thus a font is set) before getFont is called.

That is what I guessed. Accidental or deliberate ? I'd have to spend time to know.

FontUIResource is something devised by Swing, for Swing.
Making AWT components depend on it for the convenience of Swing is backwards.

D'oh.

It seems you would have to explicitly track whether the app set the font omn the JComponent rather than using this mechanism .. and if it did not then ignore whatever the font is ??
But I'm guessing and it adds just another bit more risk+complexity to this change.

Sounds like the path would be to undo my last commit and just put a note in the code.

But then there'd either be the correct font not installed or something else bad ?
How would a note in the code help ?

Maybe we are going about this all wrong ?
Maybe uninstall isn't needed ?
What are the rules (set by Swing?) for what a L&F should do when installing a UI ?
If it is "if there is a FontUIResource, then feel free to replace it with yours" then may be everything in
this PR (at least about fonts) is un-needed ?

This is code relied upon by tens of thousands of applications written over a period of 25 years.
A change like this is really risky, when you consider that there are probably 10 times more
apps than there are Swing regresssion tests .. and only a few of the regression (and other) tests cover this kind of scenario.

Testing on every platform of every test we have is a bare minimum.

In the end my point is that unless (and until) we see some application complain, these proactive changes are a bad trade-off.

[aivanov-jdk]

And if a default font prevents the FontUIResource from being installed, how does it get installed the in the first place ?
Perhaps that first time the component has no peer and it really is null ?

The font is null when it's created. Calling getFont causes it to go up the hierarchy, if it has a parent. The UI looks to be initialized (and thus a font is set) before getFont is called.

That is what I guessed. Accidental or deliberate ? I'd have to spend time to know.

This is the code of Component.getFont:

jdk/src/java.desktop/share/classes/java/awt/Component.java

Lines 1912 to 1927 in b4ea807

	public Font getFont() {
	return getFont_NoClientCode();
	}
	// NOTE: This method may be called by privileged threads.
	// This functionality is implemented in a package-private method
	// to insure that it cannot be overridden by client subclasses.
	// DO NOT INVOKE CLIENT CODE ON THIS THREAD!
	final Font getFont_NoClientCode() {
	Font font = this.font;
	if (font != null) {
	return font;
	}
	Container parent = this.parent;
	return (parent != null) ? parent.getFont_NoClientCode() : null;
	}

Until the UI is shown, there's no heavyweight peer, so the default font doesn't get set.

Even more, a UI instance is set from a component constructor:

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

Lines 176 to 182 in b4ea807

	public JLabel(String text, Icon icon, int horizontalAlignment) {
	setText(text);
	setIcon(icon);
	setHorizontalAlignment(horizontalAlignment);
	updateUI();
	setAlignmentX(LEFT_ALIGNMENT);
	}

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

Lines 85 to 90 in b4ea807

	public JPanel(LayoutManager layout, boolean isDoubleBuffered) {
	setLayout(layout);
	setDoubleBuffered(isDoubleBuffered);
	setUIProperty("opaque", Boolean.TRUE);
	updateUI();
	}

Thus, a font is (always) set on a JComponent when the constructor is called, the default L&F font would be an instance of UIResource.

However, if the font is reset to null, the default font (of heavyweight) components / peers comes into play.

The default font gets set in getGraphics:

jdk/src/java.desktop/windows/classes/sun/awt/windows/WComponentPeer.java

Lines 640 to 643 in b4ea807

	Font font = this.font;
	if (font == null) {
	font = defaultFont;
	}

jdk/src/java.desktop/unix/classes/sun/awt/X11/XWindow.java

Lines 380 to 383 in b4ea807

	Font font = afont;
	if (font == null) {
	font = XWindow.getDefaultFont();
	}

FontUIResource is something devised by Swing, for Swing.
Making AWT components depend on it for the convenience of Swing is backwards.

D'oh.

I agree. AWT should not depend on Swing.

Sounds like the path would be to undo my last commit and just put a note in the code.

But then there'd either be the correct font not installed or something else bad ?
How would a note in the code help ?

Maybe we are going about this all wrong ?
Maybe uninstall isn't needed ?
What are the rules (set by Swing?) for what a L&F should do when installing a UI ? If it is "if there is a FontUIResource, then feel free to replace it with yours" then may be everything in this PR (at least about fonts) is un-needed ?

With where the testing led us, I'm inclined to think that the omission of nullifying the font in uninstallUI wasn't accidental but rather a deliberate decision.

Yes, a Font object gets “leaked” after UI is uninstalled from a JComponent. In majority of cases uninstallUI is followed by installUI for another Look-and-Feel. (Without a UI delegate, any JComponent is unusable.) As such, the Font object gets replaced with a newer one if the previous one is UIResource or remains intact if it's not UIResource, which likely means it was explicitly set by an app developer.

The common rule for Swing: if a property is UIResource, replace it with a new default; otherwise preserve the property, don't change it. From this point of view, all the properties of a Swing component could remain untouched in uninstallUI. At the same time, many properties are reset to null.

There are issues where font doesn't get reset even though it should. The latest example that comes to my mind is JDK-6753661 and #12180. Were there others?

This current bug JDK-8278620 was submitted as the result of work on JDK-8277817 and #6603 where an object had been serialized, if I remember correctly, but the serialized form cannot be read because a class is removed in a later version of JDK. (The java/awt/dnd/BadSerializationTest/BadSerializationTest.java test seems to do the wrong thing: it reads a serialized version of a Swing object which was saved by a previous version of JDK, which isn't supported and which was mentioned in the comments.)

The description of JDK-8278620 states BasicListUI correctly uninstalls all its properties, including the font. Why is it not affected by the problem we're seeing here where the font gets reset to a non-ResourceUI font from the heavyweight peer of the frame?

This is code relied upon by tens of thousands of applications written over a period of 25 years. A change like this is really risky, when you consider that there are probably 10 times more apps than there are Swing regresssion tests .. and only a few of the regression (and other) tests cover this kind of scenario.

Testing on every platform of every test we have is a bare minimum.

In the end my point is that unless (and until) we see some application complain, these proactive changes are a bad trade-off.

Perhaps, font property should be left untouched just like it is now. However, resetting the color properties — foreground and background — doesn't seem to cause any issues. As such, the scope of this PR could be reduced to adding the LookAndFeel.uninstallColors method and using it in UI classes.

As a matter of fact, I ran the client tests with the latest changeset. The set of failing tests remains the same:

• javax/swing/GraphicsConfigNotifier/StalePreferredSize.java (on Windows only)
• one closed test;
• one JCK test.

Only the sanity/client/SwingSet/src/SwingSet2DemoTest.java test is gone from the list of failures.

So, this hasn't helped.

[openjdk]

@SWinxy this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8278620
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[SWinxy]

Apologies for the delay. I wrongly assumed that making the fonts FontUIResources would fix it. I only ran the tests on macOS, which is absolutely my fault. I'm not testing enough before submitting PRs. 😔

Maybe we are going about this all wrong ?
Maybe uninstall isn't needed ?
What are the rules (set by Swing?) for what a L&F should do when installing a UI ? If it is "if there is a FontUIResource, then feel free to replace it with yours" then may be everything in this PR (at least about fonts) is un-needed ?

With where the testing led us, I'm inclined to think that the omission of nullifying the font in uninstallUI wasn't accidental but rather a deliberate decision.

I don't think it was deliberate. Probably this same issue was ran into back then, and the easiest thing to do was to not remove the font.

The description of JDK-8278620 states BasicListUI correctly uninstalls all its properties, including the font. Why is it not affected by the problem we're seeing here where the font gets reset to a non-ResourceUI font from the heavyweight peer of the frame?

And I'm also not sure why! I'll do some more investigating. Thanks for the writeup, Alexey.

[bridgekeeper]

@SWinxy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@SWinxy This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.