On Fri, 13 Feb 2026 08:15:54 GMT, Khalid Boulanouare <[email protected]> wrote:
>> This PR will consolidate fixes of the following bugs: >> >> https://bugs.openjdk.org/browse/JDK-8361188 >> https://bugs.openjdk.org/browse/JDK-8361189 >> https://bugs.openjdk.org/browse/JDK-8361190 >> https://bugs.openjdk.org/browse/JDK-8361191 >> https://bugs.openjdk.org/browse/JDK-8361192 >> https://bugs.openjdk.org/browse/JDK-8361193 >> https://bugs.openjdk.org/browse/JDK-8361195 >> >> This PR depends on https://github.com/openjdk/jdk/pull/25971 >> >> For test : java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java, the fix >> suggested is to return false in method isValidForPixelCheck for embedded >> frame, in which case the component is set to null. For more details see bug: >> [JDK-8361188](https://bugs.openjdk.org/browse/JDK-8361188) >> >> For test : test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java, I >> had to create a a tolerance color matching method for mac for the tests to >> pass. Also, the jbuttons needed to have different color than the color of >> the background frame, in order for test to pass. For more detail see bug: >> https://bugs.openjdk.org/browse/JDK-8361193 >> >> For test : test/jdk/java/awt/Mixing/AWT_Mixing/JSplitPaneOverlapping.java, >> it seems that color selected for lightweight component matches the >> background color of the frame. And this will cause the test to fail when >> matching colors. Choosing any color different than the background color will >> get the test to pass. For more details, see bug: >> https://bugs.openjdk.org/browse/JDK-8361192 >> >> For test test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java, it >> looks like the frame when visible, the popup test does not work properly. >> The frame needs to be hidden for the test to click on popup. For more >> details see bug: https://bugs.openjdk.org/browse/JDK-8361191 >> >> For test test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java, the >> test runs successfully but it times out after the default 2 minutes of >> jtreg. increasing the timeout to 3 minutes get the test to pass. For more >> details please refer to bug: https://bugs.openjdk.org/browse/JDK-8361190 > > Khalid Boulanouare has updated the pull request incrementally with one > additional commit since the last revision: > > Re problem list unstable test In addition to my other comments, bump up the copyright year in the updated `.java` files to 2026. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 133: > 131: testedComponent.setBounds(0, 0, > 132: testedComponent.getPreferredSize().width, > 133: > testedComponent.getPreferredSize().height + 20); This added code fully duplicates the existing code at lines [167–184](https://github.com/kboulanou/jdk/blob/a2b9a388fbcf836de159705500bdb22cedb2d1d9/test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java#L167-L184) except for focus handling. Modify the code below. Otherwise, lines 167–184 are unreachable. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 135: > 133: > testedComponent.getPreferredSize().height + 20); > 134: Component focusOwner = > KeyboardFocusManager.getCurrentKeyboardFocusManager() > 135: .getFocusOwner(); Suggestion: Component focusOwner = KeyboardFocusManager .getCurrentKeyboardFocusManager() .getFocusOwner(); What do you think about this way? The line isn't as long. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 148: > 146: } > 147: Point lLoc = testedComponent.getLocationOnScreen(); > 148: lLoc.translate(1, testedComponent.getPreferredSize().height > + 1); `getLocationOnScreen` and `getPreferredSize` are better called on EDT, however, I think it's safe here. You've got a synchronisation point with `invokeAndWait`, neither `getLocationOnScreen` and `getPreferredSize` could change, therefore the returned values would be consistent. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 165: > 163: > 164: return true; > 165: } This looks weird. I'd separate the logical blocks above with blank lines, but here the blank is absolutely redundant. Moreover, it seems to me `if (!resize)` should've remained above. That is if `!resize` we don't need the latch, we don't need anything because the test is skipped. So keep it that way: exit from the method early than later. Everything the follows `if (!resize)` block is where the action is happening. In other words, what I suggest is protected boolean performTest() { if (!super.performTest()) { return false; } if (!testResize) { return true; } // The above piece of code remains unchanged // Create the latch and handle focus here // ... test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 29: > 27: import java.awt.event.ActionEvent; > 28: import java.awt.event.ActionListener; > 29: import java.lang.Override; No need to explicitly import anything from the `java.lang` package. test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 70: > 68: frame.getContentPane().setLayout(new > BoxLayout(frame.getContentPane(), BoxLayout.Y_AXIS)); > 69: frame.setSize(200, 200); > 70: cb = new JComboBox(petStrings); I prefer to keep this blank line, it separates creating the frame and setting its size from creating and configuring the combo box. test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 104: > 102: > 103: loc2.translate(75, 75); > 104: robot.mouseMove(0, 0); I believe this line is asking for comment: Suggestion: robot.mouseMove(0, 0); // Avoid capturing mouse cursor test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 32: > 30: import java.awt.event.MouseAdapter; > 31: import java.awt.event.MouseEvent; > 32: import java.lang.Override; Suggestion: Redundant import. test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 79: > 77: frame.setLocationRelativeTo(null); > 78: frame.setVisible(true); > 79: menuBar = new JMenuBar(); Suggestion: frame.setLocationRelativeTo(null); frame.setVisible(true); menuBar = new JMenuBar(); Preserve the blank line. test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 110: > 108: propagateAWTControls(frame); > 109: > 110: } Suggestion: } A blank line right before the closing brace is redundant. test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 126: > 124: Robot robot = Util.createRobot(); > 125: robot.setAutoDelay(ROBOT_DELAY); > 126: robot.mouseMove(0, 0); A comment would be nice. test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 92: > 90: frame.setVisible(true); > 91: loc = frame.getContentPane().getLocationOnScreen(); > 92: } You removed `setLocationRelativeTo` and `setVisible` in `JMenuBarOverlapping.java`, should these be removed too? `setLocationRelativeTo` is redundant here, as you already called it on line 73. test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 99: > 97: Robot robot = Util.createRobot(); > 98: robot.setAutoDelay(ROBOT_DELAY); > 99: robot.mouseMove(0, 0); I'd add a comment to explain moving to (0, 0). test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 104: > 102: > 103: pixelPreCheck(robot, loc, currentAwtControl); > 104: try { If removing a blank line, I'd rather remove the one between `translate` and `pixelPreCheck` instead of this. Those calls are closer related than the `try` block below. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 61: > 59: public class MixingPanelsResizing { > 60: > 61: static final int TOLERANCE_MACOSX = 15; Suggestion: private static final int TOLERANCE_MACOSX = 15; For consistency with other declarations. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 62: > 60: > 61: static final int TOLERANCE_MACOSX = 15; > 62: static volatile boolean failed = false; The field `failed` in unused, it can be removed then. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 119: > 117: Math.abs(borderShift) == 1 ? > 118: borderShift : > 119: (borderShift / 2); Suggestion: borderShift = Math.abs(borderShift) == 1 ? borderShift : (borderShift / 2); I prefer this formatting. Yet we don't have a style guide to follow. In general, wrapping before the operator makes it clearer that it's a continuation line. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 169: > 167: SwingUtilities.invokeAndWait(new Runnable() { > 168: > 169: public void run() { Suggestion: SwingUtilities.invokeAndWait(new Runnable() { public void run() { I'd rather not add this line… On the other hand, it's more or less consistent with other cases where the `Runnable` interface is implemented. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 255: > 253: public static void main(String args[]) throws Exception { > 254: > 255: if (!Toolkit.getDefaultToolkit().isDynamicLayoutActive()) { I'm strongly against adding blank lines at the start of a method. The declaration of a method serves as the start of a new logical block by itself. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 277: > 275: //Timed out, so fail the test > 276: throw new RuntimeException( > 277: "Timed out after " + sleepTime / 1000 + " seconds"); Suggestion: "Timed out after " + (sleepTime / 1000) + " seconds"); Parentheses aren't required here, yet they help avoid any ambiguity. test/jdk/java/awt/Mixing/AWT_Mixing/OpaqueOverlapping.java line 1: > 1: /* If there's nothing else modified in the `OpaqueOverlapping.java` file, I'd rather revert these changes and leave the file untouched, moreover importing `java.lang.Override` is not needed. test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 32: > 30: import java.awt.Dimension; > 31: import java.awt.Font; > 32: import java.awt.Helper; I'd probably put the helper into the section of helper classes… but this means that imports need manual editing after IDE updates them for you. So, probably leave it as is now. test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 47: > 45: import java.io.InputStream; > 46: import java.io.InputStreamReader; > 47: import java.lang.Override; No need to import `Override`. test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 389: > 387: return !(component == null || > 388: (component instanceof Scrollbar) || > 389: (isMac && (component instanceof Button))); Suggestion: return !(component == null || (component instanceof Scrollbar) || (isMac && (component instanceof Button))); Wrap before the operator to clearly indicate a continuation line. test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 389: > 387: return !(component == null || > 388: (component instanceof Scrollbar) || > 389: (isMac && (component instanceof Button))); Does inverting the condition makes it clearer? return component != null && !(component instanceof Scrollbar) && !(isMac && (component instanceof Button)); That is verify any non-null component except `Scrollbar`, and in addition `Button` on macOS. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 161: > 159: JFrame ancestor = (JFrame) > (testedComponent.getTopLevelAncestor()); > 160: final CountDownLatch latch = new CountDownLatch(1); > 161: if (ancestor != null) { Suggestion: final CountDownLatch latch = new CountDownLatch(1); JFrame ancestor = (JFrame) (testedComponent.getTopLevelAncestor()); if (ancestor != null) { I suggest moving the declarations around. The `latch` is helper object. You then get the ancestor and work with it. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 171: > 169: latch.countDown(); > 170: } > 171: try { Add a blank line between the `if` statement and `try`. The `if` block prepares for the code inside `try`, they're related, but still do separate job. Reading code that separates logical blocks with blank lines is easier. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 173: > 171: try { > 172: boolean await = latch.await(1, TimeUnit.SECONDS); > 173: if (!await) { Suggestion: if (!latch.await(1, TimeUnit.SECONDS)) { The usage of a local variable doesn't add clarity. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 181: > 179: throw new RuntimeException(e); > 180: } > 181: if (ancestor != null && isMultiFramesTest()) { Move the call to `clickAndBlink` back here along with the blank line. You [say](https://github.com/openjdk/jdk/pull/26625/changes#r2650824338), “`clickAndBlink` happens only when we get focus”, which is directly after the `try` block. If the focus isn't received, an exception is thrown from the `try` block. ------------- Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/26625#pullrequestreview-3797926253 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804821645 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804848528 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804834467 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804799689 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804853122 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804864577 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804869126 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804874542 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804873518 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804879173 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804891685 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804922132 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804908185 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2804912887 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805615814 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805612960 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805633218 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805636375 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805645029 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805647581 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805658685 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805702649 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805662864 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805689266 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805695680 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805715826 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805726748 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805730100 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2805750733
