-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8341311: [Accessibility,macOS,VoiceOver] VoiceOver announces incorrect number of items in submenu of JPopupMenu #25470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved. | ||
| * Copyright (c) 2021, 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
|
|
@@ -24,6 +24,10 @@ | |
| */ | ||
|
|
||
| #import "MenuAccessibility.h" | ||
| #import "ThreadUtilities.h" | ||
| #import "sun_lwawt_macosx_CAccessibility.h" | ||
|
|
||
| static jclass sjc_CAccessibility = NULL; | ||
|
|
||
| /* | ||
| * Implementing a protocol that represents menus both as submenu and as a | ||
|
|
@@ -51,4 +55,31 @@ - (id _Nullable)accessibilityValue | |
| return NULL; | ||
| } | ||
|
|
||
| /* | ||
| * Return all non-ignored children. | ||
| */ | ||
| - (NSArray *)accessibilityChildren { | ||
| JNIEnv *env = [ThreadUtilities getJNIEnv]; | ||
| GET_CACCESSIBILITY_CLASS_RETURN(nil); | ||
| DECLARE_STATIC_METHOD_RETURN(sjm_getCurrentAccessiblePopupMenu, sjc_CAccessibility, | ||
| "getCurrentAccessiblePopupMenu", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won’t it be possible to get the correct object like this, as it’s done in jobject currentAccessible = [self currentAccessibleWithENV:env];
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to utilize this method but |
||
| "(Ljavax/accessibility/Accessible;Ljava/awt/Component;)Ljavax/accessibility/Accessible;", nil); | ||
| jobject axComponent = (*env)->CallStaticObjectMethod(env, sjc_CAccessibility, | ||
| sjm_getCurrentAccessiblePopupMenu, | ||
| fAccessible, fComponent); | ||
|
|
||
| CommonComponentAccessibility *currentElement = [CommonComponentAccessibility createWithAccessible:axComponent | ||
| withEnv:env withView:self->fView isCurrent:YES]; | ||
|
|
||
| NSArray *children = [CommonComponentAccessibility childrenOfParent:currentElement | ||
| withEnv:env | ||
| withChildrenCode:sun_lwawt_macosx_CAccessibility_JAVA_AX_ALL_CHILDREN | ||
| allowIgnored:NO]; | ||
|
|
||
| if ([children count] == 0) { | ||
| return nil; | ||
| } else { | ||
| return children; | ||
| } | ||
| } | ||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| import java.awt.event.MouseAdapter; | ||
| import java.awt.event.MouseEvent; | ||
|
|
||
| import javax.swing.JFrame; | ||
| import javax.swing.JMenu; | ||
| import javax.swing.JMenuItem; | ||
| import javax.swing.JPopupMenu; | ||
|
|
||
| /* | ||
| * @test | ||
| * @bug 8341311 | ||
| * @summary Verifies that VoiceOver announces correct number of child for PopupMenu on macOS | ||
| * @requires os.family == "mac" | ||
| * @library /java/awt/regtesthelpers | ||
| * @build PassFailJFrame | ||
| * @run main/manual TestPopupMenuChildCount | ||
| */ | ||
|
|
||
| public class TestPopupMenuChildCount { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: as far as I understand, this is for macOS only. Do you think it would make more sense to have a name mention that the test is Mac only? The same for test description. Seems to me a bit confusing otherwise It's fine with me if you leave it as it is though.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is only for macOS and that is conveyed through the jtreg tag here. I don't think test name needs to be changed. Although it is not essentially required but will update the test summary and instruction.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It seems to me that this test should be generalized for all a11y platforms, since it is a regression test and can help identify regression of this functionality on other platforms.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The behaviour on Windows with a11y clients (JAWS or NVDA) is different. They don't announce the child count for the menu and submenus for java application and it is similar to the native application (notepad++) too. So, the regression test restricted for macOS seems suitable for now. |
||
| public static void main(String[] args) throws Exception { | ||
| String INSTRUCTIONS = """ | ||
| This test is applicable only on macOS. | ||
|
|
||
| Test UI contains an empty JFrame. On press of left/right mouse button, | ||
| a PopupMenu will be visible. | ||
|
|
||
| Follow these steps to test the behaviour: | ||
|
|
||
| 1. Start the VoiceOver (Press Command + F5) application | ||
| 2. Press Left/Right mouse button inside test frame window to open | ||
| the PopupMenu | ||
| 3. VO should announce "Menu" with number of child items of the Popupmenu | ||
| 4. Press Up/Down arrow to traverse popupmenu child items | ||
| 5. Press Right arrow key to open submenu | ||
| 6. VO should announce "Menu" with correct number of child items | ||
| for the submenu (For e.g. When Submenu-1 is open, VO should announce | ||
| "Menu 4 items") | ||
| 7. Repeat the process for other submenus | ||
| 8. Press Pass if you are able to hear correct announcements | ||
| else Fail"""; | ||
|
|
||
| PassFailJFrame.builder() | ||
| .instructions(INSTRUCTIONS) | ||
| .columns(45) | ||
| .testUI(TestPopupMenuChildCount::createUI) | ||
| .build() | ||
| .awaitAndCheck(); | ||
| } | ||
|
|
||
| private static JFrame createUI() { | ||
| JFrame frame = new JFrame("Test Frame"); | ||
|
|
||
| JPopupMenu popupmenu = new JPopupMenu(); | ||
| JMenuItem mi1 = new JMenuItem("MenuItem-1"); | ||
| JMenuItem mi2 = new JMenuItem("MenuItem-2"); | ||
| JMenuItem mi3 = new JMenuItem("MenuItem-3"); | ||
| popupmenu.add(mi1); | ||
| popupmenu.add(mi2); | ||
| popupmenu.add(mi3); | ||
|
|
||
| JMenu submenu1 = new JMenu("Submenu-1"); | ||
| submenu1.add("subOne"); | ||
| submenu1.add("subTwo"); | ||
| submenu1.add("subThree"); | ||
|
|
||
| JMenu submenu2 = new JMenu("Submenu-2"); | ||
| submenu2.add("subOne"); | ||
| submenu2.add("subTwo"); | ||
|
|
||
| JMenu submenu3 = new JMenu ("Submenu-3"); | ||
| submenu3.add("subOne"); | ||
| submenu1.add(submenu3); | ||
|
|
||
| popupmenu.add(submenu1); | ||
| popupmenu.add(submenu2); | ||
|
|
||
| frame.addMouseListener(new MouseAdapter() { | ||
| @Override | ||
| public void mouseClicked(MouseEvent e) { | ||
| popupmenu.show(e.getComponent(), e.getX(), e.getY()); | ||
| } | ||
| }); | ||
| frame.setSize(300, 300); | ||
| return frame; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest replacing for with while. If the desired submenu is at the beginning of the list, there’s no need to traverse it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think even though the submenu is at the beginning but if it is not visible then we need to traverse further to check for other submenu if present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimized the code to not traverse further if the desired submenu is fetched. Verified the recent change with the test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could, of course, put this in the while condition, but this way is also fine.