4512626: Non-editable JTextArea provides no visual indication of keyboard focus #21
Conversation
…oard focus Set the text caret to be visible but not blinking on the non-editable text area. Fix the regression test that becames unstable after the change.
|
Continuation of openjdk/jdk#11408 to be integrated into JDK20 workspace. |
|
👋 Welcome back kizune! A progress list of the required criteria for merging this PR into |
|
@azuev-java The following label will be automatically applied to this pull request:
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. |
Webrevs
|
| @@ -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()) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I have fixed the second test and removed both of them from the problem list.
|
@azuev-java 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: 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 35 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
|
/issue add 8194048 |
|
/issue add 8213562 |
|
@azuev-java |
|
@azuev-java |
Removed both fixed tests from ProblemList
Done and done. |
| if (flasher == null) { | ||
| flasher = new Timer(rate, handler); | ||
| } | ||
| flasher.setDelay(rate); |
There was a problem hiding this comment.
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
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.
| } | ||
| } else { | ||
| if (getBlinkRate() != 0) { | ||
| savedBlinkRate = getBlinkRate(); |
There was a problem hiding this comment.
Look like this can override blink rate set by user:
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JTextField;
import javax.swing.SwingUtilities;
import java.awt.BorderLayout;
import java.awt.Component;
import java.awt.Robot;
import java.lang.reflect.InvocationTargetException;
public class CaretThing {
static JFrame frame;
static JTextField textField;
static Robot robot;
static JButton button;
static void printBlinkRate() {
System.err.println("Caret blink rate: " + textField.getCaret().getBlinkRate());
}
static void requestFocus(Component component) throws InterruptedException, InvocationTargetException {
robot.waitForIdle();
robot.delay(300);
SwingUtilities.invokeAndWait(() -> {
System.err.println("Requesting focus on " + component);
printBlinkRate();
component.requestFocus();
printBlinkRate();
});
}
static void changeEditable() throws InterruptedException, InvocationTargetException {
robot.waitForIdle();
robot.delay(300);
SwingUtilities.invokeAndWait(()-> {
System.err.println("Changing editable");
printBlinkRate();
textField.setEditable(!textField.isEditable());
printBlinkRate();
});
}
public static void main(String[] args) throws Exception {
robot = new Robot();
robot.setAutoWaitForIdle(true);
robot.setAutoDelay(50);
SwingUtilities.invokeAndWait(() -> {
frame = new JFrame("Hey");
frame.setLocationRelativeTo(null);
textField = new JTextField("Some long field value");
textField.setEditable(false);
frame.setLayout(new BorderLayout());
frame.add(textField, BorderLayout.NORTH);
button = new JButton("Button");
button.addActionListener((e)-> printBlinkRate());
frame.add(button, BorderLayout.CENTER);
frame.pack();
frame.setVisible(true);
button.requestFocus();
textField.getCaret().setBlinkRate(2500);
});
requestFocus(textField);
requestFocus(button); // comment this to get 0 blink rate on editable field
changeEditable();
requestFocus(textField);
SwingUtilities.invokeAndWait(CaretThing::printBlinkRate);
}
}Running the sample above at least two possible misbehaviors(tested on Linux):
- you will receive blink rate
500instead of user set2500 - but if you comment
requestFocus(button);line then blink rate will be0(it shows non-blinking cursor)
Other than that I am worried about getBlinkRate() does not return the same value as passed tosetBlinkRate() (e.g. case when component is not editable, possible JCK issue?)
There was a problem hiding this comment.
I updated the code now the behaviour is correct (as far as i tested) and the blink rate is reported as if it is set to the correct value even on non-editable text element.
There was a problem hiding this comment.
I am still having issue with non-blinking cursor on editable field with the latest version of the fix.
Fails for me with test provided above and commented requestFocus(button); line.
There was a problem hiding this comment.
The latest version does not work good for me,
e.g. with the test above and textField.getCaret().setBlinkRate(250)(not 2500):
- with
requestFocus(button);:
caret blinks several times and stops
on mouse move, focus gained on text field the caret blinks several times and stops again. - without
requestFocus(button);:
it doesn't blink until focus out / focus in on the text component
after this blinks several times and stops.
There was a problem hiding this comment.
That's weird, does not happen on any of my systems. Which OS are you testing on?
There was a problem hiding this comment.
Ubuntu 22.04 and it reproduces always.
There was a problem hiding this comment.
Ok, i tried on Ubuntu 22 on x11 desktop and i see this behavior. However - i checked out master and i also can see the same glitch so this is not related to my fix.
There was a problem hiding this comment.
Can confirm, filed JDK-8299050 and JDK-8299051 to track them.
P.S. Just saw that you already filed them as JDK-8299047 and JDK-8299048
…s in different states causing some confused result. Also added code that in case of non-editable text component reports the saved blink rate to avoid any problems with JCK testing.
| @@ -366,6 +366,8 @@ protected void moveCaret(MouseEvent e) { | |||
| } | |||
| } | |||
|
|
|||
| private int savedBlinkRate = 0; | |||
| private boolean blinkRateSaved = false; | |||
There was a problem hiding this comment.
Naming of savedBlinkRate and blinkRateSaved looks almost the same and confusing for me.
Probably isBlinkRateSaved would be better to highlight boolean variable.
| } | ||
| } else { | ||
| if (getBlinkRate() != 0) { | ||
| savedBlinkRate = getBlinkRate(); |
There was a problem hiding this comment.
I am still having issue with non-blinking cursor on editable field with the latest version of the fix.
Fails for me with test provided above and commented requestFocus(button); line.
… editable state switched on a focused text field
|
Ok, i fixed the permanent blinking caret glitch. Now it is non-blinking until the focus is lost and regained. Which is an improvement, without this fix the caret would be invisible until the focus lost. |
|
/integrate |
|
Going to push as commit 3e17e3c.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@azuev-java Pushed as commit 3e17e3c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Set the text caret to be visible but not blinking on the non-editable text area. Fix the regression test that becames unstable after the change.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk20 pull/21/head:pull/21$ git checkout pull/21Update a local copy of the PR:
$ git checkout pull/21$ git pull https://git.openjdk.org/jdk20 pull/21/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21View PR using the GUI difftool:
$ git pr show -t 21Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk20/pull/21.diff