8329667: [macos] Issue with JTree related fix for JDK-8317771 #19255

azuev-java · 2024-05-16T01:28:37Z

Caching children and selected children of the thee on the native level;
Caching all children of a specific parent in CAccessibility to enhance recursive children selection algorithm;
Removing fix for JDK-8317771 as no longer needed;

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-8329667: [macos] Issue with JTree related fix for JDK-8317771 (Bug - P3)

Reviewers

Artem Semenov (@savoptik - Committer)
Abhishek Kumar (@kumarabhi006 - Committer)
Prasanta Sadhukhan (@prsadhuk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19255

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

Using diff file

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

Webrev

Link to Webrev Comment

Caching children and selected children of the thee on the native level; Caching all children of a specific parent in CAccessibility to enhance recursive children selection algorythm; Removing fix for JDK-8317771 as no longer needed;

bridgekeeper · 2024-05-16T01:29:13Z

👋 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 · 2024-05-16T01:29:47Z

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

8329667: [macos] Issue with JTree related fix for JDK-8317771

Reviewed-by: asemenov, abhiscxk, psadhukhan

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

239c1b3: 8332807: Parallel: Make some APIs in ParMarkBitMap private
9b61a76: 8332615: RISC-V: Support vector unsigned comparison instructions for machines with RVV
a71b404: 8331398: G1: G1HeapRegionPrinter reclamation events should print the original region type
af056c1: 8332106: VerifyError when using switch pattern in this(...) or super(...)
da3001d: 8331975: Enable case-insensitive check in ccache and keytab entry lookup
424eb60: 8331683: Clean up GetCarrierThread
9b1d6d6: 8316328: Test jdk/jfr/event/oldobject/TestSanityDefault.java times out for some heap sizes
f8a3e4e: 8328998: Encoding support for Intel APX extended general-purpose registers
ddd73b4: 8332082: Shenandoah: Use consistent tests to determine when pre-write barrier is active
0a9d1f8: 8332749: Broken link in MemorySegment.Scope.html
... and 171 more: https://git.openjdk.org/jdk/compare/2d622152b07bba0aba8fd5b1e24293e28d6e69f5...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 · 2024-05-16T01:30:29Z

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

mlbridge · 2024-05-16T01:33:30Z

Webrevs

victordyakov · 2024-05-16T16:04:08Z

@savoptik @kumarabhi006 @prsadhuk please review

savoptik · 2024-05-18T18:35:15Z

src/java.desktop/macosx/native/libawt_lwawt/awt/a11y/OutlineAccessibility.h

+ NSMutableArray<id<NSAccessibilityRow>> *rowCache;
+ BOOL rowCacheValid;
+ NSMutableArray<id<NSAccessibilityRow>> *selectedRowCache;
+ BOOL selectedRowCacheValid;


And can we use a single array for the cache in the same way as we do in Table Accessibility?

They have different meanings and different content - plus selection being requested by OS much more frequently. And we do not want to invalidate the whole rows cache when selection changes - otherwise it will be pretty costly.

savoptik · 2024-05-18T18:39:42Z

src/java.desktop/macosx/native/libawt_lwawt/awt/a11y/CommonComponentAccessibility.m

@@ -349,6 +349,9 @@ - (void)postSelectedTextChanged
 - (void)postSelectionChanged
 {
 AWT_ASSERT_APPKIT_THREAD;
+ if ([self respondsToSelector:NSSelectorFromString(@"invalidateSelectionCache")]) {


You can override this method for the tree. to avoid checking the selector support... This is a time-consuming procedure.

- Rolled back changes to the CommonComponentAccessibility and added relevant post* methods overrides to the OutlineAccessibility.

kumarabhi006 · 2024-05-17T10:00:08Z

src/java.desktop/macosx/classes/sun/lwawt/macosx/CAccessibility.java

@@ -39,7 +39,9 @@
 import java.lang.reflect.Constructor;


Please remove unused import import javax.swing.tree.TreePath;.

savoptik · 2024-05-20T07:16:49Z

src/java.desktop/macosx/native/libawt_lwawt/awt/a11y/OutlineAccessibility.m

+ [super postTreeNodeExpanded];
+}
+
+- (void)postSelectedCellsChanged


Does OutlineRowAccessibility use CellAccessibility?

It does not - i am just covering all methods that can be potentially called on the table-like controls. It does no harm to override method that will never be called. But it will cause some discrepancies in the behavior if some custom control will call this notification and we fail to update the selection cache accordingly.

savoptik

LGTM
It seems to me that it would be a good idea to do the same for lists.

kumarabhi006 · 2024-05-21T08:45:54Z

src/java.desktop/macosx/native/libawt_lwawt/awt/a11y/OutlineAccessibility.m

+
+- (BOOL)isCacheValid
+{
+ if (rowCacheValid && [[self parent] respondsToSelector:NSSelectorFromString(@"isCacheValid")]) {


When isCacheValid is invoked for the first time then rowCacheValid is FALSE and we return FALSE value from this method and then the accessibilityChildren method from super class is invoked to get all children and store them in an array(rowCache). Subsequently rowCacheValid will be set to TRUE.

As per my understanding [[self parent] respondsToSelector:NSSelectorFromString(@"isCacheValid")] condition will be checked only if rowCacheValid is true and once we have a valid array of children then what is the point of checking isCacheValid method in parent ?

In multilevel trees when someone in the parent's path is collapsed the treeCollapsed event will be delivered to that node. We can either invalidate all leaf nodes caches (which is costly) or mark that exact node's cache as invalid and when the expand will happen when a11y cursor will try to position inside one of the childrens it will check that parent was invalidated and will regenerate its own cache.

Ok.
Is it possible to add a test to verify this behavior?

The test would be manual and it is hard to even describe what is the result of the test should look or sound like since we do not provide any string messages, VoiceOver does it and we can not tell ahead how it will be put. And for multilivel trees i am just running the SwingSet2 JTree demo and see that VoiceOver correctly handles full/partial collapse and expand - that after that there are no anomalies like VO output does not correspond to where cursor actually positioned.

Ok. I was thinking of something like when nodes are collapsed or expanded then can we log some output like Children are valid or invalid.
Otherwise I have tested the fix with swingset2 demo and the test mentioned in JDK-8320664 which behaves fine with the fix.

kumarabhi006 · 2024-05-21T08:47:04Z

src/java.desktop/macosx/native/libawt_lwawt/awt/a11y/OutlineAccessibility.m

+
+- (nullable NSArray<id<NSAccessibilityRow>> *)accessibilityChildren
+{
+ if (![self isCacheValid]) {


Why can't we use rowCacheValid value to check like it is done for accessibilitySelectedChildren method?

Again - to cover the situation where expand/collapse events were delivered to the parent node. Or to the parent's parent.

…no effect but it is more uiform and according to the ObjectibeC convention where objects are referred as nil and references to the primitives as NULL.

kumarabhi006

Looks good to me.

savoptik · 2024-05-23T12:45:31Z

src/java.desktop/macosx/native/libawt_lwawt/awt/a11y/OutlineAccessibility.m

+ [self invalidateSelectionCache];
+ [super postSelectedCellsChanged];
+}
+


It seems to me that it is also worth defining a destructor that will destroy the cache before destroying the instance.

The problem is that on Java side we are notified when the accessibility is being enabled but we have no notification of when it is no longer used so we do not deliberately release native peers and in my testing even when i add dealloc function to the class it is never being called.

savoptik

Ok.

prsadhuk

I hope you have ran the whole gamut of jtreg tests, if not, please do it before integrating and SwingSet2 too..

azuev-java · 2024-05-25T00:34:36Z

Of course i did all the tests - just to be on the safe side i rerun all of available tests on the latest changeset and got 100% pass rate.

azuev-java · 2024-05-25T00:34:45Z

/integrate

openjdk · 2024-05-25T00:35:57Z

Going to push as commit 05f13e7.
Since your change was applied there have been 196 commits pushed to the master branch:

7bf1989: 8320575: generic type information lost on mandated parameters of record's compact constructors
253508b: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
ebc520e: 8332841: GenShen: Pull shared members from control thread into common base class
236432d: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook
b3b3366: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
f66a586: 8332641: Update nsk.share.jpda.Jdb to don't use finalization
cd3e4c0: 8326734: text-decoration applied to lost when mixed with or

c2cca2a: 8330647: Two CDS tests fail with -UseCompressedOops and UseSerialGC/UseParallelGC

6d2aeb8: 8332745: Method::is_vanilla_constructor is never used

cfdc64f: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations

... and 186 more: https://git.openjdk.org/jdk/compare/2d622152b07bba0aba8fd5b1e24293e28d6e69f5...master

~~Your commit was automatically rebased without conflicts.~~

openjdk · 2024-05-25T00:36:03Z

@azuev-java Pushed as commit 05f13e7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label May 16, 2024

openjdk bot added the client [email protected] label May 16, 2024

savoptik reviewed May 18, 2024

View reviewed changes

- Removed the unused import in CAccessibility.java

1703333

- Rolled back changes to the CommonComponentAccessibility and added relevant post* methods overrides to the OutlineAccessibility.

kumarabhi006 reviewed May 20, 2024

View reviewed changes

savoptik reviewed May 20, 2024

View reviewed changes

- Remove unused import;

6d6cd81

savoptik approved these changes May 20, 2024

View reviewed changes

kumarabhi006 reviewed May 21, 2024

View reviewed changes

Replaced NULL with nil in OutlineAccessibility - functionally it has …

4836a92

…no effect but it is more uiform and according to the ObjectibeC convention where objects are referred as nil and references to the primitives as NULL.

kumarabhi006 approved these changes May 23, 2024

View reviewed changes

savoptik reviewed May 23, 2024

View reviewed changes

savoptik approved these changes May 24, 2024

View reviewed changes

prsadhuk approved these changes May 24, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 24, 2024

openjdk bot added the integrated Pull request has been integrated label May 25, 2024

openjdk bot closed this May 25, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329667: [macos] Issue with JTree related fix for JDK-8317771 #19255

8329667: [macos] Issue with JTree related fix for JDK-8317771 #19255

azuev-java commented May 16, 2024 •

edited by openjdk bot

bridgekeeper bot commented May 16, 2024

openjdk bot commented May 16, 2024 •

edited

openjdk bot commented May 16, 2024

mlbridge bot commented May 16, 2024 •

edited

victordyakov commented May 16, 2024 •

edited

savoptik May 18, 2024

azuev-java May 19, 2024

savoptik May 18, 2024

azuev-java May 19, 2024

kumarabhi006 May 17, 2024

azuev-java May 20, 2024

savoptik May 20, 2024

azuev-java May 20, 2024

savoptik left a comment

kumarabhi006 May 21, 2024

azuev-java May 22, 2024

kumarabhi006 May 22, 2024

azuev-java May 23, 2024

kumarabhi006 May 23, 2024

kumarabhi006 May 21, 2024

azuev-java May 22, 2024

kumarabhi006 left a comment

savoptik May 23, 2024

azuev-java May 24, 2024

savoptik left a comment

prsadhuk left a comment

azuev-java commented May 25, 2024

azuev-java commented May 25, 2024

openjdk bot commented May 25, 2024

openjdk bot commented May 25, 2024

8329667: [macos] Issue with JTree related fix for JDK-8317771 #19255

8329667: [macos] Issue with JTree related fix for JDK-8317771 #19255

Conversation

azuev-java commented May 16, 2024 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 16, 2024

openjdk bot commented May 16, 2024 • edited

openjdk bot commented May 16, 2024

mlbridge bot commented May 16, 2024 • edited

Webrevs

victordyakov commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savoptik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarabhi006 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savoptik left a comment

Choose a reason for hiding this comment

prsadhuk left a comment

Choose a reason for hiding this comment

azuev-java commented May 25, 2024

azuev-java commented May 25, 2024

openjdk bot commented May 25, 2024

openjdk bot commented May 25, 2024

azuev-java commented May 16, 2024 •

edited by openjdk bot

openjdk bot commented May 16, 2024 •

edited

mlbridge bot commented May 16, 2024 •

edited

victordyakov commented May 16, 2024 •

edited