8273986: JEditorPane HTML Demo - Accessibility issues #12707

kumarabhi006 · 2023-02-22T05:24:20Z

JAccessWalker was not able to show component tree correctly if we switch pages for HTML content.

Observation:
The issue observed is that the children are not reported correct for root element when switching of pages happened. The reason behind it is that the getAccessibleChildrenCount API is called on the old accessibleContext object which return the children count as 0. Whenever we switch the page the children count is recalculated based on the root element but the accessibleContext object used to retrieve the child remains unchanged and due to that it return the children count 0.

Solution:

Added a method to update the root element info to find out the children count correctly whenever we switch the pages in JEditorPane.

Checked with the SwingSet2 JEditorPane demo and it reports well the component tree in JAccessWalker. Test case also added to verify the fix.

Steps to verify:

JBS contains the steps to reproduce the scenario.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8273986: JEditorPane HTML Demo - Accessibility issues

Reviewers

Alexander Zuev (@azuev-java - Reviewer) ⚠️ Review applies to 1bf80092
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12707/head:pull/12707
$ git checkout pull/12707

Update a local copy of the PR:
$ git checkout pull/12707
$ git pull https://git.openjdk.org/jdk.git pull/12707/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12707

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12707.diff

bridgekeeper · 2023-02-22T05:26:11Z

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

kumarabhi006 · 2023-02-22T05:27:58Z

@azuev-java Request you to please review this PR.

openjdk · 2023-02-22T05:28:10Z

@kumarabhi006 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 · 2023-02-22T05:32:05Z

Webrevs

azuev-java · 2023-02-22T19:58:08Z

You might to bump up the copyright year in both headers. Other than that looks good.

kumarabhi006 · 2023-02-23T03:18:20Z

You might to bump up the copyright year in both headers. Other than that looks good.

Copyright year updated.

openjdk · 2023-02-23T17:02:13Z

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

8273986: JEditorPane HTML Demo - Accessibility issues

Reviewed-by: kizune, serb

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

ac6af6a: 7176515: ExceptionInInitializerError for an enum with multiple switch statements
dd23ee9: 8303917: Update ISO 639 language codes table
6f67abd: 8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
568dd57: 8304716: Clean up G1Policy::calc_max_old_cset_length()
af0504e: 8304691: Remove jlink --post-process-path option
3859faf: 8231349: Move intrinsic stubs generation to compiler runtime initialization code
f37674a: 8304711: Combine G1 root region abort and wait into a single method
7f9e691: 8304712: Only pass total number of regions into G1Policy::calc_min_old_cset_length
51035a7: 8294137: Review running times of java.math tests
46cca1a: 4842457: (bf spec) Clarify meaning of "(optional operation)"
... and 417 more: https://git.openjdk.org/jdk/compare/29f392e4344e467882c36b5737d432b2d0ee7ebb...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mrserb · 2023-02-24T08:26:43Z

src/java.desktop/share/classes/javax/swing/text/html/HTMLEditorKit.java

@@ -684,9 +685,11 @@ public AccessibleContext getAccessibleContext() {
        if (theEditor == null) {
            return null;
        }
-        if (accessibleContext == null) {
+        if (accessibleContext == null
+            || doc != theEditor.getDocument()) {
            AccessibleHTML a = new AccessibleHTML(theEditor);


The "AccessibleHTML" internally registers the listener on the editor to update the state/doc. Why that notification does not work and it is necessary to recreate the "AccessibleHTML" here?

The "AccessibleHTML" does internally register the listener on the editor to update the state/doc.

But the rootHTMLAccessibleContext is not updated/created for the new root element on doc change and when getAccessibleChildrenCount is called it returns the child count 0, because the API is get called on the old object.

if the "rootHTMLAccessibleContext" depends on the doc then I think it should be updated in that listener at the same moment the doc is replaced. especially taking into account that the "rootElementInfo" is updated already.

In that case I think the rootElementInfo needs to update for rootHTMLAccessibleContext everytime whenever doc change happened.

@mrserb The elementInfo variable is updated for rootHTMLAccessibleContext while setting up the rootElementInfo on doc change.
It does report the children correctly and as of now there is no need to re-create the accessibleContext everytime on doc change.

@mrserb Please review the latest changes.

mrserb · 2023-03-02T03:42:06Z

src/java.desktop/share/classes/javax/swing/text/html/AccessibleHTML.java

@@ -183,6 +183,9 @@ private void buildInfo() {
            Element root = doc.getDefaultRootElement();

            rootElementInfo = new ElementInfo(root);
+            if (rootHTMLAccessibleContext != null) {
+                rootHTMLAccessibleContext.setElementInfo(rootElementInfo);
+            }
            rootElementInfo.validate();


probably we should call validate first, so it will be in the valid state before being leaked to the "setElementInfo" method.

I think this can be verified by the test, you can check the number of children returned by the a11y API is correct when the doc is replaced.

I think this can be verified by the test, you can check the number of children returned by the a11y API is correct when the doc is replaced.

Probably we don't required a test because I think we can verify the number of children by using JAccessWalker a11y tool, so whenever there is a doc change, the component hierarchy should be updated.

probably we should call validate first, so it will be in the valid state before being leaked to the "setElementInfo" method.

Ok, I will check.

probably we should call validate first, so it will be in the valid state before being leaked to the "setElementInfo" method.

Updated.

I think this can be verified by the test, you can check the number of children returned by the a11y API is correct when the doc is replaced.

Added an automated test to verify the fix. Along with test, few html files and support files are also pushed.
Please review the latest changes.

Please fix all that "Whitespace error" in the test.

Please fix all that "Whitespace error" in the test.
Fixed.

kumarabhi006 · 2023-03-09T05:08:07Z

@azuev-java As discussed earlier, the access specifier for setElementInfo method changed from public to protected. It can't be declared as private as method is a part of abstract class.

Please review the latest changes.

azuev-java · 2023-03-10T19:50:03Z

My question would be - is it really necessary to bring the entire HTML set from the demo to test the accessibility? Can we just create a small and simple HTML document with some structure that should be interpreted as a document with nested children? Additionally - i do not understand why do we even need an interactive UI test with the robot and such. Why is it necessary? Why simple creation of JEditorPane and assigning url to it does not work?

kumarabhi006 · 2023-03-13T04:54:08Z

My question would be - is it really necessary to bring the entire HTML set from the demo to test the accessibility?

The idea to get entire HTML set is that we can validate the child count for existing HTML doc (index and title html files). It should be similar to swingset2 demo.

Can we just create a small and simple HTML document with some structure that should be interpreted as a document with nested children?

I think we can, I will try to check this.

Additionally - i do not understand why do we even need an interactive UI test with the robot and such. Why is it necessary?

When html files are loaded in JEditorPane and I try to get the child count, it always returned 1. So, I thought to change the page and get child count using interactive UI and it does return the correct child count.

Why simple creation of JEditorPane and assigning url to it does not work?

I am not sure why it doesn't return the correct child count by simply assigning url to JEditorpane.

kumarabhi006 · 2023-03-13T17:56:10Z

@azuev-java As suggested, I tried to create two simple html files for test. Modified the test case to load the url and get the accessible child count for both html files. The usage of robot as well as interactive UI is removed. Cross-checked with the JAccessWalker tool, the child count is same for the test and in JAccessWalker component tree.

Extra delay added in test case to get the correct child count else the child count always returns 1.

mrserb · 2023-03-13T22:04:33Z

test/jdk/javax/accessibility/JEditorPane/TestEditorPaneAccessibleChildCount.java

+    private static JEditorPane jep;
+    private static AccessibleContext ac;
+    private static URL url;
+    private static JFrame frame;


All these ui-related fields should be accessed on EDT in the test.
url can be made local variable.

Updated. URL is declared as local variable now.

mrserb · 2023-03-16T17:24:13Z

test/jdk/javax/accessibility/JEditorPane/TestEditorPaneAccessibleChildCount.java

+                loadHtmlPage(url);
+            });
+            addDelay(500);
+            int childCount2 = ac.getAccessibleChildrenCount();


note: "ac" initialized on EDT, and accessed here w/o synchronization.

I am facing an error local variables referenced from a lambda expression must be final or effectively final when I tried to get the ChildrenCount and assign it to local variable childCount2 on EDT.

My piece of code looks like this in main method

int childCount2 = 0; SwingUtilities.invokeAndWait(() -> { childCount2 = ac.getAccessibleChildrenCount(); });

@mrserb Accessing the accessibleContext (ac) on EDT now. Moved the local variables (childCount1 and childCount2) to class variable and assigned the values on EDT.

Please review the changes.

@mrserb Please review the latest changes.

mrserb · 2023-03-22T18:35:09Z

test/jdk/javax/accessibility/JEditorPane/TestEditorPaneAccessibleChildCount.java

+                childCount2 = ac.getAccessibleChildrenCount();
+            });
+
+            if ((childCount1 != childCount2) &&


This version will update the childCount1/2 on EDT but then will use it in the main thread w/o synchronization.

So, the if block should also be inside EDT as mentioned below. Right?

SwingUtilities.invokeAndWait(() -> { childCount2 = ac.getAccessibleChildrenCount(); if ((childCount1 != childCount2) && (childCount1 != 0 && childCount2 != 0)) { System.out.println("passed"); } else { System.out.println("Test1 html page accessible children" + " count is: "+ childCount1); System.out.println("Test2 html page accessible children" + " count is: "+ childCount2); throw new RuntimeException("getAccessibleChildrenCount" + " returned wrong child count"); } });

This version will update the childCount1/2 on EDT but then will use it in the main thread w/o synchronization.

Updated the childCount1/2 to be used on EDT in stead of main thread.

kumarabhi006 · 2023-03-24T16:14:36Z

/integrate

openjdk · 2023-03-24T16:16:29Z

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

5727610: 8304353: Add lib-test tier1 testing in GHA
d8ba227: 8304069: ClassFileParser has ad-hoc hashtables
9a8a60f: 8304833: (fc) Remove dead code in sun.nio.ch.FileChannelImpl::implCloseChannel
f96aee7: 8291154: Create a non static nested class without enclosing class throws VerifyError
4ec720d: 8297977: vmTestbase/nsk/stress/except/except012.java fails with unexpected Exception
13dd19a: 8304802: After JDK-8297639 the flag G1UsePreventiveGC needs to be added to the obsoletion table
d61de14: 8303508: Vector.lane() gets wrong value on x86
941a7ac: 8304301: Remove the global option SuperWordMaxVectorSize
ac6af6a: 7176515: ExceptionInInitializerError for an enum with multiple switch statements
dd23ee9: 8303917: Update ISO 639 language codes table
... and 425 more: https://git.openjdk.org/jdk/compare/29f392e4344e467882c36b5737d432b2d0ee7ebb...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-24T16:16:47Z

@kumarabhi006 Pushed as commit 9764948.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JEditorPane HTML Demo fix

2cc92c7

openjdk bot added the rfr Pull request is ready for review label Feb 22, 2023

openjdk bot added the client client-libs-dev@openjdk.org label Feb 22, 2023

Copyright year update

1bf8009

azuev-java approved these changes Feb 23, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 23, 2023

mrserb reviewed Feb 24, 2023

View reviewed changes

Review comment update

1e8b0bd

mrserb reviewed Mar 2, 2023

View reviewed changes

kumarabhi006 added 2 commits March 2, 2023 10:14

Review comment update

0701ee4

Test case and supporting file added

bd4d65c

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 9, 2023

Whitespace error fix and unused image file removed

82672b2

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 9, 2023

Test update and added simple html file for test, removed unused files

e25daed

mrserb reviewed Mar 13, 2023

View reviewed changes

Review comment update

7f89422

mrserb reviewed Mar 16, 2023

View reviewed changes

Access Ui field on EDT

2dec603

mrserb reviewed Mar 22, 2023

View reviewed changes

Review comment update

0d667a0

mrserb approved these changes Mar 23, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 24, 2023

openjdk bot closed this Mar 24, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 24, 2023

8273986: JEditorPane HTML Demo - Accessibility issues #12707

8273986: JEditorPane HTML Demo - Accessibility issues #12707

kumarabhi006 commented Feb 22, 2023 •

edited

bridgekeeper bot commented Feb 22, 2023

kumarabhi006 commented Feb 22, 2023

openjdk bot commented Feb 22, 2023

mlbridge bot commented Feb 22, 2023 •

edited

azuev-java commented Feb 22, 2023

kumarabhi006 commented Feb 23, 2023

openjdk bot commented Feb 23, 2023 •

edited

mrserb Feb 24, 2023

kumarabhi006 Feb 27, 2023

mrserb Feb 27, 2023

kumarabhi006 Feb 28, 2023

kumarabhi006 Feb 28, 2023

kumarabhi006 Mar 1, 2023

mrserb Mar 2, 2023

mrserb Mar 2, 2023

kumarabhi006 Mar 2, 2023

kumarabhi006 Mar 2, 2023

kumarabhi006 Mar 2, 2023

kumarabhi006 Mar 9, 2023

mrserb Mar 9, 2023

kumarabhi006 Mar 9, 2023

kumarabhi006 commented Mar 9, 2023 •

edited

azuev-java commented Mar 10, 2023

kumarabhi006 commented Mar 13, 2023

kumarabhi006 commented Mar 13, 2023 •

edited

mrserb Mar 13, 2023

kumarabhi006 Mar 14, 2023

mrserb Mar 16, 2023

kumarabhi006 Mar 16, 2023

kumarabhi006 Mar 16, 2023

kumarabhi006 Mar 21, 2023

mrserb Mar 22, 2023

kumarabhi006 Mar 22, 2023 •

edited

kumarabhi006 Mar 22, 2023

kumarabhi006 commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

8273986: JEditorPane HTML Demo - Accessibility issues #12707

8273986: JEditorPane HTML Demo - Accessibility issues #12707

Conversation

kumarabhi006 commented Feb 22, 2023 • edited

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 22, 2023

kumarabhi006 commented Feb 22, 2023

openjdk bot commented Feb 22, 2023

mlbridge bot commented Feb 22, 2023 • edited

Webrevs

azuev-java commented Feb 22, 2023

kumarabhi006 commented Feb 23, 2023

openjdk bot commented Feb 23, 2023 • edited

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

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

kumarabhi006 commented Mar 9, 2023 • edited

azuev-java commented Mar 10, 2023

kumarabhi006 commented Mar 13, 2023

kumarabhi006 commented Mar 13, 2023 • edited

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

kumarabhi006 Mar 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarabhi006 commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

kumarabhi006 commented Feb 22, 2023 •

edited

mlbridge bot commented Feb 22, 2023 •

edited

openjdk bot commented Feb 23, 2023 •

edited

kumarabhi006 commented Mar 9, 2023 •

edited

kumarabhi006 commented Mar 13, 2023 •

edited

kumarabhi006 Mar 22, 2023 •

edited