8054572: [macosx] JComboBox paints the border incorrectly

[DamonGuy]

trafficstars

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)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8054572: [macosx] JComboBox paints the border incorrectly

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[bot]]

:wave: 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[bot]]

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

[mlbridge[bot]]

Webrevs

• 07: Full - Incremental (6fec827e)
• 06: Full - Incremental (0eae1016)
• 05: Full - Incremental (fe0d2f81)
• 04: Full - Incremental (78db9fbe)
• 03: Full - Incremental (4291d21e)
• 02: Full - Incremental (ffc44a89)
• 01: Full - Incremental (1d888259)
• 00: Full (dab81933)

[prsadhuk]

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]

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 with the 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]

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 with the fix:

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]

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 with the fix:

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]

. 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]

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:

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:

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:

[DamonGuy]

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.

[DamonGuy]

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

[prsadhuk]

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]

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]

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.

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

[DamonGuy]

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.

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]

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

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

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.

[honkar-jdk]

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

With Height = 21

[openjdk[bot]]

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

• 005b49bb78a468d4e372e6f5fa48bb0db4fd73c2: 8293044: C1: Missing access check on non-accessible class
• 91d00b3022b8bb59ce04fb5f214e3deb93590f46: 8288473: Remove unused frame::set_pc_preserve_deopt methods
• 45ff10cc68296c7c73d0eafe6fcc9946ab98267e: 8292695: SIGQUIT and jcmd attaching mechanism does not work with signal chaining library
• dbec22b84b0ffce447b43271e12ed7d0eed6c387: 8293287: add ReplayReduce flag
• b8598b02979dff8a947a523a6d76768a1bfe594b: 8291660: Grapheme support in BreakIterator
• a14c3a493a98792a61de253920bb177a5c35fd8e: 8288933: Improve the implementation of Double/Float.isInfinite
• 00befddd7ce97d324250807824469daaa9434eef: 8292675: Add identity transformation for removing redundant AndV/OrV nodes
• 7169ee5c73c130aacce73cbd3f88377ec07c8311: 8293477: IGV: Upgrade to Netbeans Platform 15
• 3dd94f33b2dddf8ea28805499d110c2347476c19: 8292671: Hotspot Style Guide should allow covariant returns
• 9d6b0285f53599816c30393b87d16772ef6314b7: 8234315: GTK LAF does not gray out disabled JMenu
• ... and 684 more: https://git.openjdk.org/jdk/compare/31f7fc043b4616cb2d5f161cda357d0ebfb795f0...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 (@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).

[DamonGuy]

/integrate

[openjdk[bot]]

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

[DamonGuy]

@prsadhuk Could you sponsor this PR?

[prsadhuk]

/sponsor

[openjdk[bot]]

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

• b920d2999fed5ec5afe666559e14f8e1a0e90852: 8271328: User is able to choose the color after disabling the color chooser.
• 5725a93c078dac9775ccef04f3624647a8d38e83: 8293879: Remove unnecessary castings in jdk.hotspot.agent
• ab7f58a3771f5f8e7240f53d595bdf91a17874d2: 6286501: JTabbedPane throws NPE from its stateChanged listener in particular case
• d41f69f9c0297fe78884b5aa2d149745215ec9d2: 8293849: PrintIdealPhase in compiler directives file is ignored when used with other compile commands
• 471e2f12b44cafc583a8ae118e36df5f00dfd624: 8292088: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue
• a93cf926356b974b8fc5a97d230a15bad066ac2a: 8293920: G1: Add index based heap region iteration
• 36c9034ff1274f37969550a3f9239f1bb16a0b25: 8293808: mscapi destroyKeyContainer enhance KeyStoreException: Access is denied exception
• cbd0688b321ff88a405be4f7929d6862e543ab50: 8293851: hs_err should print more stack in hex dump
• 04d7b7d5747d887e12797df8ca3f7608d73d41ff: 8293503: gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64 failed assertGreaterThanOrEqual: expected MMM >= NNN
• d77c464c3804362b80fecca9df05fbef90bed14a: 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[bot]]

@prsadhuk @DamonGuy Pushed as commit 8082c24a0df3f4861ea391266bdfe6cdd1a77bab.

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