Skip to content

8054572: [macosx] JComboBox paints the border incorrectly #9473

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

Closed

Conversation

DamonGuy
Copy link
Contributor

@DamonGuy DamonGuy commented Jul 12, 2022

When a JComboBox is editable, the button segment of the combo box is misaligned vertically and has a different height. This change fixes these issues and adds a manual test that checks the appearance of an editable and non-editable JComboBox.

One of the discussions revolving this issue is the native macOS appearance of editable JComboBoxes. After looking through native macOS apps, the only one found is in System Preferences > Date & Time. The problem here is that the native equivalent found here uses a blue button with a single down arrow as the button's symbol. The current swing implementation uses a white button with an up & down arrow symbol for the button. A JRS widget button that has this blue button with a single downward arrow exists but does not support text fields.

As such, I believe the best fix for this issue is to mainly fix the alignment and sizing issue. I looked through Apple's documentation for these UI elements but editable JComboBoxes aren't specifically listed anywhere. Similarly, there's barely any editable JComboBoxes used in native mac apps (only the date & time). So, I don't think it's a major issue if JComboBox does not exactly match the example found in Date & Time.


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-8054572: [macosx] JComboBox paints the border incorrectly

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9473

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 12, 2022

👋 Welcome back dnguyen! 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 Jul 12, 2022
@openjdk
Copy link

openjdk bot commented Jul 12, 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 Jul 12, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 12, 2022

PassFailJFrame.addTestFrame(frame);
PassFailJFrame.positionTestFrame(frame,
PassFailJFrame.Position.HORIZONTAL);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

These methods need to be changed to addTestWindow and positionTestWindow based on the recently integrated changes to PassFailJFrame.

public static synchronized void addTestWindow(Window testWindow) {

Copy link
Contributor Author

@DamonGuy DamonGuy Jul 12, 2022

Choose a reason for hiding this comment

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

I remember this conversation. I synced my local branch and updated the methods. Tested and the test still works.

// } catch (Exception e) {
//
// }

Copy link
Contributor

Choose a reason for hiding this comment

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

Might have missed removing commented lines

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. I also added a few newlines for long lines of code. I think I forgot to save my latest changes.

@prsadhuk
Copy link
Contributor

Can you please show before and after fix image of the editable combobox?
It seems the border around the button makes it feel it's still not aligned properly which gives the impression that the button is bigger than textfield. Probably we can do away with the button border, if we can, in case we cannot draw the border around the whole textfield+button.

Also, I think if you try with different font size e.g.
comboBox.setFont(new Font("Serif", Font.PLAIN, 30));

you will again see the editable combobox button height is not matching textfield height whereas non editable one it is aligned..

@DamonGuy
Copy link
Contributor Author

Can you please show before and after fix image of the editable combobox? It seems the border around the button makes it feel it's still not aligned properly which gives the impression that the button is bigger than textfield. Probably we can do away with the button border, if we can, in case we cannot draw the border around the whole textfield+button.

Also, I think if you try with different font size e.g. comboBox.setFont(new Font("Serif", Font.PLAIN, 30));

you will again see the editable combobox button height is not matching textfield height whereas non editable one it is aligned..

Local test without the fix:
Local Test Without Fix

Local test with the fix:
Local Test With Alignment Fix

I have additional images on JBS, but I'll gladly post them here as well.

I'm looking into the text size and the borders. The sizing of the button seems to also be affected by a component size value, so I'm adjusting that instead.

@prsadhuk
Copy link
Contributor

Can you please show before and after fix image of the editable combobox? It seems the border around the button makes it feel it's still not aligned properly which gives the impression that the button is bigger than textfield. Probably we can do away with the button border, if we can, in case we cannot draw the border around the whole textfield+button.
Also, I think if you try with different font size e.g. comboBox.setFont(new Font("Serif", Font.PLAIN, 30));
you will again see the editable combobox button height is not matching textfield height whereas non editable one it is aligned..

Local test without the fix: Local Test Without Fix

Local test with the fix: Local Test With Alignment Fix

Screenshot 2022-07-14 at 10 41 39 AM

I am not getting the same uniform blue border around textfield and button as can be seen in my screenshot with your test, which is seen in your image (with fix), so I asked to remove that incomplete blue border.. I am testing on BigSur 11.6..which os version is yours?

@DamonGuy
Copy link
Contributor Author

Can you please show before and after fix image of the editable combobox? It seems the border around the button makes it feel it's still not aligned properly which gives the impression that the button is bigger than textfield. Probably we can do away with the button border, if we can, in case we cannot draw the border around the whole textfield+button.
Also, I think if you try with different font size e.g. comboBox.setFont(new Font("Serif", Font.PLAIN, 30));
you will again see the editable combobox button height is not matching textfield height whereas non editable one it is aligned..

Local test without the fix: Local Test Without Fix
Local test with the fix: Local Test With Alignment Fix

Screenshot 2022-07-14 at 10 41 39 AM

I am not getting the same uniform blue border around textfield and button as can be seen in my screenshot with your test, which is seen in your image (with fix), so I asked to remove that incomplete blue border.. I am testing on BigSur 11.6..which os version is yours?

Discussed this with Prasanta over a meeting. I'm on Monterey 12.3.1. The border around the text field can be removed, but I can't seem to remove the button's border. I tried these but none removed the border (even though appearance changes for a standard JButton in testing):

  • arrowButton.setBorder(javax.swing.BorderFactory.createEmptyBorder());
  • arrowButton.setBorderPainted(false);
  • arrowButton.setFocusPainted(false);

The appearance of this button in JComboBox seems to be set by the JRS, thus making a change to the button's border not possible. This concern will be compiled in a list alongside other questions regarding appearance of a JComboBox.

@prsadhuk
Copy link
Contributor

. I tried these but none removed the border (even though appearance changes for a standard JButton in testing):

  • arrowButton.setBorder(javax.swing.BorderFactory.createEmptyBorder());
  • arrowButton.setBorderPainted(false);
  • arrowButton.setFocusPainted(false);

The appearance of this button in JComboBox seems to be set by the JRS, thus making a change to the button's border not possible. This concern will be compiled in a list alongside other questions regarding appearance of a JComboBox.

Since the arrowButton is rendered by JRS, it needs to be instucted not to draw the border in that case..The above methods will not work as JRSUIPainter does not know about that, so one can call
painter.state.set(FOCUSED.NO)
or something like that to remove the focused border.
Having said that, I dont think it will be correct to remove the border from the button as it is meant to show focus is on that component.
Ideally, the textfield also should have a focus border around it similar to button, so it will look uniform, so instead of removing border from button, we can try to draw focus border around textfield (maybe something similar to JDK-7124282)

@DamonGuy DamonGuy marked this pull request as draft July 22, 2022 07:25
@openjdk openjdk bot removed the rfr Pull request is ready for review label Jul 22, 2022
@DamonGuy
Copy link
Contributor Author

DamonGuy commented Aug 8, 2022

Can you please show before and after fix image of the editable combobox? It seems the border around the button makes it feel it's still not aligned properly which gives the impression that the button is bigger than textfield. Probably we can do away with the button border, if we can, in case we cannot draw the border around the whole textfield+button.

Also, I think if you try with different font size e.g. comboBox.setFont(new Font("Serif", Font.PLAIN, 30));

you will again see the editable combobox button height is not matching textfield height whereas non editable one it is aligned..

I have looked into and tested for the setFont issue for sizing editable ComboBox's in Aqua L&F. A non-editable ComboBox in Aqua does NOT increase the height of the text area of the ComboBox with larger fonts.
Here is an image where I increased the button by 50px in height and width:
Screen Shot 2022-08-08 at 2 04 32 AM

This seems to be Aqua specific because when tested in other L&F's like Metal, the text areas and buttons increase in height to accommodate larger text.
Here's an image from Metal L&F:
Screen Shot 2022-08-08 at 3 20 59 AM

Additionally, the button of the ComboBox in Aqua never changes size as it seems the button appearance is pre-determined by JRS values. Increasing the size of the arrowButton in Aqua takes up the space that the enlarged button should take, but the button will still appear small.
This screenshot shows the Aqua button triggering the dropdown list after clicking the blank space where the button should be stretched to:
Screen Shot 2022-08-08 at 2 04 48 AM

@DamonGuy
Copy link
Contributor Author

The solution I worked towards was to match the behavior of the non-editable combo box in Aqua L&F. This is because, as previously mentioned, the button's visual size can't change. If the arrow button's size is increased, space will be dedicated for the button in its container and the button will function if clicked within that space but the button would still appear to be the same size (which would be too small for larger text fields).

So, the text field will not resize in height according to the font and the font would remain the larger font size of 30 in this example. This requires me to override the rectangle calculation method and update the logic for an editable text field of varying font sizes to correctly center the text field to be aligned with the arrow button.

Screen Shot 2022-08-11 at 6 55 47 AM

@DamonGuy DamonGuy marked this pull request as ready for review August 11, 2022 15:36
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 11, 2022
midHeight = 0;
}

if(comboBox.getComponentOrientation().isLeftToRight()) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a space after if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -454,6 +454,32 @@ protected LayoutManager createLayoutManager() {
}

class AquaComboBoxLayoutManager extends BasicComboBoxUI.ComboBoxLayoutManager {
protected Rectangle rectangleForCurrentValue() {
System.out.println("rectForCurrentValue NEW: " + comboBox.getHeight());
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed removing print statement :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks for catching that. I had a lot of comments to clean up after testing and this one slipped through.

protected Rectangle rectangleForCurrentValue() {
System.out.println("rectForCurrentValue NEW: " + comboBox.getHeight());
int width = comboBox.getWidth();
int height = 21;
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 a Aqua LAF constraint on max height of text-field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This height limitation is set to match the appearance of a non-oversized font in an editable JComboBox. I tested different values, and this value matches the appearance of an editable JComboBox when the AquaLookAndFeel control font is used.

The reason for overriding this method is to set this max height and condense the logic for the editor rectangle values. Previously, this class hard coded adjustments for the width and height values of the editor rectangle in layoutContainer when editor != null. This occurs right after this rectangleForCurrentValue is called, and this method is only used to calculate the rectangle position. So, overriding this method makes more sense since it does exactly that for Aqua L&F values specifically.

@prsadhuk
Copy link
Contributor

I am not sure if this is correct approach...
Have you seen any macos documentation citing combobox height should not increase even if font size change?
That might be problematic for say, accessibility usecase, for slight visually impaired more hearing impaired, where we need to draw them big, right?

So, assuming it's a issue, I think
Since textField height changes with font size, why can't we use the same rectangleForCurrentValue() with which we draw the textField height, for arrowButton height
as seen here (https://github.com/openjdk/jdk/blob/master/src/java.desktop/macosx/classes/com/apple/laf/AquaComboBoxUI.java#L470)

I see that we call AquaCombobBoxButton with height.
Are you saying this height, even you change according to font size, is ignored?

Then it seems this painter.paint() which calls AquaPainter.paintFromSingleCachedImage() which calls MultiResolutionCachedImage to create a MRI maybe is not creating proper MRI image as per the given arrowbutton size.
Can you find out why?

Maybe non-editable combobox can have "single cached image" to have single-sized arrowbutton, but it may not be correct for editable combobox where font size can be changed
OR maybe non-editable combobox not changing height for diff. fontsize, also is a result of the above MRI creation issue.

@DamonGuy
Copy link
Contributor Author

DamonGuy commented Aug 15, 2022

I am not sure if this is correct approach... Have you seen any macos documentation citing combobox height should not increase even if font size change? That might be problematic for say, accessibility usecase, for slight visually impaired more hearing impaired, where we need to draw them big, right?

So, assuming it's a issue, I think Since textField height changes with font size, why can't we use the same rectangleForCurrentValue() with which we draw the textField height, for arrowButton height as seen here (https://github.com/openjdk/jdk/blob/master/src/java.desktop/macosx/classes/com/apple/laf/AquaComboBoxUI.java#L470)

I see that we call AquaCombobBoxButton with height. Are you saying this height, even you change according to font size, is ignored?

Then it seems this painter.paint() which calls AquaPainter.paintFromSingleCachedImage() which calls MultiResolutionCachedImage to create a MRI maybe is not creating proper MRI image as per the given arrowbutton size. Can you find out why?

Maybe non-editable combobox can have "single cached image" to have single-sized arrowbutton, but it may not be correct for editable combobox where font size can be changed OR maybe non-editable combobox not changing height for diff. fontsize, also is a result of the above MRI creation issue.

The best documentation I have is the webpage for pull-down buttons here:
https://developer.apple.com/design/human-interface-guidelines/components/menus-and-actions/pull-down-buttons

There's no text describing the behavior, but the image shows a bracket and lines with arrows on each end to show variable width of a pull-down button. However, for height, the bracket has no arrows, which indicate that the height is fixed.

I'm currently looking at the MRI image issued, but that image combined with the behavior of non-editable comboboxes (even with larger fonts) led me to believe this approach was acceptable.

@honkar-jdk
Copy link
Contributor

honkar-jdk commented Aug 17, 2022

Tested on Mac 12.3. The editable combobox looks quite close to the non editable one.

Ideally, the textfield also should have a focus border around it similar to button, so it will look uniform, so instead of removing border from button, we can try to draw focus border around textfield.

Just a thought: As @prsadhuk suggested earlier, carrying forward the focus ring style & thickness of the button over to the textfield might help in giving the editable combobox (textfield + button) a unified look when it has focus.

image

image

Minor changes:
Copyright year for AquaComboBoxUI needs to be updated
Adding MacOS test - either in the instructionText or summary of the test.

@DamonGuy
Copy link
Contributor Author

Tested on Mac 12.3. The editable combobox looks quite close to the non editable one.

Ideally, the textfield also should have a focus border around it similar to button, so it will look uniform, so instead of removing border from button, we can try to draw focus border around textfield.

Just a thought: As @prsadhuk suggested earlier, carrying forward the focus ring style & thickness of the button over to the textfield might help in giving the editable combobox (textfield + button) a unified look when it has focus.

image

image

Minor changes: Copyright year for AquaComboBoxUI needs to be updated Adding MacOS test - either in the instructionText or summary of the test.

Updated the copyright and text. For the border, I brought this topic up in previous talks and it was noted that the current appearance is sufficient, at least for now. The editable combobox uses the default textfield's focusborder and an additional issue can be created for updating the focus border to match the button's.

@prsadhuk
Copy link
Contributor

@andy-goryachev-oracle I took into consideration your comment regarding RTL and LTR behavior regarding JComboBoxes. I did the testing as we briefly discussed and all results led me to believe Aqua L&F combo boxes only supported the button being on the right. Will still need to check with a device natively set to RTL, but it turns out a nearly decade old JBS issue already exists for this JComboBox button issue. I'll leave that separate from this PR and possibly look into that other issue later.

Yes, JDK-8023912...
I am not sure if this RTL is an issue. I guess it was mentioned in our meeting that RTL should not affect JComboBox button.
Maybe another question for apple, since the button image drawn by JRS?

@DamonGuy
Copy link
Contributor Author

@andy-goryachev-oracle I took into consideration your comment regarding RTL and LTR behavior regarding JComboBoxes. I did the testing as we briefly discussed and all results led me to believe Aqua L&F combo boxes only supported the button being on the right. Will still need to check with a device natively set to RTL, but it turns out a nearly decade old JBS issue already exists for this JComboBox button issue. I'll leave that separate from this PR and possibly look into that other issue later.

Yes, JDK-8023912... I am not sure if this RTL is an issue. I guess it was mentioned in our meeting that RTL should not affect JComboBox button. Maybe another question for apple, since the button image drawn by JRS?

I believe the question regarding font sizes was already sent to Apple last week. I can definitely add it to my ever-growing list of questions to Apple, but I was advised to keep this question to Apple focused on this particular issue.

@DamonGuy
Copy link
Contributor Author

I am not sure if this is correct approach... Have you seen any macos documentation citing combobox height should not increase even if font size change? That might be problematic for say, accessibility usecase, for slight visually impaired more hearing impaired, where we need to draw them big, right?

Through Phil, Apple has confirmed that the height of the JComboBox should be fixed. The reason being that the UI was designed around a control font size, and other UI such as button backgrounds and other related elements also have fixed height. The UI's height being resizable is possible, but is not intended for Aqua.

So I believe this fix aligns with that philosophy and is an OK way of resolving the discrepancy between editable & non-editable JComboBoxes in Aqua L&F.

}

if (comboBox.getComponentOrientation().isLeftToRight()) {
return new Rectangle(insets.left, insets.top + midHeight,
Copy link
Contributor

@prsadhuk prsadhuk Aug 26, 2022

Choose a reason for hiding this comment

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

OK. Since Apple has confirmed combobox height shouldn't change with font size, we can have fixed height.

Only thing I guess, till we sort out JDK-8023912: [macosx] JComboBox RTL orientation problem
lets use LTR as it seems it might give a false impression that we support RTL when we dont even know if Apple supports it or not. Do you see any difference with combobox if you use LTR code vs RTL code with RTL testcase present in the above JBS? I dont in my BigSur.

It will also be good to test it out in non-retina display to see if this "22" height is being shown ok there.

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 don't see a difference either but was following the same logic as used in BasicComboBoxUI. I can remove the LTR vs RTL logic until the other issue is resolved if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can keep this for now...You can take in JDK-8023912 incase which I guess needs a followup question to Apple.

@honkar-jdk
Copy link
Contributor

Tested on non-retina display, combobox height of 22 works well.
Increasing or decreasing the height by 1 produces slight offset. I think height=22 works well here.

With Height = 23
image

With Height = 21
image


frame = new JFrame();
frame.getContentPane().add(panel);
frame.setVisible(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

@DamonGuy To avoid flickering of the test frame move frame.setVisible(true) after PassFailJFrame.positionTestWindow()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks, will note for the future.

@openjdk
Copy link

openjdk bot commented Sep 9, 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:

8054572: [macosx] JComboBox paints the border incorrectly

Reviewed-by: honkar, 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 694 new commits pushed to the master branch:

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 (@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 Sep 9, 2022
@DamonGuy
Copy link
Contributor Author

DamonGuy commented Sep 9, 2022

/integrate

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

openjdk bot commented Sep 9, 2022

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

@DamonGuy
Copy link
Contributor Author

@prsadhuk Could you sponsor this PR?

@prsadhuk
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 19, 2022

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

  • b920d29: 8271328: User is able to choose the color after disabling the color chooser.
  • 5725a93: 8293879: Remove unnecessary castings in jdk.hotspot.agent
  • ab7f58a: 6286501: JTabbedPane throws NPE from its stateChanged listener in particular case
  • d41f69f: 8293849: PrintIdealPhase in compiler directives file is ignored when used with other compile commands
  • 471e2f1: 8292088: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue
  • a93cf92: 8293920: G1: Add index based heap region iteration
  • 36c9034: 8293808: mscapi destroyKeyContainer enhance KeyStoreException: Access is denied exception
  • cbd0688: 8293851: hs_err should print more stack in hex dump
  • 04d7b7d: 8293503: gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64 failed assertGreaterThanOrEqual: expected MMM >= NNN
  • d77c464: 8293891: gc/g1/mixedgc/TestOldGenCollectionUsage.java (still) assumes that GCs take 1ms minimum
  • ... and 770 more: https://git.openjdk.org/jdk/compare/31f7fc043b4616cb2d5f161cda357d0ebfb795f0...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 19, 2022
@openjdk openjdk bot closed this Sep 19, 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 Sep 19, 2022
@openjdk
Copy link

openjdk bot commented Sep 19, 2022

@prsadhuk @DamonGuy Pushed as commit 8082c24.

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

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
Development

Successfully merging this pull request may close these issues.

4 participants