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

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

Closed
wants to merge 5 commits into from

Conversation

azuev-java
Copy link
Member

@azuev-java azuev-java commented Dec 12, 2022

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

  • 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

Issues

  • JDK-4512626: Non-editable JTextArea provides no visual indication of keyboard focus
  • JDK-8194048: Regression automated test '/open/test/jdk/javax/swing/text/DefaultCaret/HidingSelection/HidingSelectionTest.java' fails
  • JDK-8213562: Test javax/swing/text/DefaultCaret/HidingSelection/MultiSelectionTest.java fails

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk20/pull/21.diff

…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.
@azuev-java
Copy link
Member Author

Continuation of openjdk/jdk#11408 to be integrated into JDK20 workspace.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 12, 2022

👋 Welcome back kizune! 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
Copy link

openjdk bot commented Dec 12, 2022

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

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Dec 12, 2022
@mlbridge
Copy link

mlbridge bot commented Dec 12, 2022

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()) {
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.

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

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

It would be better to verify the updated test passes on Windows. If so, remove it from the problem list.

@openjdk
Copy link

openjdk bot commented Dec 13, 2022

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

4512626: Non-editable JTextArea provides no visual indication of keyboard focus
8194048: Regression automated test '/open/test/jdk/javax/swing/text/DefaultCaret/HidingSelection/HidingSelectionTest.java' fails
8213562: Test javax/swing/text/DefaultCaret/HidingSelection/MultiSelectionTest.java fails

Reviewed-by: aivanov, azvegint

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 master branch:

  • 188aaef: 8277074: Qualified exported types show up in JavaDoc
  • 2c69c41: 8298894: java/lang/Thread/virtual/stress/Skynet.java timed out and threw OutOfMemoryError
  • 3410555: 8298987: ProblemList jdk/internal/vm/Continuation/Fuzz.java#default with ZGC on X64
  • d102672: 8298905: Test "java/awt/print/PrinterJob/ImagePrinting/PrintARGBImage.java" fails because the frames of instruction does not display
  • 41cc044: 8298970: Problem list java/awt/event/KeyEvent/KeyTyped/CtrlASCII.java
  • 3b7970c: 8298217: Regressions 30-110% in SwingMark on MacOS, more so on aarch64
  • 0ecad28: 8298976: ProblemList java/util/concurrent/ExecutorService/CloseTest.java on macosx-aarch64
  • c997b5b: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10
  • 9e10f00: 8298919: Add a regression test for JDK-8298520
  • b14794d: 8298852: Use of uninitialized memory in MetadataFactory::free_metadata
  • ... and 25 more: https://git.openjdk.org/jdk20/compare/0267aa528b83be9914fee4bea8f548b8404b31f8...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 13, 2022
@azuev-java
Copy link
Member Author

/issue add 8194048

@azuev-java
Copy link
Member Author

/issue add 8213562

@openjdk
Copy link

openjdk bot commented Dec 15, 2022

@azuev-java
Adding additional issue to issue list: 8194048: Regression automated test '/open/test/jdk/javax/swing/text/DefaultCaret/HidingSelection/HidingSelectionTest.java' fails.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Dec 15, 2022
@openjdk
Copy link

openjdk bot commented Dec 15, 2022

@azuev-java
Adding additional issue to issue list: 8213562: Test javax/swing/text/DefaultCaret/HidingSelection/MultiSelectionTest.java fails.

Removed both fixed tests from ProblemList
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 15, 2022
@azuev-java
Copy link
Member Author

It would be better to verify the updated test passes on Windows. If so, remove it from the problem list.

Done and done.

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:

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.

}
} else {
if (getBlinkRate() != 0) {
savedBlinkRate = getBlinkRate();
Copy link
Member

Choose a reason for hiding this comment

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

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 500 instead of user set 2500
  • but if you comment requestFocus(button); line then blink rate will be 0 (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?)

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

The latest version does not work good for me,
e.g. with the test above and textField.getCaret().setBlinkRate(250)(not 2500):

  1. 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.
  2. without requestFocus(button);:
    it doesn't blink until focus out / focus in on the text component
    after this blinks several times and stops.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's weird, does not happen on any of my systems. Which OS are you testing on?

Copy link
Member

Choose a reason for hiding this comment

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

Ubuntu 22.04 and it reproduces always.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@azvegint azvegint Dec 19, 2022

Choose a reason for hiding this comment

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

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

}
} else {
if (getBlinkRate() != 0) {
savedBlinkRate = getBlinkRate();
Copy link
Member

Choose a reason for hiding this comment

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

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.

@azuev-java
Copy link
Member Author

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.

@azuev-java
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Dec 19, 2022

Going to push as commit 3e17e3c.
Since your change was applied there have been 35 commits pushed to the master branch:

  • 188aaef: 8277074: Qualified exported types show up in JavaDoc
  • 2c69c41: 8298894: java/lang/Thread/virtual/stress/Skynet.java timed out and threw OutOfMemoryError
  • 3410555: 8298987: ProblemList jdk/internal/vm/Continuation/Fuzz.java#default with ZGC on X64
  • d102672: 8298905: Test "java/awt/print/PrinterJob/ImagePrinting/PrintARGBImage.java" fails because the frames of instruction does not display
  • 41cc044: 8298970: Problem list java/awt/event/KeyEvent/KeyTyped/CtrlASCII.java
  • 3b7970c: 8298217: Regressions 30-110% in SwingMark on MacOS, more so on aarch64
  • 0ecad28: 8298976: ProblemList java/util/concurrent/ExecutorService/CloseTest.java on macosx-aarch64
  • c997b5b: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10
  • 9e10f00: 8298919: Add a regression test for JDK-8298520
  • b14794d: 8298852: Use of uninitialized memory in MetadataFactory::free_metadata
  • ... and 25 more: https://git.openjdk.org/jdk20/compare/0267aa528b83be9914fee4bea8f548b8404b31f8...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 19, 2022
@openjdk openjdk bot closed this Dec 19, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 19, 2022
@openjdk
Copy link

openjdk bot commented Dec 19, 2022

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants