8277497 Last column cell in the JTAble row is read as empty cell #6538

savoptik · 2021-11-24T15:16:54Z

Testing https://bugs.openjdk.java.net/browse/JDK-8271071
Step to reproduce

Run SwingSet2 in JDK 18 ( I used b24 )
Enable Voiceover.
Select JTable demo
Click any row in the table or select the first row . Observe that row is selected & VoiceOver reads the column values or navigate the columns one by one by pressing tab key. When the focus reaches the last column ( Favourite Food ) Observe that column value is read as null. If you hear the same then the bug is reproduced.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8277497: Last column cell in the JTable row is read as empty cell

Reviewers

Anton Tarasov (@forantar - Reviewer)
Alexander Zuev (@azuev-java - Reviewer) ⚠️ Review applies to 2199bd5
pbansal - Reviewer ⚠️ Added manually

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6538/head:pull/6538
$ git checkout pull/6538

Update a local copy of the PR:
$ git checkout pull/6538
$ git pull https://git.openjdk.java.net/jdk pull/6538/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6538

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6538.diff

bridgekeeper · 2021-11-24T15:18:31Z

👋 Welcome back asemenov! 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.

savoptik · 2021-11-24T15:19:19Z

/reviewer credit @forantar @azuev-java @pankaj-bansal

openjdk · 2021-11-24T15:20:45Z

@savoptik
Reviewer ant successfully credited.

Reviewer kizune successfully credited.

Reviewer pbansal successfully credited.

openjdk · 2021-11-24T15:23:48Z

@savoptik 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 · 2021-11-24T15:27:21Z

Webrevs

forantar · 2021-11-24T16:01:04Z

src/java.desktop/share/classes/javax/swing/JLabel.java

@@ -1086,6 +1080,15 @@ public String getAccessibleName() {
            if (name == null) {
                name = super.getAccessibleName();
            }
+


Please remove the extra line.

mrserb · 2021-11-25T03:53:44Z

src/java.desktop/share/classes/javax/swing/JLabel.java

+            if (((name == null) || name.isEmpty()) &&
+                    (JLabel.this.getIcon() != null)) {
+                name = ResourceBundle.getBundle("com.sun.accessibility.internal.resources.accessibility", Locale.getDefault()).getString("image");
+            }


Probably it should somehow ask the icon itself about possible description? I guess the JLabel should work similar to Icon/ImageIcon/AccessibleImageIcon/etc when the text is empty but the icon is set. But I am not sure that the iicons are supported by the a11y in Swing, for example how the reader will cover the simple Icon? Will it say something?

Done. thank you very much.

I few questions to thinking about:

If the label and icon is not accessible then should we say something? Or we should ignore it like we do for any other non-accessible components?

Why the image text is used, don't we need to use the "javax.accessibility.AccessibleRole#ICON" role for such label/icon and allow the reader to say something standard for the icon. Does the voiceover has some default text for the icon/image when the alt text is not set?

Thank you very much. We've made a change. A comparison of the behavior before and after the changes is below.

On MAcOS.

State before our changes .:

in a standalone label with an icon without text, VO says Image;

in a standalone label with an icon with text VO speaks the text of the label;

If a label without text with an icon is in the table VO says "Empty cell".

If you add just a role change:

in a standalone label with an icon without text, VO says Image;

in a standalone label with an icon with text VO speaks the text of the label;

In the table for a label with an icon without text, when navigating horizontally, VO does not say anything;

In the table for a label with an icon without text for vertical navigation, VO says "image cell".

Unacceptable option.

With a complete fix:

in a standalone label with an icon without text, VO says Image;

in a standalone label with an icon with text VO speaks the text of the label;

In the table for a label with an icon without text, for any navigation, VO says "image".

ON Windows.

State before our changes:

in a standalone label with an icon and text, the label text is spoken;

in a standalone label with an icon without text, the label is skipped;

In the table, a label with an icon without text is not spoken, the column heading is spoken.

With full patch:

in a standalone label with an icon and text, the label text is spoken;

in a standalone label with an icon without text the "Image icon" is spoken;

In the table, a label with an icon without text is pronounced as "Image".

forantar · 2021-11-25T13:43:53Z

Looks fine to me.

openjdk · 2021-11-25T13:46:16Z

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

8277497: Last column cell in the JTAble row is read as empty cell

Reviewed-by: ant, kizune, pbansal

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

5045eb5: 8278273: Remove unnecessary exclusion of doclint accessibility checks
587e540: 8210558: serviceability/sa/TestJhsdbJstackLock.java fails to find '^\s+- waiting to lock <0x[0-9a-f]+> (a java.lang.Class ...'
082fdf4: 8172065: javax/swing/JTree/4908142/bug4908142.java The selected index should be "aad"
ab78187: 8277105: Inconsistent handling of missing permitted subclasses
adf3952: 8277372: Add getters for BOT and card table members
7c6f57f: 8275610: C2: Object field load floats above its null check resulting in a segfault
a885aab: 8276125: RunThese24H.java SIGSEGV in JfrThreadGroup::thread_group_id
6994d80: 8278291: compiler/uncommontrap/TraceDeoptimizationNoRealloc.java fails with release VMs after JDK-8154011
286a26c: 8278277: G1: Simplify implementation of G1GCPhaseTimes::record_or_add_time_secs
d14f06a: 8278031: MultiThreadedRefCounter should not use relaxed atomic decrement
... and 196 more: https://git.openjdk.java.net/jdk/compare/8051041eb2b3d70d4cc62b6f2726279d51e44733...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@forantar, @azuev-java) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mrserb · 2021-12-01T22:41:55Z

src/java.desktop/share/classes/javax/swing/JLabel.java

+                        name = ac.getAccessibleName();
+                    }
+                }
+                if ((name == null) || name.isEmpty()) {


I continue our discussion from code line above.

Why for the empty label and non-accessible image we should say "image", and not for example the "empty label" or just ignore it?

I mean that if the icon used by the label does not have an "accessible" icon, or any other accessible component why we will try to mention it to the user?

If non-accessible component should not be mentioned to user then you do not need to add a new code here, we already have the "getAccessibleIcon" method which can be called for example from the "CAccessibility" every time we request name/description/text and it empty while the icon is not null and accessible - this will cover all accesible components not only JLabel.

BTW it could be a SwingSet/App bug that the provided image does not have a description (similar to the common bug in html when the image tag does not have an alt).

Looks like If the change will be done in the "CAccessibility" it will cover the custom components as well.

Thank you. We’ve considered your suggestion and found it reasonable. Indeed, it should be enough here to propagate the icon’s accessible name when it exists. Also, we will change SwingSet2 so that those icons had accessible names.

savoptik · 2021-12-06T17:23:21Z

/integrate

openjdk · 2021-12-06T17:24:27Z

@savoptik
Your change (at version f5f1ac1) is now ready to be sponsored by a Committer.

forantar · 2021-12-06T17:38:37Z

/sponsor

openjdk · 2021-12-06T17:39:44Z

Going to push as commit 70bad89.
Since your change was applied there have been 206 commits pushed to the master branch:

5045eb5: 8278273: Remove unnecessary exclusion of doclint accessibility checks
587e540: 8210558: serviceability/sa/TestJhsdbJstackLock.java fails to find '^\s+- waiting to lock <0x[0-9a-f]+> (a java.lang.Class ...'
082fdf4: 8172065: javax/swing/JTree/4908142/bug4908142.java The selected index should be "aad"
ab78187: 8277105: Inconsistent handling of missing permitted subclasses
adf3952: 8277372: Add getters for BOT and card table members
7c6f57f: 8275610: C2: Object field load floats above its null check resulting in a segfault
a885aab: 8276125: RunThese24H.java SIGSEGV in JfrThreadGroup::thread_group_id
6994d80: 8278291: compiler/uncommontrap/TraceDeoptimizationNoRealloc.java fails with release VMs after JDK-8154011
286a26c: 8278277: G1: Simplify implementation of G1GCPhaseTimes::record_or_add_time_secs
d14f06a: 8278031: MultiThreadedRefCounter should not use relaxed atomic decrement
... and 196 more: https://git.openjdk.java.net/jdk/compare/8051041eb2b3d70d4cc62b6f2726279d51e44733...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-06T17:39:58Z

@forantar @savoptik Pushed as commit 70bad89.

💡 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 Nov 24, 2021

openjdk bot added the client client-libs-dev@openjdk.org label Nov 24, 2021

forantar reviewed Nov 24, 2021

View reviewed changes

mrserb reviewed Nov 25, 2021

View reviewed changes

savoptik force-pushed the JDK-8277497 branch from 8966cff to 09d3fa5 Compare November 25, 2021 08:44

forantar approved these changes Nov 25, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 25, 2021

azuev-java approved these changes Nov 29, 2021

View reviewed changes

8277497: Last column cell in the JTAble row is read as empty cell

22dfb01

savoptik force-pushed the JDK-8277497 branch from 2199bd5 to 22dfb01 Compare December 1, 2021 12:19

mrserb reviewed Dec 1, 2021

View reviewed changes

8277497: Last column cell in the JTAble row is read as empty cell

f5f1ac1

forantar approved these changes Dec 6, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 6, 2021

openjdk bot closed this Dec 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8277497 Last column cell in the JTAble row is read as empty cell #6538

8277497 Last column cell in the JTAble row is read as empty cell #6538

savoptik commented Nov 24, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 24, 2021

savoptik commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

mlbridge bot commented Nov 24, 2021 •

edited

Loading

forantar Nov 24, 2021

savoptik Nov 25, 2021

mrserb Nov 25, 2021

savoptik Nov 25, 2021

mrserb Nov 26, 2021

savoptik Dec 1, 2021

forantar commented Nov 25, 2021

openjdk bot commented Nov 25, 2021 •

edited

Loading

mrserb Dec 1, 2021 •

edited

Loading

mrserb Dec 2, 2021

savoptik Dec 3, 2021

savoptik commented Dec 6, 2021

openjdk bot commented Dec 6, 2021

forantar commented Dec 6, 2021

openjdk bot commented Dec 6, 2021

openjdk bot commented Dec 6, 2021

8277497 Last column cell in the JTAble row is read as empty cell #6538

8277497 Last column cell in the JTAble row is read as empty cell #6538

Conversation

savoptik commented Nov 24, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 24, 2021

savoptik commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

mlbridge bot commented Nov 24, 2021 • edited Loading

Webrevs

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

On MAcOS.

State before our changes .:

If you add just a role change:

With a complete fix:

ON Windows.

State before our changes:

With full patch:

forantar commented Nov 25, 2021

openjdk bot commented Nov 25, 2021 • edited Loading

mrserb Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savoptik commented Dec 6, 2021

openjdk bot commented Dec 6, 2021

forantar commented Dec 6, 2021

openjdk bot commented Dec 6, 2021

openjdk bot commented Dec 6, 2021

savoptik commented Nov 24, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 24, 2021 •

edited

Loading

openjdk bot commented Nov 25, 2021 •

edited

Loading

mrserb Dec 1, 2021 •

edited

Loading