8360070: AccessibleText.getBeforeIndex returns null for last character

[kumarabhi006]

AccessibleText.getBeforeIndex method returns null for last character due to the wrong boundary value condition check.
This method returns null when the passed index parameter is equal to text's length which is incorrect.
getBeforeIndex method should return null only if the passed index parameter is less than 0 and greater than the text's length.

After modifying the condition check, expected character is returned. Test is added to verify the check,

---

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-8360070: AccessibleText.getBeforeIndex returns null for last character (Bug - P4)

Reviewers

• Alexey Ivanov (@aivanov-jdk - Reviewer)
• Alexander Zuev (@azuev-java - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25941

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back abhiscxk! 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]

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

8360070: AccessibleText.getBeforeIndex returns null for last character

Reviewed-by: aivanov, kizune

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

• a49ecb2: 8352635: Improve inferencing of Float16 operations with constant inputs
• 7f702cf: 8360458: Rename Deferred<> to DeferredStatic<> and improve usage description
• bd666f9: 8360522: G1: Flag constraint functions for G1SATBBufferSize and G1UpdateBufferSize are skipped during argument validation
• ... and 35 more: https://git.openjdk.org/jdk/compare/a3609ba5ac6a5531d905cdd136eea72706f9a028...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]

@kumarabhi006 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]

Webrevs

• 02: Full - Incremental (e9c5ae37)
• 01: Full - Incremental (b7852974)
• 00: Full (6e5eef63)

[mrserb]

getBeforeIndex method should return null only if the passed index parameter is less than 0 and greater than the text's length.

I am not sure about the statement above. I think the check should take care of the direction, which is -1 in your case. This is actually properly handled by the code below(in t he same method you changed):

                    if (index + direction < model.getLength() &&
                        index + direction >= 0) {
                        return model.getText(index + direction, 1);
                    }

The code you added also affects WORD and SENTENCE cases. I suggest covering those with a test as well.

[kumarabhi006]

I am not sure about the statement above. I think the check should take care of the direction, which is -1 in your case

I think the check is to ensure the passed index parameter is to verify the boundary of text length.

This is actually properly handled by the code below(in t he same method you changed):

If the index is equal to the the text length, the returned character from the getBeforeIndex should return the last character i.e. 6 but the existing condition evaluates to true due to index equals to model.getLength() and method returns null. So, even the direction is handled correctly below, execution doesn't reach to that point.

[kumarabhi006]

@mrserb The code you added also affects WORD and SENTENCE cases. I suggest covering those with a test as well.

Yes, that affects the WORD and SENTENCE cases. I have extended the test to cover them.
Before the fix the returned value for WORD and SENTENCE is null that seems incorrect as well.

I think the return string in case of WORD should be Test6 and SENTENCE should be Test4 Test5.

[aivanov-jdk]

Why not include the values in the error message?

Suggested change

	throw new RuntimeException("The actual result does not match " +
	"the expected result.");
	throw new RuntimeException("Result doesn't match: '" + actual + "' vs. '" + expected + "'");

[mrserb]

I think the check is to ensure the passed index parameter is to verify the boundary of text length.

Yes, but the index passed to the method is an index within the text, so it should be from 0 to length - 1. You can take a look at the opposite case when the index is outside the range, for example at the beginning of the text and getAfterIndex:

result = at.getAfterIndex(AccessibleText.CHARACTER, -1);
verifyResult("T", result);

Is that expectation correct or not? I think we should first decide whether this is actually a bug or not.

[kumarabhi006]

I think the check is to ensure the passed index parameter is to verify the boundary of text length.

Yes, but the index passed to the method is an index within the text, so it should be from 0 to length - 1. You can take a look at the opposite case when the index is outside the range, for example at the beginning of the text and getAfterIndex:

result = at.getAfterIndex(AccessibleText.CHARACTER, -1);
verifyResult("T", result);

Is that expectation correct or not? I think we should first decide whether this is actually a bug or not.

I got your point now. The spec says that index an index within the text and that means the value should range from 0 to length - 1. And that means, getBeforeIndex won't be able to fetch the last character and getAfterIndex won't be able to fetch the first character.

So, if someone wants to retrieve the first or last character, they shouldn't rely on getAfterIndex and getBeforeIndex method respectively, rather use getAtIndex method to get the first and last character with index passed as 0 and length -1 respectively.

Right ?

[aivanov-jdk]

The spec says

What spec in particular?

[kumarabhi006]

The spec says

What spec in particular?

https://docs.oracle.com/en/java/javase/24/docs/api/java.desktop/javax/accessibility/AccessibleText.html#getBeforeIndex(int,int)

It says, index - an index within the text for the getBeforeIndex method description

[mrserb]

So, if someone wants to retrieve the first or last character, they shouldn't rely on getAfterIndex and getBeforeIndex method respectively, rather use getAtIndex method to get the first and last character with index passed as 0 and length -1 respectively.

Right ?

That is my understanding.

[azuev-java]

Looks good.

[kumarabhi006]

@aivanov-jdk @azuev-java As per the discussion with @mrserb here, it seems this is not an issue with the code. I would like to know about your opinion ?

If this is not an issue to fix, can we update the javadoc for the APIs for better understanding ?

Something like getBeforeIndex shouldn't be used to retrieve the last character or word or sentence and similar for getAfterIndex and getAtIndex APIs.

[mrserb]

Something like getBeforeIndex shouldn't be used to retrieve the last character or word or sentence and similar for getAfterIndex and getAtIndex APIs.

It might be useful

[aivanov-jdk]

I modified Abhishek's test:

            System.out.println(" i  bef   at  aft");
            for (int i : new int[] {0, 1, 2, 8, 9, 10}) {
                System.out.printf("%2d%5s%5s%5s\n",
                                  i,
                                  at.getBeforeIndex(CHARACTER, i),
                                  at.getAtIndex(CHARACTER, i),
                                  at.getAfterIndex(CHARACTER, i));
            }

This way, I created a table of returned values:

Text: "0123456789"
Text Length: 10
 i  bef   at  aft
 0 null    0    1
 1    0    1    2
 2    1    2    3
 8    7    8    9
 9    8    9 null
10 null null null

I'd expect to see 9 as the return value of getBeforeIndex for the index of 10. This would make the table symmetrical.

Therefore, I'm in favour for Abhishek's proposed fix.

---

The description of the methods in AccessibleText is vague. None of the three methods specifies what the index means and what values are accepted for it.

According to the text model, index 10 is still valid, it's the index between the last character and the implied line break, so tf.getDocument().getText(10, 1) returns \n.

Having this in mind, the specification of the getBeforeIndex, getAtIndex, getAfterIndex methods should be updated to explicitly specify the valid values for the index parameter.

[azuev-java]

I got your point now. The spec says that index an index within the text and that means the value should range from 0 to length - 1. And that means, getBeforeIndex won't be able to fetch the last character and getAfterIndex won't be able to fetch the first character.

Here i disagree, the "within the text" does not imply that the position behind the last character is not within the text. Otherwise with the caret is at the end of the text it is impossible to request last word or character before the caret position which is one of the valid use cases of this accessibility method. I think the fix is valid.

[mrserb]

I'd expect to see 9 as the return value of getBeforeIndex for the index of 10. This would make the table symmetrical.

Why do you expect that? The spec states that the index should be "an index within the text." That index is also used in other methods, such as getCharacterBounds(), etc. If we fixed it only for text at the end, it would make the behavior even less symmetric, since for a negative value, the model will always throw an exception.

According to the text model, index 10 is still valid, it's the index between the last character and the implied line break, so tf.getDocument().getText(10, 1) returns \n.

The last character is implementation detail, it is not part of the "users data" this is the reason why it is excluded from the Document.getLength(). So passing length as a last character might cause an exception for custom documents. We also have the getCharCount() methods which returns "the number of characters (valid indices)".

Also, why are we only talking about JTextComponent? There are other same implementations of AccessibleText for example JLabel.

Here i disagree, the "within the text" does not imply that the position behind the last character is not within the text. Otherwise with the caret is at the end of the text it is impossible to request last word or character before the caret position which is one of the valid use cases of this accessibility method. I think the fix is valid.

It means exactly that, and it is also impossible to request the first character after the caret position if it is at the start of the document. For both cases getAtIndex() should work.

[OnePatchGuy]

I agree with @mrserb: there already is getAtIndex and the documentation is clear about that the index should be within the text. From my side I'd add that changing this behavior may break backward-compatibility with client's custom AWT/Swing components.