8277922: Unable to click JCheckBox in JTable through Java Access Bridge #7416
Conversation
|
👋 Welcome back alitvinov! A progress list of the required criteria for merging this PR into |
Webrevs
|
| } | ||
|
|
||
| private void createGUI() { | ||
| if (!(UIManager.getLookAndFeel() instanceof MetalLookAndFeel)) { |
There was a problem hiding this comment.
Why the metal L&F is tested, should at least any default L&F works(like Aqua)?
There was a problem hiding this comment.
Hi Sergey! Thank you for review of this fix, I am glad to hear from you. Metal L&F is not related anyhow to the bug or the tested scenarios, it is not required for the test at all. I used Metal L&F just to make sure that UI of the test will be shown in generic way on all OS families on which it will be run, what will decrease possibility that it may fail because of other unrelated bug connected to some not very well supported L&F. Yes, of course, the test should work with any other L&F including Aqua, but I would like to emphasize that this bug is about MS Windows OS and requires assistive software native application to call "AccessibleAction" of "JTable" cell through Java Access Bridge API. If you insist I can remove this code which sets Metal L&F from the test.
| return accessibleContext; | ||
| } | ||
|
|
||
| protected class AccessibleBooleanRenderer |
There was a problem hiding this comment.
I guess the doAccessibleAction(i); should work in a similar way as something like "table,getCellJComponent()".click(). Is it actually possible to click on the cell w/o using robot and w/o a11y API just via Swing API and w/o adding code for each type of the renderer?
There was a problem hiding this comment.
I do not see the method "getCellJComponent" in JTable or anything like this, but what I see in the code is, yes, call to the method "doAccessibleAction(0)" of accessible action of accessible context of "JCheckBox" just calls "doClick()" method of "JCheckBox". Yes, sure, "doClick()" is the public method of any "javax.swing.AbstractButton" and can be called from anywhere having a reference to the instance of "javax.swing.AbstractButton".
In the fix I added code to this particular renderer "JTable.BooleanRenderer" in order to be able to intercept every invocation of "boolean doAccessibleAction(int i)" method of its default "AccessibleAction" implementation which is "JCheckBox.AccessibleJCheckBox" and to be able to change a boolean value of a cell in "JTable".
I am not going to add any similar code to other types of default table cell renderers, because first of all there are not many of them: "DefaultTableCellRenderer.UIResource", "NumberRenderer", "DoubleRenderer", "DateRenderer", "IconRenderer", "BooleanRenderer", they are used in the method "JTable.createDefaultRenderers()", and all of these renderers except for "BooleanRenderer" are derivatives of "JLabel" component which reports 0 supported accessible actions, so any default renderer except of "BooleanRenderer" does not support any accessible action. And the problem is that "BooleanRenderer" is derivative of "JCheckBox" and therefore it reports to the user through Java Access Bridge API, through Java Accessibility API that it supports 1 action called "click".
There was a problem hiding this comment.
Is it actually possible to click on the cell w/o using robot and w/o a11y API just via Swing API and w/o adding code for each type of the renderer?
No, it's not possible because there's no real component in each cell. The table draws the checkbox using TableCellRenderer, by default it's BooleanRenderer which extends JCheckBox.
| if ((oldSelectedState != newSelectedState) && | ||
| (table != null) && table.isEnabled() && | ||
| table.isCellEditable(row, column)) { | ||
| table.setValueAt(Boolean.valueOf(newSelectedState), |
There was a problem hiding this comment.
Where this code is triggered if the user click on the checkbox by the mouse?
There was a problem hiding this comment.
Mouse click on JCheckBox is handled differently from execution of "click" accessible action on this JCheckBox, because behind mouse click different AWT mouse events stand which are processed properly, while "doAccessibleAction()" does not involve these mouse events and all their code. So if you are worrying about focus or selection as a result of "doAccessibleAction()", then I checked the behavior of "doAccessibleAction" on JCheckBox, JButton located outside JTable and I confirm that it does not lead to setting of focus to these components, hence there is no need to set focus to this "JCheckBox" "BooleanRenderer" when it is in table cell and no need to change selection in JTable.
I suppose that this code should be invoked only by calling "doAccessibleAction(int)" on object of "AccessibleAction" interface either through Java Accessibility API like it is in this regression test or through Java Access Bridge API from the native assistive software application like it is in the manual test attached to the bug and in that case it is called from the method "private boolean doAccessibleActions(final AccessibleContext ac, final String name)" in the file "src/jdk.accessibility/windows/classes/com/sun/java/accessibility/internal/AccessBridge.java".
There was a problem hiding this comment.
The reason of the issue is the fact that when the assistive technology software tries to do "AccessibleAction" on "AccessibleContext" associated with a cell with boolean data type in "JTable" component through Java Access Bridge (JAB), the JDK executes this "AccessibleAction" on "AccessibleContext" of a renderer, which is an instance of the class "javax.swing.JTable.BooleanRenderer" which is a derivative of "JCheckBox" class, and the instance of this renderer is single and common for all cells of boolean data type. Therefore execution of "click" "AccessibleAction" on this renderer component which is not permanently bound to any particular cell in the table does not lead to update of the table cell value.
But how do the real mouse clicks trigger selection/deselection of the checkbox state, I guess that the "table.setValueAt()" should be called somewhere, can we do something similar from the "doAccessibleAction()"?
There was a problem hiding this comment.
Hi Sergey. When real mouse click happens "MOUSE_PRESSED", "MOUSE_RELEASED" AWT events are generated and properly handled by code in "JTable" and in other related to "JTable" classes, for example "javax.swing.plaf.basic.BasicTableUI". In the process of handling "JTable" switches to the editing mode and starts considering that at this moment the mouse events are related to not "BooleanRenderer" but to the editor component "BooleanEditor". All these focus/selection/deselection happens during processing of the "MOUSE_PRESSED" event in the methods:
- "javax.swing.plaf.basic.BasicTableUI.Handler.mousePressed(MouseEvent e)"
- "javax.swing.plaf.basic.BasicTableUI.Handler.adjustSelection(MouseEvent e)"
- "javax.swing.JTable.editCellAt(int row, int column, EventObject e)"
- "javax.swing.JTable.changeSelection(int rowIndex, int columnIndex, boolean toggle, boolean extend)"
So real mouse click is not a simple call of some one generic click method, which does everything.
My position is that "selection/deselection" should not happen as a result of "doAccessibleAction(int)" on "AccessibleAction" of "BooleanRenderer". "selection/deselection" must also include setting of focus to the edited cell. Regarding focus I repeat what I said in my prior comment above:
I checked the behavior of "doAccessibleAction" on JCheckBox, JButton located outside JTable and I confirm that it does not lead to setting of focus to these components, hence there is no need to set focus to this "JCheckBox" "BooleanRenderer" when it is in table cell and no need to change selection in JTable.
I do not see the need in imitating calls to the methods of "BasicTableUI.Handler", "JTable", which are listed upper and involved in real mouse clicks, in handling of "doAccessibleAction". To my mind execution of accessible action "click" in any "JToggleButton" including "JCheckBox" IS NOT EQUAL to real mouse click on them and therefore there is no need to aim to make "doAccessibleAction" on "BooleanRenderer" to be equal to real mouse click on it.
There was a problem hiding this comment.
I agree we shouldn't try to repeat the same actions that are caused by mouse click. It aligns with the behaviour of a stand-alone checkbox.
| } catch (InterruptedException | InvocationTargetException | | ||
| AWTException e) { | ||
| throw new RuntimeException(e); | ||
| } finally { |
There was a problem hiding this comment.
You can declare main to throw Exception or this specific list of exceptions and drop catch-block: jtreg catches all types of exceptions and it's a failure.
There was a problem hiding this comment.
Hi Alexey. I agree in this implementation of the "main" method of the test catching these exceptions is unnecessary. This remark is fully addressed in the second version of the fix.
| return accessibleContext; | ||
| } | ||
|
|
||
| protected class AccessibleBooleanRenderer |
There was a problem hiding this comment.
I think this class, AccessibleBooleanRenderer as well as setCellToDoActionOn method can have the default access. They're inside package-private class BooleanRenderer, I see no reason why these should have higher access level.
There was a problem hiding this comment.
Agree, higher access level is not needed in the places specified by you. In the second version of the fix your remark is fully addressed.
|
The second version of the fix for this bug is created and added to this Pull Request. Please review the new version of the fix. In the second version of the fix the next changes were introduced: In the file "src/java.desktop/share/classes/javax/swing/JTable.java": In the file "test/jdk/javax/accessibility/JTable/JCheckBoxInJTableCannotBeClickedTest.java": |
|
Hello Alexey @aivanov-jdk and Sergey @mrserb, The new third version of the fix is created and pushed to the branch of this pull request. The third version of the fix is completely alternative and guaranteed way of resolution of this bug, because of this I would recommend to look at its Full Webrev to simplify understanding of the fix before looking at the exact third commit in GitHub interface. Could you please review the third version of the fix? In the third fix version I refused from the whole idea to implement the functionality accomplishing click action in BooleanRenderer component, and instead of this decided just to prohibit users to execute any accessible actions on AccessibleContext of BooleanRenderer and to put a point in this controversial bug. Among 6 default table cell renderers in JDK:
BooleanRenderer was the only component, whose AccessibleContext had non-null AccessibleAction, however as we see in all research in this bug and this review request, execution of this AccessibleAction of BoleanRenderer did not have any opportunity to change internal states and visual states of the corresponding table cell, so it just never worked. Also Java Access Bridge API for assistive technology software in C++/C, provides only functions to read states and data from accessible table object and does not provide functions for modification of accessible table, therefore implementation of AccessibleAction for BooleanRenderer would create unwanted contradiction in both Java Access Bridge API and Java Accessibility API. Also recent offline feedback from the bug filer proved that the original approach in 1st and 2nd fix versions was not reliable and proper way of resolution of the bug. Because the main conceptual problem standing behind this bug is not resolvable reliably in current design of Java Access Bridge, which is the fact that 1 or more table cells with Boolean data in them are all associated with a single instance of BooleanRenderer by design of Java Access Bridge, and during execution of "doAccessibleAction(int)" of AccessibleAction of BooleanRenderer the BooleanRenderer HAS NO ANY MEANS to reliably know for which exact cell the user intended to execute the AccessibleAction. Currently each JTable cell has own AccessibleContext object which is instance of "JTable.AccessibleJTable.AccessibleJTableCell" and which internally calls API of AccessibleContext of renderer, but at the same time there is always an opportunity for the end user to get AccessibleContext of the renderer directly bypassing API of AccessibleJTable and AccessibleJTableCell, it can be as simple as this Therefore I am convinced that the only solid way to resolve this bug is to stop reporting to the end users that "AccessibleContext" of "BolleanRenderer" supports "AccessibleAction", what has never been supported in reality. Thank you, |
|
In addition to my previous comment. The regression test in the third fix version is completely rewritten and now tests scenarios corresponding to the fix and which are different from scenarios in the regression test of the first and the second fix versions. |
There was a problem hiding this comment.
The new approach by returning no accessible action looks cleaner. It resolves the ambiguity where the user of the API could get the accessible context of the cell renderer directly, in which case the accessible action cannot be performed because there's no way for the renderer to determine to which cell the action applies.
| } | ||
|
|
||
| private void testAccessibleActionInCellRenderer(int row, int column, | ||
| boolean shouldBeNull) { |
There was a problem hiding this comment.
Since shouldBeNull is always true, I propose to drop it.
There was a problem hiding this comment.
In my opinion this argument does not create any troubles for the test but makes real goals achieved by the method "testAccessibleActionInCellRenderer" more clear and makes this test method more generic. I do not want to remove it. If the goal is to optimize the code, then everything in this test can be rearranged, rewritten, I am not trying to achieve the goal to write absolutely minimal test case.
| if ((shouldBeNull && (cellRendererAa != null)) || | ||
| (!shouldBeNull && (cellRendererAa == null))) { |
There was a problem hiding this comment.
If shouldBeNull is dropped from parameters, this condition can be simplified to
| if ((shouldBeNull && (cellRendererAa != null)) || | |
| (!shouldBeNull && (cellRendererAa == null))) { | |
| if (cellRendererAa != null)) { |
And the error message below should say 'cellRendererAa' is not null.
There was a problem hiding this comment.
Alexey, I refuse to make the forth version of the fix just to remove this innocent "shouldBeNull" argument. Information in the current error message "Test failed. 'cellRendererAa' is not as should be" is enough to understand what is the reason of the failure. To understand what value was expected by the failing test scenario, it is needed just to read in "System.out" the output of the code
143 System.out.println(String.format(
144 "testAccessibleActionInCellRenderer():" +
145 " row='%d', column='%d', shouldBeNull='%b'",
146 row, column, shouldBeNull));
|
|
||
| private void testAccessibleActionInCellRenderer(int row, int column, | ||
| boolean shouldBeNull) { | ||
| System.out.println(String.format( |
There was a problem hiding this comment.
I suggest using System.out.printf, just add "\n" to the end of format string.
There was a problem hiding this comment.
I am not against that somebody prefers "System.out.printf", let they use it, if they want, but I personally like to use "System.out.println" and "System.out.print" and have full right to use it.
| if (ac != null) { | ||
| return ac.getAccessibleAction(); | ||
| } |
There was a problem hiding this comment.
This is the fix for possible NPE, right?
There was a problem hiding this comment.
Yes, correct, this "if" condition is a fix for possible NPE, the method "javax.swing.JTable.AccessibleJTable.AccessibleJTableCell.getCurrentAccessibleContext()" may return "null" according to its documentation and to its code. I already took into account this possible NPE in the 1st and 2nd versions of the fix for this bug, and saw it amoral not to take it into account in the 3rd version of the fix, therefore I added this "if (ac != null) {" block in the 3rd fix version.
| private volatile JFrame frame; | ||
| private volatile JTable table; |
There was a problem hiding this comment.
Neither frame nor table are accessed from any other thread but EDT, I believe volatile isn't necessary in this case.
There was a problem hiding this comment.
I am not going to remove these "volatile" modifiers. These two variables are initialized and assigned "null" values on the Main thread in "main" method and then other values are assigned to them as well as they are accessed from EDT. Hence we got two threads accessing these two references, therefore these references must be thread-safe from my view point. I do not want to start again our endless arguing on this subject, which we periodically had before, in this review request and about variables in the regression test, especially when you are suggesting to decrease thread-safety.
| import javax.swing.table.DefaultTableModel; | ||
| import javax.swing.table.TableCellRenderer; | ||
|
|
||
| public class BooleanRendererHasAccessibleActionTest { |
There was a problem hiding this comment.
Would BooleanRendererHasNoAccessibleActionTest be a better name? After all, you test for no accessible action.
There was a problem hiding this comment.
For almost 10 years of fixing bugs in JDK, I have always given names to the regression tests to describe exactly the failing test scenario, rather then the expected and not failing behavior. So for me the test name "BooleanRendererHasNoAccessibleActionTest" would mean that the test should fail when BooleanRenderer does not have "AccessibleAction", and this is the opposite from what the bug and what I am fixing by the 3rd fix version. I am not going to change the test name.
|
@alitvinv 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 319 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 |
|
/integrate |
|
Going to push as commit 1668c02.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Hello,
Could you please review the following fix for the bug. The bug consists in the fact that When an assistive technology software by means of Java Access Bridge API executes "AccessibleAction" "click" on "AccessibleContext" which corresponds to "javax.swing.JTable" cell containing "javax.swing.JCheckBox", then the cell's value and cell's view represented by "JCheckBox" stay unchanged. The issue is a bug in JDK and should be fixed in JDK, because JDK informs the assistive technology software through Java Access Bridge API in particular through the function
"BOOL getAccessibleActions(long vmID, AccessibleContext accessibleContext, AccessibleActions *actions)"
that "AccessibleContext" of the table cell with "JCheckBox" supports one action "click", while real execution of this action on this accessible context does not lead to any result.
THE ROOT CAUSE OF THE BUG:
The reason of the issue is the fact that when the assistive technology software tries to do "AccessibleAction" on "AccessibleContext" associated with a cell with boolean data type in "JTable" component through Java Access Bridge (JAB), the JDK executes this "AccessibleAction" on "AccessibleContext" of a renderer, which is an instance of the class "javax.swing.JTable.BooleanRenderer" which is a derivative of "JCheckBox" class, and the instance of this renderer is single and common for all cells of boolean data type. Therefore execution of "click" "AccessibleAction" on this renderer component which is not permanently bound to any particular cell in the table does not lead to update of the table cell value.
THE FIX:
The fix implements an approach which guarantees setting of new values to the table's cells with boolean data type on each execution of "AccessibleAction" of "javax.swing.JTable.BooleanRenderer" instance, when execution of this action changes the "selected" state of this "BooleanRenderer" JCheckBox component.
Please take into account that the created automatic regression test simulates the issue only with Java Accessibility API what is not fully equal to the original test scenario which requires the assistive technology software and usage of Java Access Bridge API and which can be tested using the manual test case attached to the issue in JBS. However the regression test still allows to reproduce the issue and verify that the fix resolves it.
Thank you,
Anton
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7416/head:pull/7416$ git checkout pull/7416Update a local copy of the PR:
$ git checkout pull/7416$ git pull https://git.openjdk.java.net/jdk pull/7416/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7416View PR using the GUI difftool:
$ git pr show -t 7416Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7416.diff