8298382: JavaFX ChartArea Accessibility Reader #1016

azuev-java · 2023-01-30T21:56:45Z

Change the underlying class XYChart to take into account labels for axes. Make label patterns and default axes labels localized.

Progress

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

Issue

JDK-8298382: JavaFX ChartArea Accessibility Reader

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1016

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1016.diff

Change the underlying class XYChart to take into account labels for axes. Make label patterns and default axes labels localized.

kevinrushforth · 2023-01-30T22:11:50Z

/reviewers 2

bridgekeeper · 2023-01-30T22:45:09Z

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

openjdk · 2023-01-30T22:46:51Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2023-01-30T22:50:46Z

Webrevs

kevinrushforth · 2023-01-31T18:48:41Z

Can you provide an evaluation of the bug and a description of the fix?

azuev-java · 2023-01-31T19:51:51Z

Can you provide an evaluation of the bug and a description of the fix?

Done.

kevinrushforth · 2023-02-02T00:22:53Z

Can you provide an evaluation of the bug and a description of the fix?

Done.

Thanks.

Can you enable GitHub actions for your repo? If you click on the "Checks" tab you 'll see a message from Skara with a pointer as to how to do that (the short answer is you go to "Actions" for your personal fork and click the big green button to enable it). Then you can either manually trigger a test run or push a commit (e.g., an empty commit or a merge from master) to trigger a run.

kevinrushforth

My initial testing (on Windows) looks good. I have a general question in addition to the one inline comment. Since you added new i18n resource strings have you verified that it works for other locales to ensure no regression? (it should just speak the English string, since there are no translated strings yet)

As for the implementation, I see you've added a (private) shadow property for the x and y axes and the series, as opposed to making them actual properties with only the getter being exposed publicly. As long as the fields are only ever set via the setter method, I think it should work fine the way you have it, but want to take a second look.

modules/javafx.controls/src/main/java/javafx/scene/chart/XYChart.java

andy-goryachev-oracle · 2023-02-02T17:00:04Z

....controls/src/main/resources/com/sun/javafx/scene/control/skin/resources/controls.properties

 Dialog.confirm.header=Confirmation
+
+### Charts ###
+XYChart.series.accessibleText={0} {1} is {2} {3} is {4}


Do we provide a hint to the translators on how these strings need to be translated, like giving an example of rendered text?
I am sure they will have fun translating "A B is C D is E"

Do we provide a hint to the translators on how these strings need to be translated, like giving an example of rendered text?

And that is why i did not do any "Google translate" translation in this PR - i will raise a new bug to translate the new strings where i will describe their meaning and hints on how to translate. Because even if that will be me who is doing the translation i would like to give a reviewer more information on what and how i translate it.

one way to do it is to describe the expected result in the comment immediately preceding the string. For example:

# "the current execution is complete" Class.runText=The run is run.

Also, for testing purposes, we could consider pseudolocalization
https://en.wikipedia.org/wiki/Pseudolocalization

Better discuss this with Naoto Sato.

Sorry for accidentally editing your comment. I think I restored it correctly.

one way to do it is to describe the expected result in the comment immediately preceding the string.
...

Also, for testing purposes, we could consider pseudolocalization

These are good suggestions that should be captured in the follow-up issue that Alex said he will file.

Localization and testing is out of scope for this PR. Alex did the minimum needed to correct the fact that the string wasn't internationalized at all, which makes sense only because he had to modify the string as part of this PR (even then, I would have been OK with no i18n changes in this PR, leaving them for a separate issue as well).

Better discuss this with Naoto Sato.

Good idea.

Still, I would recommend adding a comment (since now is the time when we know all the context, and in the future we might forget or lose the context), may be something like

# {seriesName} {xAxisName} is {currentX} {yAxisName} is {currentY} XYChart.series.accessibleText={0} {1} is {2} {3} is {4}

Still, I would recommend adding a comment (since now is the time when we know all the context, and in the future we might forget or lose the context), may be something like

Good idea, let me do it for both strings with parameters

sorry to hold up the review - I'll speak to Naoto Sato tomorrow (2/3/23 @ 11:00) regarding the requirements for translation hints, and either approve or let you know what they expect us to do.

OK. Presuming that the comment Alex already left is fine, or just needs a slight format change, then that's good. If it's more than that, it would be better to do it in a follow-up issue.

The comment is good.
The idea of giving translators hints in the form of comments like this was very warmly welcomed by our i18n lead Naoto Sato. So I think we should follow this approach for every new string we add in jfx (and jdk).
The goal is to give the translators more context, by either giving an example of resulting text or by describing the parameters like Alex did in this case (or both).

azuev-java · 2023-02-02T17:25:24Z

Since you added new i18n resource strings have you verified that it works for other locales to ensure no regression?

I did - tested with Japanese VoiceOver on Mac, it correctly constructs the string.

azuev-java · 2023-02-02T21:57:01Z

Can you enable GitHub actions for your repo? If you click on the "Checks" tab you 'll see a message from Skara with a pointer as to how to do that (the short answer is you go to "Actions" for your personal fork and click the big green button to enable it). Then you can either manually trigger a test run or push a commit (e.g., an empty commit or a merge from master) to trigger a run.

Done and done, tests are passed.

kevinrushforth

Looks good, apart from the recommendation to remove the one unused method you added.

modules/javafx.controls/src/main/java/javafx/scene/chart/XYChart.java

andy-goryachev-oracle

very good, thank you.

andy-goryachev-oracle · 2023-02-03T19:21:59Z

....controls/src/main/resources/com/sun/javafx/scene/control/skin/resources/controls.properties

 Dialog.confirm.header=Confirmation
+
+### Charts ###
+XYChart.series.accessibleText={0} {1} is {2} {3} is {4}


The comment is good.
The idea of giving translators hints in the form of comments like this was very warmly welcomed by our i18n lead Naoto Sato. So I think we should follow this approach for every new string we add in jfx (and jdk).
The goal is to give the translators more context, by either giving an example of resulting text or by describing the parameters like Alex did in this case (or both).

openjdk · 2023-02-03T19:24:14Z

@azuev-java 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:

8298382: JavaFX ChartArea Accessibility Reader

Reviewed-by: kcr, angorya

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

e1f7d0c: Merge
8f2fac0: 8300705: Update boot JDK to 19.0.2
39d8747: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
a4bc9d1: 8300013: Node.focusWithin doesn't account for nested focused nodes
67c55e5: 8251862: Wrong position of Popup windows at the intersection of 2 screens
fa4884c: 8301604: Replace Collections.unmodifiableList with List.of
301bbd6: 8299977: Update WebKit to 615.1

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@kevinrushforth, @andy-goryachev-oracle) 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).

azuev-java · 2023-02-03T21:31:03Z

/integrate

openjdk · 2023-02-03T21:32:35Z

@azuev-java
Your change (at version 1358b98) is now ready to be sponsored by a Committer.

andy-goryachev-oracle · 2023-02-03T21:36:02Z

/sponsor

openjdk · 2023-02-03T21:37:21Z

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

e1f7d0c: Merge
8f2fac0: 8300705: Update boot JDK to 19.0.2
39d8747: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
a4bc9d1: 8300013: Node.focusWithin doesn't account for nested focused nodes
67c55e5: 8251862: Wrong position of Popup windows at the intersection of 2 screens
fa4884c: 8301604: Replace Collections.unmodifiableList with List.of
301bbd6: 8299977: Update WebKit to 615.1

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-03T21:37:33Z

@andy-goryachev-oracle @azuev-java Pushed as commit 33f1f62.

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

azuev-java and others added 3 commits January 30, 2023 13:52

8298382: JavaFX ChartArea Accessibility Reader

3206553

Change the underlying class XYChart to take into account labels for axes. Make label patterns and default axes labels localized.

Merge branch 'master' into JDK-8298382

17f767d

Fixed the properties file copyright year

1741c1a

kevinrushforth requested review from arapte and kevinrushforth January 30, 2023 22:11

openjdk bot added the rfr Ready for review label Jan 30, 2023

kevinrushforth reviewed Feb 2, 2023

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/chart/XYChart.java Outdated Show resolved Hide resolved

andy-goryachev-oracle reviewed Feb 2, 2023

View reviewed changes

Removed debug output.

4dbdb2f

azuev-java added 2 commits February 2, 2023 13:12

Added commentary explaining fields ised in parametrized string patterns

4f28dc0

Trigger test jobs

ce66bfb

kevinrushforth reviewed Feb 3, 2023

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/chart/XYChart.java Outdated Show resolved Hide resolved

Removed getSeries() method

1358b98

kevinrushforth approved these changes Feb 3, 2023

View reviewed changes

andy-goryachev-oracle approved these changes Feb 3, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 3, 2023

openjdk bot added the sponsor Ready to sponsor label Feb 3, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 3, 2023

openjdk bot closed this Feb 3, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Feb 3, 2023

8298382: JavaFX ChartArea Accessibility Reader #1016

8298382: JavaFX ChartArea Accessibility Reader #1016

Uh oh!

Conversation

azuev-java commented Jan 30, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

kevinrushforth commented Jan 30, 2023

Uh oh!

bridgekeeper bot commented Jan 30, 2023

Uh oh!

openjdk bot commented Jan 30, 2023

Uh oh!

mlbridge bot commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Jan 31, 2023

Uh oh!

azuev-java commented Jan 31, 2023

Uh oh!

kevinrushforth commented Feb 2, 2023

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Feb 2, 2023 • edited by kevinrushforth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azuev-java commented Feb 2, 2023

Uh oh!

azuev-java commented Feb 2, 2023

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 3, 2023

Uh oh!

azuev-java commented Feb 3, 2023

Uh oh!

openjdk bot commented Feb 3, 2023

Uh oh!

andy-goryachev-oracle commented Feb 3, 2023

Uh oh!

openjdk bot commented Feb 3, 2023

Uh oh!

openjdk bot commented Feb 3, 2023

Uh oh!

Reviewers

Assignees

azuev-java commented Jan 30, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 30, 2023 •

edited

Loading

andy-goryachev-oracle Feb 2, 2023 •

edited by kevinrushforth

Loading