8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure #10626

DamonGuy · 2022-10-10T05:26:23Z

The previous change to AquaComboBoxUI had 1 pixel of overlap between the text field and the combo button. This caused a few pixels to darken sometimes when an editable combobox is displayed. Since this test passes sometimes and fails some other times, it was not initially detected.

Since these few darkening pixels occur due to the change in AquaComboBoxUI, no reasonable change to the test could really be made, and the change would need to be to the class for the combobox. This fix was tested with a high count repeat and no failures occurred for various macOS systems.

Also removing the test from the problem list.

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-8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure

Reviewers

Jayathirth D V (@jayathirthrao - Reviewer) ⚠️ Review applies to 6ee1f92f
Alisen Chung (@alisenchung - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10626

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

Using diff file

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

bridgekeeper · 2022-10-10T05:27:32Z

👋 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 · 2022-10-10T05:31:16Z

@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 · 2022-10-10T05:35:27Z

Webrevs

honkar-jdk · 2022-10-10T19:05:50Z

Suggestion: Does running the test - CustomComboBoxFocusTest.java with uiScale always set to 1 make it more robust?

jayathirthrao

Its good that JComboBox misalignment issue is resolved under : https://bugs.openjdk.org/browse/JDK-8054572(I used to observe this at all places in SwingSet2 while testing Lanai)

I ran javax/swing/plaf/aqua/CustomComboBoxFocusTest.java without this change in built-in display and external display:
Built-in display(uiScale 2) : Fails 1/5 times
External display(uiScale 1) : Fails 3/5 times

Initially i was also under the impression that display scaling in test might be the root cause and making it run at some default scaling will solve the issue. But looks like this is a product regression introduced with fix under https://bugs.openjdk.org/browse/JDK-8054572.

Since the current fix takes care of both https://bugs.openjdk.org/browse/JDK-8054572 & https://bugs.openjdk.org/browse/JDK-8073001. This product change looks good to me.

openjdk · 2022-10-11T06:29:56Z

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

8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure

Reviewed-by: jdv, achung

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

8487c56: 8295294: Misc cleanups in runtime/InvocationTests
0043d58: 8293531: C2: some vectorapi tests fail assert "Not monotonic" with flag -XX:TypeProfileLevel=222
2e2a51e: 8295267: Improve G1 scan to merge ratio calculation
dfcd9d5: 8295320: [BACKOUT] 8276687 Remove support for JDK 1.4.1 PerfData shared memory files
2da079c: 8294426: Two fingers tap generates wrong mouse modifiers on M2 MacBooks
449b52f: 8295158: G1: Increase card-based cost sample reporting threshold
312985e: 8295156: G1: Improve constant other time calculation
786ce1c: 8295024: Cyclic constructor error is non-deterministic and inconsistent
1efa93e: 8294844: Improve G1 young gen length revise trigger
3dbc38a: 8295288: Some vm_flags tests associate with a wrong BugID
... and 71 more: https://git.openjdk.org/jdk/compare/1bfcc2790adbc273864c74faab0bd43613c75982...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 (@jayathirthrao) 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).

jayathirthrao · 2022-10-11T06:30:52Z

Additionally if we want to make javax/swing/plaf/aqua/CustomComboBoxFocusTest.java run at some default uiScale that also should be fine.

prsadhuk · 2022-10-11T07:08:57Z

src/java.desktop/macosx/classes/com/apple/laf/AquaComboBoxUI.java

                        height - (insets.top + insets.bottom));
            }
            else {
                return new Rectangle(insets.left + buttonSize, insets.top + midHeight,
-                        width - (insets.left + insets.right + buttonSize) + 4,
+                        width - (insets.left + insets.right + buttonSize) + 3,


I guess previous to this fix, we were using BasicComboBoxUI#rectangleForCurrentValue() which returns a width on which we add +4 for editor/textField width.
Since the calculation is basically same for width, why was it giving problem now (where only height is modified in the 8054572 fix) and not before this fix. I dont think this test use to fail before this fix was integrated with promoted CI builds

Previously, editable comboboxes on macOS were misaligned altogether. The button and textfield were not at the same height (as seen on the "before" screenshots in https://bugs.openjdk.org/browse/JDK-8054572). My assumption is that these pixels that would normally overlap are no longer overlapping because of this.

DamonGuy · 2022-10-11T18:35:54Z

Suggestion: Does running the test - CustomComboBoxFocusTest.java with uiScale always set to 1 make it more robust?

Sure, I can add a uiscale of 1 for robustness. Seems like a good idea since Jay's results at different uiscales may show variable pass rates at different uiscales.

…Guy/jdk into 8294254/overlapFixEditableCombobox

openjdk · 2022-10-11T19:14:09Z

⚠️ @DamonGuy This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

alisenchung

Looks fine

DamonGuy · 2022-10-14T20:49:09Z

/integrate

openjdk · 2022-10-14T20:51:58Z

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

DamonGuy · 2022-10-17T17:42:08Z

@jayathirthrao Could you please sponsor this PR?

jayathirthrao · 2022-10-18T03:54:10Z

/sponsor

openjdk · 2022-10-18T03:55:33Z

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

490fcd0: 8293833: Error mixing types with -XX:+UseCMoveUnconditionally -XX:+UseVectorCmov
529cc48: 8295396: RISC-V: Cleanup useless CompressibleRegions
692cdab: 8295016: Make the arraycopy_epilogue signature consistent with its usage
21a825e: 8288387: GetLocalXXX/SetLocalXXX spec should require suspending target thread
8d751de: 8295231: Move all linking of native libraries to make
f300ec8: 8294546: document where javac differs when invoked via launcher and ToolProvider
b269c51: 8295395: Linux Alpha Zero builds fail after JDK-8292591
ae60599: 8295023: Interpreter(AArch64): Implement -XX:+PrintBytecodeHistogram and -XX:+PrintBytecodePairHistogram options
4d37ef2: 8295262: Build binutils out of source tree
0919a3a: 8294186: AArch64: VectorMaskToLong failed on SVE2 machine with -XX:UseSVE=1
... and 94 more: https://git.openjdk.org/jdk/compare/1bfcc2790adbc273864c74faab0bd43613c75982...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-18T03:55:54Z

@jayathirthrao @DamonGuy Pushed as commit 358ac07.

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

DamonGuy added 3 commits October 5, 2022 22:31

Adjust rect width to remove overlap

f39a259

Remove test from problem list

893f43d

Merge branch 'openjdk:master' into 8294254/overlapFixEditableCombobox

e614f08

openjdk bot added the rfr Pull request is ready for review label Oct 10, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Oct 10, 2022

Merge branch 'master' into 8294254/overlapFixEditableCombobox

6ee1f92

jayathirthrao approved these changes Oct 11, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 11, 2022

prsadhuk reviewed Oct 11, 2022

View reviewed changes

DamonGuy added 2 commits October 11, 2022 11:41

Set uiscale

5ba103f

Merge branch '8294254/overlapFixEditableCombobox' of github.com:Damon…

8c0e0d8

…Guy/jdk into 8294254/overlapFixEditableCombobox

alisenchung approved these changes Oct 14, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 14, 2022

openjdk bot added the integrated Pull request has been integrated label Oct 18, 2022

openjdk bot closed this Oct 18, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 18, 2022

openjdk bot removed the sponsor Pull request is ready to be sponsored label Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure #10626

8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure #10626

DamonGuy commented Oct 10, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 10, 2022

openjdk bot commented Oct 10, 2022

mlbridge bot commented Oct 10, 2022 •

edited

Loading

honkar-jdk commented Oct 10, 2022

jayathirthrao left a comment •

edited

Loading

openjdk bot commented Oct 11, 2022 •

edited

Loading

jayathirthrao commented Oct 11, 2022

prsadhuk Oct 11, 2022

DamonGuy Oct 11, 2022

DamonGuy commented Oct 11, 2022

openjdk bot commented Oct 11, 2022

alisenchung left a comment

DamonGuy commented Oct 14, 2022

openjdk bot commented Oct 14, 2022

DamonGuy commented Oct 17, 2022

jayathirthrao commented Oct 18, 2022

openjdk bot commented Oct 18, 2022

openjdk bot commented Oct 18, 2022

8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure #10626

8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure #10626

Conversation

DamonGuy commented Oct 10, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 10, 2022

openjdk bot commented Oct 10, 2022

mlbridge bot commented Oct 10, 2022 • edited Loading

Webrevs

honkar-jdk commented Oct 10, 2022

jayathirthrao left a comment • edited Loading

Choose a reason for hiding this comment

openjdk bot commented Oct 11, 2022 • edited Loading

jayathirthrao commented Oct 11, 2022

prsadhuk Oct 11, 2022

Choose a reason for hiding this comment

DamonGuy Oct 11, 2022

Choose a reason for hiding this comment

DamonGuy commented Oct 11, 2022

openjdk bot commented Oct 11, 2022

alisenchung left a comment

Choose a reason for hiding this comment

DamonGuy commented Oct 14, 2022

openjdk bot commented Oct 14, 2022

DamonGuy commented Oct 17, 2022

jayathirthrao commented Oct 18, 2022

openjdk bot commented Oct 18, 2022

openjdk bot commented Oct 18, 2022

DamonGuy commented Oct 10, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 10, 2022 •

edited

Loading

jayathirthrao left a comment •

edited

Loading

openjdk bot commented Oct 11, 2022 •

edited

Loading