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

JDK-8282772: JButton text set as HTML content has unwanted padding #8407

Closed

Conversation

DamonGuy
Copy link
Contributor

@DamonGuy DamonGuy commented Apr 26, 2022

The insets for buttons were incorrect for L&Fs except for Aqua when the text is set to HTML. This was fixed in Aqua by adding a conditional to check for the BasicHTML property key in the button component. This same logic can be used to fix Metal & Motif L&Fs in BasicButtonUI, but Nimbus is not fixed by this. Nimbus gets its default values from a skin.laf file, and when the defaults here are set to have left & right insets to 0 for ButtonUI, the issue is fixed. I also tested for non-HTML text after the changes, and the changes do not affect normal text.

The HtmlButtonImageTest has been changed to cycle through all L&Fs available on a device.


Progress

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

Issue

  • JDK-8282772: JButton text set as HTML content has unwanted padding

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8407/head:pull/8407
$ git checkout pull/8407

Update a local copy of the PR:
$ git checkout pull/8407
$ git pull https://git.openjdk.java.net/jdk pull/8407/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8407

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8407.diff

…ttonUI insets. Edited HtmlButtonImageTest to cycle all L&Fs. Recreated this branch to fix sync issue.
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 26, 2022

👋 Welcome back DamonGuy! 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 openjdk bot added the rfr Pull request is ready for review label Apr 26, 2022
@openjdk
Copy link

openjdk bot commented Apr 26, 2022

@DamonGuy 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.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Apr 26, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 26, 2022

Webrevs

@@ -268,7 +268,7 @@
</uiComponent>
<uiComponent opaque="false" type="javax.swing.JButton" name="Button" ui="ButtonUI" subregion="false">
<stateTypes/>
<contentMargins top="6" bottom="6" left="14" right="14"/>
<contentMargins top="6" bottom="6" left="0" right="0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I also tested for non-HTML text after the changes, and the changes do not affect normal text.

And this is true for this Nimbus case too ? Whereas your code update in BasicButtonUI is checking for HTML, I don't see how it could not change normal text in the Nimbus case.

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 thought the same thing. In the test, I forced the L&F to nimbus only and tested with the left and right insets set to 0 and 14. The text appears normally and identically for both cases (HTML text and non-HTML text). I also tried changing the size of the button from 37x37 to 100x100 and 200x200. The appearance was the same for all cases.

However, I just tried using different text from the tests before and I do get cases where text gets snipped to "..." when the new insets wouldn't. So, you're right that normal text is affected. I just used text that wasn't long enough in testing.

The reason I went this route for the fix was because in NimbusStyle, the insets are retrieved but the property key but the button component for BasicHTML is not set yet and returns null. So, I can't use this method to set the insets to 0 since the property key has not been set in the stack yet.

I finally found the applicable class of where to make the edits yesterday. I just made the changes similar to the other L&Fs.

@openjdk
Copy link

openjdk bot commented Apr 29, 2022

@DamonGuy 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:

8282772: JButton text set as HTML content has unwanted padding

Reviewed-by: prr, psadhukhan

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 293 new commits pushed to the master branch:

  • 89de756: 8286387: Remove unused FreeListAllocator::reduce_free_list
  • ae695d6: 8286476: x86_32: Fix crashes with non-preview mode after JDK-8284161 (Virtual Threads)
  • 87f3d2b: 8286446: PPC64: fix crashes after JDK-8284161 (virtual threads preview)
  • 7a2bbbb: 8286396: Address possibly lossy conversions in jdk.management.jfr
  • 9ac52b0: 8286392: Address possibly lossy conversions in jdk.jfr
  • f628966: 8286541: JFR: RecordingFile.write is missing "since 19"
  • f1554fc: 8285872: JFR: Remove finalize() methods
  • faa1aad: 8286515: JFR: Remove SimpleStringIdPool class
  • 7612bba: 8285698: Create a test to check the focus stealing of JPopupMenu from JComboBox
  • 73c5e99: 8286473: Drop --enable-preview from Record related tests
  • ... and 283 more: https://git.openjdk.java.net/jdk/compare/018017a9175cbfe02e9db0db402ca2aa689ac587...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace, @prsadhuk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 29, 2022
@@ -24,7 +24,6 @@
/*
* @test
* @bug 8015854
* @requires (os.family == "mac")
* @summary Tests HTML image as JButton text for unwanted padding on macOS Aqua LAF
* @run main HtmlButtonImageTest
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you have run this test on CI systems on all platforms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that unedited summary. Fixed.

paintViewR.y = insets.top;
paintViewR.width = c.getWidth() - (insets.left + insets.right);
paintViewR.height = c.getHeight() - (insets.top + insets.bottom);
final View v = (View)c.getClientProperty(BasicHTML.propertyKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this issue is only for JButton so is it needed to apply this for all components or we need to put a check for c instance of JButton

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't completely sure if it was necessary to add the check since it passed without, meaning I wouldn't need to import JButton into this class. However, it does make sense to check for JButtons, so I made the change. Tested the change and everything still passes.

@@ -110,6 +116,15 @@ private static void testImageCentering(int... colors) throws IOException {
throw new RuntimeException("HTML image not centered in button");
Copy link
Contributor

Choose a reason for hiding this comment

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

overall looks ok. Only thing is during failure, it does not show for which L&F the test is failing since you are iterating for all L&Fs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this can be changed. I can instead iterate through all L&F's and track all failing L&F's in a buffer to be displayed in the runtime exception in the end. How should the images be handled? Should I just generate X images where X is the number of failed L&F's?

@DamonGuy
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label May 11, 2022
@openjdk
Copy link

openjdk bot commented May 11, 2022

@DamonGuy
Your change (at version 82d9ca2) is now ready to be sponsored by a Committer.

@prrace
Copy link
Contributor

prrace commented May 11, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented May 11, 2022

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

  • 1586bf8: 8286401: Address possibly lossy conversions in Microbenchmarks
  • 1c50ea3: 8282607: runtime/ErrorHandling/MachCodeFramesInErrorFile.java failed with "RuntimeException: 0 < 2"
  • fcf49f4: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
  • 46a775a: 8286540: Build failure caused by missing DefaultPollerProvider implementation on AIX
  • 4ad8cfa: 8286002: Add support for intel syntax to capstone hsdis
  • cec23b1: 8286556: Remove EagerInitialization develop option
  • 89de756: 8286387: Remove unused FreeListAllocator::reduce_free_list
  • ae695d6: 8286476: x86_32: Fix crashes with non-preview mode after JDK-8284161 (Virtual Threads)
  • 87f3d2b: 8286446: PPC64: fix crashes after JDK-8284161 (virtual threads preview)
  • 7a2bbbb: 8286396: Address possibly lossy conversions in jdk.management.jfr
  • ... and 289 more: https://git.openjdk.java.net/jdk/compare/018017a9175cbfe02e9db0db402ca2aa689ac587...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 11, 2022
@openjdk openjdk bot closed this May 11, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 11, 2022
@openjdk
Copy link

openjdk bot commented May 11, 2022

@prrace @DamonGuy Pushed as commit ccbe8fa.

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

@DevCharly
Copy link

This PR breaks correct text location when using HTML text in buttons.

It works only if horizontal/vertical alignment is center and if using symmetric margin (top == bottom and left == right).
If using left/right alignment or using asymmetric margin (e.g. top=30, bottom=4) then the HTML text location is wrong.

And shouldn't text positioned the same for HTML and non-HTML buttons?

Screenshot that demonstrates the problem.
Left is made with Java 20, right with Java 17. Red arrows mark wrong text locations.
In Java 17, HTML text is painted at same location as non-HTML text.
Since Java 19, there is a huge difference because margin is simply ignored when painting.
(interestingly margin is still used to compute preferred size 😕 )

image

Code used to create above screenshots:

import java.awt.*;
import javax.swing.*;
import javax.swing.border.*;

public class HtmlButtonTest
{
    public static void main( String[] args ) {
        SwingUtilities.invokeLater( () -> {
            JFrame frame = new JFrame( "HTML Button Test" );
            frame.setDefaultCloseOperation( JFrame.EXIT_ON_CLOSE );

            JPanel panel = new JPanel( new GridBagLayout() );
            panel.setBorder( new EmptyBorder( 20, 20, 20, 20 ) );

            createButtons( panel, "center", SwingConstants.CENTER, SwingConstants.CENTER, null );
            createButtons( panel, "left", SwingConstants.LEFT, SwingConstants.CENTER, null );
            createButtons( panel, "right", SwingConstants.RIGHT, SwingConstants.CENTER, null );

            createButtons( panel, "center with margin 30,4,4,4", SwingConstants.CENTER, SwingConstants.CENTER, new Insets( 30, 4, 4, 4 ) );
            createButtons( panel, "left with margin 30,4,4,4", SwingConstants.LEFT, SwingConstants.CENTER, new Insets( 30, 4, 4, 4 ) );
            createButtons( panel, "left/top with margin 30,4,4,4", SwingConstants.LEFT, SwingConstants.TOP, new Insets( 30, 4, 4, 4 ) );

            frame.add( new JLabel( "Java version " + System.getProperty( "java.version" ) ), BorderLayout.NORTH );
            frame.add( panel );
            frame.pack();
            frame.setVisible( true );
        } );
    }

    private static void createButtons( JPanel panel, String text, int horizontalAlignment, int verticalAlignment, Insets margin ) {
        JButton button = new JButton( text );
        button.setHorizontalAlignment( horizontalAlignment );
        button.setVerticalAlignment( verticalAlignment );
        if( margin != null )
            button.setMargin( margin );
        panel.add( button, new GridBagConstraints( 0, GridBagConstraints.RELATIVE, 1, 1, 1.0, 0.0,
            GridBagConstraints.CENTER, GridBagConstraints.HORIZONTAL, new Insets( 4, 4, 4, 4 ), 0, 0 ) );

        JButton htmlButton = new JButton( "<html>HTML " + text + "</html>" );
        htmlButton.setHorizontalAlignment( horizontalAlignment );
        htmlButton.setVerticalAlignment( verticalAlignment );
        if( margin != null )
            htmlButton.setMargin( margin );
        panel.add( htmlButton, new GridBagConstraints( 0, GridBagConstraints.RELATIVE, 1, 1, 1.0, 0.0,
            GridBagConstraints.CENTER, GridBagConstraints.HORIZONTAL, new Insets( 4, 4, 24, 4 ), 0, 0 ) );
    }
}

@DevCharly
Copy link

BTW this PR also breaks 3rd party L&Fs like FlatLaf.
See JFormDesigner/FlatLaf#746

@DevCharly
Copy link

I really wonder why the (10 year old) issue JDK-8015854 was accepted as bug. 😕
It is not a bug in JDK, it is a bug in users code.

He sets the preferred size of the button to 30x30 and adds an HTML image to that button and expects that it is painted centered.
But he ignores (or does not know) that a button has a margin (2,14,2,14 in Metal).
Subtracting the margin from the preferred size results in a view size of 2x26 pixel.
This view is centered withing the component bounds and the HTML image/text is painted left-top aligned in that view.
This paints the HTML image/text out of center, but this is expected behavior.

image

If the user reduces the preferred size, he also needs to reduce the margin.
E.g. adding following line to users code (from JDK-8015854) fixes the problem:

testButton.setMargin(new Insets(0, 0, 0, 0));

image

I would recommend to revert this PR and also PR #7310.
Both break compatibility when using HTML text in buttons.

@DamonGuy
Copy link
Contributor Author

Hi @DevCharly, I'll look into this. I see your point on how this sort of goes against expected behavior, knowing the margins are (2, 14, 2, 14). I can distinguish between HTML images and HTML text to resolve this issue somewhat for the unexpected padding, but I think reverting the PRs is also a valid solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants