JBR-4478 Implement support for native accessible caret events on Windows #456

dmitrii-drobotov · 2024-09-03T17:47:34Z

The feature adds caret tracking support for assistive tools that don't work with Java Access Bridge, specifically, for the built-in Windows Magnifier. Caret tracking will be supported on a similar level to the macOS one added in #255. Here's a quick demo: https://github.com/user-attachments/assets/9c0ef675-364c-40ba-94af-63f98e6809aa

In short, it works by implementing Win32 IAccessible interface for the text caret and sending EVENT_OBJECT_LOCATIONCHANGE events whenever it changes, which is very close to the way Windows system caret behaves from the accessibility side. It's also somewhat similar to the way it's implemented in chromium, and to XInputMethod.ClientComponentCaretPositionTracker.

For the proper Magnifier support, we would need to implement UI Automation API, which is a modern accessibility API on Windows. I initially started implementing it, but it turned out to be quite complex, so I decided to go with this approach of implementing IAccessible API (which is legacy, but still well supported) only for the text caret.

The feature is enabled by default and can be disabled by setting sun.awt.windows.use.native.caret.accessibility.events property to false.

dmitrii-drobotov · 2024-09-03T17:53:42Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.cpp

+    // This is a workaround to make sure the foreground window is set to the actual focused window.
+    // Otherwise, in some cases, e.g., when opening a popup,
+    // the root frame can still stay as the foreground window instead of the popup,
+    // and Magnifier will be focused on it instead of the popup.
+    // We only do it if the caret object is actually used to minimize risks.
+    if (AccessibleCaret::isCaretUsed && ::GetForegroundWindow() != hwnd) {
+        ::SetForegroundWindow(hwnd);
+    }


Just to clarify, this is how opening and closing popups work with this workaround: https://github.com/user-attachments/assets/a99211ec-41b8-4b60-bc2f-8a675a806d09, and without: https://github.com/user-attachments/assets/b62a9c16-f6a0-4fde-a5e7-f8b1d4dc7b31.
The current behavior for popups is the way it works for JFrames and JDialogs (e.g., Find in Files dialog) even without any changes, and now will also apply to popups. But it is somewhat a regression, because this is how popups worked without this PR: https://github.com/user-attachments/assets/fe8fa24e-e13c-4461-9408-75748fc81be7, so no viewport jumping at all. This is apparently affected by the presense of an accessible object that magnifier interacted with in a window, and a proper fix would probably require implementing hierarchy (parent and child methods of IAccessible) and location of accessible objects. I think we can keep it this way for now, and maybe improve later, because I don't think it's a major problem, and can also be completely avoided by turning off keyboard focus tracking in Magnifier settings.

In general, I don't feel like touching the focus subsystem here because it's a really fragile system. WDYT if we drop this part of the patch for now and will investigate under a separate ticket later how to fix it? Especially if taking into account, that this part is causing IJPL-162581.

Secondly, have you tested how it works after the magnifier gets closed? If I understand the current revision correctly, SetForegroundWindow will be called even after the magnifier is closed because isUsed isn't reset to false anywhere.

And finally, maybe we should just play with NotifyWinEvent here instead? Like to force the magnifier to re-ask all the information.

src/java.desktop/windows/classes/sun/awt/windows/AccessibleCaretLocationNotifier.java

The feature adds caret tracking support for assistive tools that don't work with Java Access Bridge, specifically, for the built-in Windows Magnifier. It works by implementing Win32 IAccessible interface for the text caret, and sending EVENT_OBJECT_LOCATIONCHANGE events whenever it changes. It's enabled by default and can be disabled by setting `sun.awt.windows.use.native.caret.accessibility.events` property to false.

NikitkoCent

Thank you for the patch! I have several topics to discuss

NikitkoCent · 2024-10-04T12:18:55Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.h

@@ -0,0 +1,80 @@
+/*
+* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.


Suggested change

* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.

* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.

NikitkoCent · 2024-10-04T12:19:05Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.cpp

@@ -0,0 +1,270 @@
+/*
+* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.


Suggested change

* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.

* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.

NikitkoCent · 2024-10-04T12:43:08Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.cpp

+    : m_refCount(1), m_x(0), m_y(0), m_width(0), m_height(0) {
+}
+
+AccessibleCaret *AccessibleCaret::instance = NULL;


Let's use nullptr instead of NULL since we're using C++ :)

NikitkoCent · 2024-10-04T13:15:26Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.h

+
+class AccessibleCaret : public IAccessible {
+public:
+    AccessibleCaret();


From the responsibilities point of view, the Release method now controls how exactly objects of this class should be destroyed, and there appears a little asymmetry - the class controls how to destroy its objects but doesn't have control over objects creation. In practice it also would lead to UB if someone created an object of the class not via the new expression and then invoked the Release method.
Thus I propose to get the class control over object creation via the following steps:

Make all the constructors private or protected ;

Instead, provide a public fabric method for objects creation ;

Make the destructor protected or private so that the only way to destroy an object is to call the Release() method.

NikitkoCent · 2024-10-04T13:23:15Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.h

+    static AccessibleCaret *instance;
+    static bool isCaretUsed;


If the class actually represents a singleton, let's restrict the access to its state via additional methods to avoid misusages in the future

Also, as I see, the state is read and/or written from different threads (the Toolkit and Swing), so the changes to it have to be somehow guarded. Especially when the instance pointer gets destroyed and then set to nullptr.
I'd first try to design and implement API in a lock-free way (like on atomics), not sure if it's possible though.

The same for read/writes accesses to m_x, m_y, m_width, m_height.

NikitkoCent · 2024-10-04T14:40:25Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.cpp

+        return E_POINTER;
+    }
+    *ppdispParent = NULL;
+    return S_OK;


Maybe

Suggested change

return S_OK;

return S_FALSE;

?

https://learn.microsoft.com/en-us/windows/win32/api/oleacc/nf-oleacc-iaccessible-get_accparent#return-value

NikitkoCent · 2024-10-04T14:57:11Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.cpp

+    }
+
+    pvarState->vt = VT_I4;
+    pvarState->lVal = 0; // The state without any flags, corresponds to "normal".


According to https://learn.microsoft.com/en-us/windows/win32/winauto/caret, there also may be the state STATE_SYSTEM_INVISIBLE. I think it happens when the keyboard focus belongs to a non-text component (e.g. a button) or e.g. to a non-editable text component

NikitkoCent · 2024-10-04T15:18:19Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.cpp

+IFACEMETHODIMP_(ULONG) AccessibleCaret::Release() {
+    ULONG count = InterlockedDecrement(&m_refCount);
+    if (count == 0) {
+        delete this;


Shouldn't we add isCaretUsed = false; to here (or to the destructor)?

NikitkoCent · 2024-10-04T16:21:51Z

src/java.desktop/windows/native/libawt/windows/AccessibleCaret.cpp

+    // This is a workaround to make sure the foreground window is set to the actual focused window.
+    // Otherwise, in some cases, e.g., when opening a popup,
+    // the root frame can still stay as the foreground window instead of the popup,
+    // and Magnifier will be focused on it instead of the popup.
+    // We only do it if the caret object is actually used to minimize risks.
+    if (AccessibleCaret::isCaretUsed && ::GetForegroundWindow() != hwnd) {
+        ::SetForegroundWindow(hwnd);
+    }


In general, I don't feel like touching the focus subsystem here because it's a really fragile system. WDYT if we drop this part of the patch for now and will investigate under a separate ticket later how to fix it? Especially if taking into account, that this part is causing IJPL-162581.

Secondly, have you tested how it works after the magnifier gets closed? If I understand the current revision correctly, SetForegroundWindow will be called even after the magnifier is closed because isUsed isn't reset to false anywhere.

And finally, maybe we should just play with NotifyWinEvent here instead? Like to force the magnifier to re-ask all the information.

NikitkoCent · 2024-10-05T12:27:21Z

src/java.desktop/windows/classes/sun/awt/windows/AccessibleCaretLocationNotifier.java

+        if (nativeCaretEventsEnabled && caretNotifier == null) {
+            SwingUtilities.invokeLater(() -> {
+                caretNotifier = new AccessibleCaretLocationNotifier(hwnd);


Since caretNotifier is read and written from different threads, we should:

Have memory barriers around the access operations

Add additional check that caretNotifier is still null on the EDT

dmitrii-drobotov requested a review from NikitkoCent September 3, 2024 17:47

dmitrii-drobotov self-assigned this Sep 3, 2024

dmitrii-drobotov commented Sep 3, 2024

View reviewed changes

dmitrii-drobotov force-pushed the ddrobotov/JBR-4478 branch from 8131fb5 to c647762 Compare September 11, 2024 16:23

dmitrii-drobotov commented Sep 11, 2024

View reviewed changes

src/java.desktop/windows/classes/sun/awt/windows/AccessibleCaretLocationNotifier.java Show resolved Hide resolved

dmitrii-drobotov force-pushed the ddrobotov/JBR-4478 branch 2 times, most recently from f386b63 to c46b34a Compare September 13, 2024 14:29

NikitkoCent requested changes Oct 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JBR-4478 Implement support for native accessible caret events on Windows #456

JBR-4478 Implement support for native accessible caret events on Windows #456

dmitrii-drobotov commented Sep 3, 2024

dmitrii-drobotov Sep 3, 2024

NikitkoCent Oct 4, 2024 •

edited

Loading

NikitkoCent left a comment

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024

NikitkoCent Oct 4, 2024 •

edited

Loading

NikitkoCent Oct 5, 2024

		@@ -0,0 +1,80 @@
		/*
		* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.

		@@ -0,0 +1,270 @@
		/*
		* Copyright (c) 2024, JetBrains s.r.o.. All rights reserved.

JBR-4478 Implement support for native accessible caret events on Windows #456

Are you sure you want to change the base?

JBR-4478 Implement support for native accessible caret events on Windows #456

Conversation

dmitrii-drobotov commented Sep 3, 2024

Choose a reason for hiding this comment

NikitkoCent Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

NikitkoCent 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikitkoCent Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikitkoCent Oct 4, 2024 •

edited

Loading

NikitkoCent Oct 4, 2024 •

edited

Loading