On Thu, 9 Nov 2023 04:39:32 GMT, Abhishek Kumar <abhis...@openjdk.org> wrote:
>> The issue exist only for non-editable combobox and the root cause is
>> accessible object is not created due to incorrect index returned from
>> component class which results in no a11y API invoked.
>>
>> Proposed solution is to return the correct accessible child from
>> getAccessibleChild method which is AquaComboBoxButton (arrowButton) instance
>> and that results in invoking the a11y APIs to return the current selected
>> item in combobox.
>>
>> Further when the application comes up first time the accessible name is not
>> set for current displayed item in JCombobox that is handled in
>> AquaComboBoxButton which will take care for the current selected item as
>> well as if user modifies the selection by drop-down list.
>>
>> CI link is posted in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix for custom renderer
Changes requested by aivanov (Reviewer).
src/java.desktop/macosx/classes/com/apple/laf/AquaComboBoxButton.java line 201:
> 199: }
> 200:
> 201: final Component getRendererComponent() {
Suggestion:
private Component getRendererComponent() {
I wanted to use this method in `AquaComboBoxUI` but eventually I added
`updateAccessibleName` instead, thus this method can be made `private`.
src/java.desktop/macosx/classes/com/apple/laf/AquaComboBoxUI.java line 143:
> 141: public void itemStateChanged(final ItemEvent e) {
> 142:
> 143: if (e.getStateChange() != ItemEvent.SELECTED) return;
Suggestion:
public void itemStateChanged(final ItemEvent e) {
if (e.getStateChange() != ItemEvent.SELECTED) return;
The blank line at the start of the method isn't necessary, is it?
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line 1:
> 1: /*
You can use HTML for instructions and builder pattern for configuring
`PassFailJFrame` if you like. I'm just advertising new features.
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
31:
> 29: * @requires (os.family == "mac")
> 30: * @summary Verifies if JComboBox selected item magnifies using
> 31: * screen magnifier a11y tool.
Suggestion:
* @summary Verifies if item selected in JComboBox magnifies using
* screen magnifier a11y tool
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
48:
> 46: private static JFrame frame;
> 47: private static final String INSTRUCTIONS =
> 48: "1) Enable Screen magnifier on the Mac \n\n" +
Suggestion:
"1) Enable Screen magnifier on the Mac\n\n" +
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
52:
> 50: "Select \"Enable Hover Text\"\n\n" +
> 51: "2) Move the mouse over the combobox selected item by
> pressing " +
> 52: "\"command\" button.\n\n" +
Suggestion:
"2) Move the mouse over the combo box and press " +
""Command" button.\n\n" +
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
53:
> 51: "2) Move the mouse over the combobox selected item by
> pressing " +
> 52: "\"command\" button.\n\n" +
> 53: "3) If magnified label is visible, Press Pass else Fail.";
Suggestion:
"3) If magnified label is visible, press Pass else Fail.";
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
68:
> 66:
> 67: String[] fruits = new String[] {"Apple", "Orange",
> 68: "Mango", "Pine Apple", "Banana"};
Suggestion:
"Mango", "Pineapple", "Banana"};
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
71:
> 69: JComboBox<String> comboBox = new JComboBox<String>(fruits);
> 70: JPanel fruitPanel = new JPanel(new GridLayout(1, 2));
> 71: JLabel fruitLabel = new JLabel("Fruits : ", JLabel.CENTER);
Suggestion:
JLabel fruitLabel = new JLabel("Fruits:", JLabel.CENTER);
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
75:
> 73: fruitPanel.add(fruitLabel);
> 74: fruitPanel.add(comboBox);
> 75: comboBox.getAccessibleContext().setAccessibleName("Fruit
> jCombobox");
Suggestion:
comboBox.getAccessibleContext().setAccessibleName("Fruit Combo box");
test/jdk/javax/accessibility/JComboBox/TestJComboBoxScreenMagnifier.java line
81:
> 79: PassFailJFrame.positionTestWindow(frame,
> 80: PassFailJFrame.Position.HORIZONTAL);
> 81: frame.setLocationRelativeTo(null);
Suggestion:
It's not needed, the location of the frame is set explicitly by the above call
to `PassFailJFrame.positionTestWindow`.
test/jdk/javax/swing/JComboBox/6567433/UpdateUIRecursionTest.java line 1:
> 1: /*
You should update the copyright year.
I wonder if it makes sense to resolve warnings about raw classes.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14497#pullrequestreview-1722830503
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388181016
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388182328
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388201084
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388185906
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388191437
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388193414
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388193775
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388190556
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388195249
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388188204
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388197459
PR Review Comment: https://git.openjdk.org/jdk/pull/14497#discussion_r1388204973
