Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

4512626: Non-editable JTextArea provides no visual indication of keyboard focus #21

Closed
wants to merge 5 commits into from
Closed

4512626: Non-editable JTextArea provides no visual indication of keyboard focus #21

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9463a0b
4512626: Non-editable JTextArea provides no visual indication of keyb…
azuev-java Dec 12, 2022
70da8f6
Fixed MultiSelectionText so it is now stable on Linux
azuev-java Dec 15, 2022
3b9d9cf
Handle different cases where refresh rate can be set when component i…
azuev-java Dec 16, 2022
abf5d77
Fixing situation where we can be stuck with non-blinking cursor after…
azuev-java Dec 17, 2022
e11d54b
Renamed blinkRateSaved to isBlinkRateSaved
azuev-java Dec 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 40 additions & 5 deletions src/java.desktop/share/classes/javax/swing/text/DefaultCaret.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -366,6 +366,8 @@ protected void moveCaret(MouseEvent e) {
}
}

private int savedBlinkRate = 0;
private boolean blinkRateSaved = false;
Copy link
Member

Choose a reason for hiding this comment

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

Naming of savedBlinkRate and blinkRateSaved looks almost the same and confusing for me.
Probably isBlinkRateSaved would be better to highlight boolean variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

// --- FocusListener methods --------------------------

/**
Expand All @@ -379,8 +381,21 @@ protected void moveCaret(MouseEvent e) {
public void focusGained(FocusEvent e) {
if (component.isEnabled()) {
if (component.isEditable()) {
setVisible(true);
if (blinkRateSaved) {
setBlinkRate(savedBlinkRate);
savedBlinkRate = 0;
blinkRateSaved = false;
}
} else {
if (getBlinkRate() != 0) {
if (!blinkRateSaved) {
savedBlinkRate = getBlinkRate();
blinkRateSaved = true;
}
setBlinkRate(0);
}
}
setVisible(true);
setSelectionVisible(true);
updateSystemSelection();
}
Expand Down Expand Up @@ -1031,17 +1046,34 @@ public void setVisible(boolean e) {
* @see Caret#setBlinkRate
*/
public void setBlinkRate(int rate) {
if (rate < 0) {
throw new IllegalArgumentException("Invalid blink rate: " + rate);
}
if (rate != 0) {
if (flasher == null) {
flasher = new Timer(rate, handler);
if (component.isEditable()) {
if (flasher == null) {
flasher = new Timer(rate, handler);
}
flasher.setDelay(rate);
Copy link
Member

Choose a reason for hiding this comment

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

Coming back to negative rate, looks like this changes the previous behavior:

Before the fix it did throw IAE in all cases:

jdk20/src/java.desktop/share/classes/javax/swing/Timer.java

Lines 397 to 406 in ca39eb9

public void setDelay(int delay) {
checkDelay(delay, "Invalid delay: ");
this.delay = delay;
}
private static void checkDelay(int delay, String message) {
if (delay < 0) {
throw new IllegalArgumentException(message + delay);
}
}

After the fix IAE is thrown only if component is editable

In case of non-editable and may throw it way later after setEditable(true) call and focus gain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

if (!flasher.isRunning()){
flasher.restart();
}
} else {
savedBlinkRate = rate;
blinkRateSaved = true;
}
flasher.setDelay(rate);
} else {
if (flasher != null) {
flasher.stop();
flasher.removeActionListener(handler);
flasher = null;
}
if (component.isEditable()) {
if (blinkRateSaved) {
savedBlinkRate = 0;
blinkRateSaved = false;
}
}
}
}

Expand All @@ -1053,6 +1085,9 @@ public void setBlinkRate(int rate) {
* @see Caret#getBlinkRate
*/
public int getBlinkRate() {
if (blinkRateSaved) {
return savedBlinkRate;
}
return (flasher == null) ? 0 : flasher.getDelay();
}

Expand Down
2 changes: 0 additions & 2 deletions test/jdk/ProblemList.txt
View file
Open in desktop
Expand Up @@ -656,8 +656,6 @@ javax/swing/JPopupMenu/6800513/bug6800513.java 7184956 macosx-all
javax/swing/JTabbedPane/8007563/Test8007563.java 8051591 generic-all
javax/swing/JTabbedPane/4624207/bug4624207.java 8064922 macosx-all
javax/swing/SwingUtilities/TestBadBreak/TestBadBreak.java 8160720 generic-all
javax/swing/text/DefaultCaret/HidingSelection/HidingSelectionTest.java 8194048 windows-all
javax/swing/text/DefaultCaret/HidingSelection/MultiSelectionTest.java 8213562 linux-all
javax/swing/JFileChooser/6798062/bug6798062.java 8146446 windows-all
javax/swing/JPopupMenu/4870644/bug4870644.java 8194130 macosx-all,linux-all
javax/swing/dnd/8139050/NativeErrorsInTableDnD.java 8202765 macosx-all,linux-all
Expand Down
53 changes: 14 additions & 39 deletions test/jdk/javax/swing/text/DefaultCaret/HidingSelection/HidingSelectionTest.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -21,10 +21,17 @@
* questions.
*/

import javax.swing.*;
import java.awt.*;
import javax.swing.JFrame;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JTextField;
import javax.swing.MenuSelectionManager;
import javax.swing.SwingUtilities;
import java.awt.FlowLayout;
import java.awt.Point;
import java.awt.Robot;
import java.awt.event.InputEvent;
import java.awt.image.BufferedImage;

/**
* @test
Expand All @@ -39,7 +46,6 @@ public class HidingSelectionTest {
private static JTextField field1;
private static JTextField field2;
private static JFrame frame;
private static Rectangle bounds;
private static JMenu menu;
private static JTextField anotherWindow;
private static Point menuLoc;
Expand Down Expand Up @@ -67,17 +73,9 @@ public static void main(String[] args) throws Exception {
Robot robot = new Robot();
robot.waitForIdle();
robot.delay(200);
SwingUtilities.invokeAndWait(() -> {
bounds = field2.getBounds();
bounds.setLocation(field2.getLocationOnScreen());
});
BufferedImage nosel = robot.createScreenCapture(bounds);

SwingUtilities.invokeAndWait(field2::requestFocus);
SwingUtilities.invokeAndWait(field2::selectAll);
robot.waitForIdle();
robot.delay(200);
BufferedImage sel = robot.createScreenCapture(bounds);

SwingUtilities.invokeAndWait(() -> {
menuLoc = menu.getLocationOnScreen();
Expand All @@ -89,7 +87,7 @@ public static void main(String[] args) throws Exception {
robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
robot.waitForIdle();
robot.delay(200);
if (!biEqual(robot.createScreenCapture(bounds), sel)) {
if (!field2.getCaret().isSelectionVisible()) {
throw new RuntimeException("Test fails: menu hides selection");
}

Expand All @@ -98,7 +96,7 @@ public static void main(String[] args) throws Exception {
SwingUtilities.invokeAndWait(field1::requestFocus);
robot.waitForIdle();
robot.delay(200);
if (!biEqual(robot.createScreenCapture(bounds), nosel)) {
if (field2.getCaret().isSelectionVisible()) {
throw new RuntimeException(
"Test fails: focus lost doesn't hide selection");
}
Expand All @@ -119,35 +117,12 @@ public static void main(String[] args) throws Exception {
SwingUtilities.invokeAndWait(anotherWindow::requestFocus);
robot.waitForIdle();
robot.delay(200);
if (biEqual(robot.createScreenCapture(bounds), nosel)) {
if (!field2.getCaret().isSelectionVisible()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need similar change in test/jdk/javax/swing/text/DefaultCaret/HidingSelection/MultiSelectionTest.java
as well?
Since the logic is changed, I will still request to make this test stable on windows (now that we are not using screen capture) and then deproblemlist

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it need similar change in test/jdk/javax/swing/text/DefaultCaret/HidingSelection/MultiSelectionTest.java as well? Since the logic is changed, I will still request to make this test stable on windows (now that we are not using screen capture) and then deproblemlist

I haven't tested the MultiSelectionTest but since it is problemlisted on Linux similar change might make it more stable. As of this test - since you are the owner of JDK-8194048 i thought that you can review my changes and deproblemlist the test if it is stable on the rest of the platforms and if changes make sense. I definitely do not want to steal your thunder. As of MultiSelectionTest i can take care of it either here or in the original bug that is noted in ProblemList.txt since my fix does not affect it.

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 anyway one is supposed to do a CI job for the fix and test the regression test on all platforms, so if the modified test is problemlisted it will not be tested in CI job, so it's better to deproblemlist and test...
You can do /issue add of that problemlisted bug (it just happens to be in my name for the time being, nothing stopping you assigning to you since you did all the hard work)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have fixed the second test and removed both of them from the problem list.

throw new RuntimeException(
"Test fails: switch window hides selection");
}

SwingUtilities.invokeAndWait(anotherWindow::selectAll);
robot.waitForIdle();
robot.delay(200);
if (biEqual(robot.createScreenCapture(bounds), sel)) {
throw new RuntimeException(
"Test fails: selection ownership is lost selection is shown");
}

SwingUtilities.invokeLater(frame2::dispose);
SwingUtilities.invokeLater(frame::dispose);
}

static boolean biEqual(BufferedImage i1, BufferedImage i2) {
if (i1.getWidth() == i2.getWidth() &&
i1.getHeight() == i2.getHeight()) {
for (int x = 0; x < i1.getWidth(); x++) {
for (int y = 0; y < i1.getHeight(); y++) {
if (i1.getRGB(x, y) != i2.getRGB(x, y)) {
return false;
}
}
}
return true;
}
return false;
}
}