Skip to content
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

8262981: Create implementation for NSAccessibilitySlider protocol #2874

Closed
wants to merge 7 commits into from

Conversation

@pankaj-bansal
Copy link

@pankaj-bansal pankaj-bansal commented Mar 8, 2021

Create implementation of NSAccessibilitySlider protocol https://developer.apple.com/documentation/appkit/nsaccessibilityslider

The implementation has the function performIncrement and performDecrement to increase/decrease the value of slider using the VoiceOver. To implement this functionality, I could think of following two ways. I have chosen the first one here though it is more intrusive.

  1. Make the AccessibleJSlider class implement the AccessibleAction interface. This makes AccessibleJSlider consistent with the AccessibleJSpinner class. It is more clear and the logic to increase/decrease Slider value can be changed easily. But this changes AccessibleJSlider class and will need a CSR.
  2. Get the current Accessible Value from the component and just increment/decrement it in SliderAccessibility.m class itself and then set the current accessible value fro there only. This will not need any changes in AccessibleJSlider class, but this does not look correct way and I have not used this.

The changes can be easily tested by using a JSlider example, like the following example. VO should announce the correct the slider values. To change the slider values, use ctrl+opt+shift+down key to start interacting with the slider, then use ctrl+opt+up/down arrow keys to increment/decrement slider values.

  import javax.swing.JFrame;
  import javax.swing.JSlider;
  
  public class JSliderDemo{
      public static void main(String[] args) throws Exception {
          JFrame jframe = new JFrame("JSlider Demo");
          jframe.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
          jframe.add(new JSlider());
  
          jframe.setLocationRelativeTo(null);
          jframe.pack();
          jframe.setVisible(true);
      }
  }

Progress

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

Issue

  • JDK-8262981: Create implementation for NSAccessibilitySlider protocol

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2874

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 8, 2021

👋 Welcome back pbansal! 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
Copy link

@openjdk openjdk bot commented Mar 8, 2021

@pankaj-bansal The following labels will be automatically applied to this pull request:

  • awt
  • swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@pankaj-bansal pankaj-bansal marked this pull request as ready for review Mar 9, 2021
@openjdk openjdk bot added the rfr label Mar 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 9, 2021

Webrevs

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 10, 2021

If you need a new functionality and CSR I wonder how it currently works on Windows and macOS, or it does not work?

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Mar 10, 2021

If you need a new functionality and CSR I wonder how it currently works on Windows and macOS, or it does not work?

Haven't tested this functionality on Windows, on macOS with current JDK it does not work at all. After entering the JSlider interaction mode VoiceOver does not announce the a11y shortcuts and the attempt to invoke said shortcuts do nothing.

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Mar 10, 2021

If you need a new functionality and CSR I wonder how it currently works on Windows and macOS, or it does not work?

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Mar 10, 2021

If you need a new functionality and CSR I wonder how it currently works on Windows and macOS, or it does not work?

Haven't tested this functionality on Windows, on macOS with current JDK it does not work at all. After entering the JSlider interaction mode VoiceOver does not announce the a11y shortcuts and the attempt to invoke said shortcuts do nothing.

Yes, as Alex pointed out, it does not work on MacOS. The VO only announces the value and user can not modify the slider values using VO shortcut keys.

I saw the windows code and I did not see anything where the current value of a component is being set. I tested this by running JAWS as well. When a JSlider is in focus, JAWS announces the slider values and then says "To increase or decrease, use the arrow keys". The keys are being handled by JAVA only and JAWS is not doing any part in it. To verify this, I commented the code to handle keys events in Java and slider value does not change. So looks like this does not work on Windows as well.

If these changes are too intrusive, I can go with the second approach of getting and setting the current values of slider in performincrement/performdecrement functions in SliderAccessibility.m. I am not sure if this will be correct approach, but it will be less intrusive and will not need a CSR.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 10, 2021

If it does not work on Windows then I suggest checking that the new functionality requested via CSR will help it as well.

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Mar 11, 2021

If these changes are too intrusive, I can go with the second approach of getting and setting the current values of slider in performincrement/performdecrement functions in SliderAccessibility.m. I am not sure if this will be correct approach, but it will be less intrusive and will not need a CSR.

I would suggest to fix it properly by filing CSR for the functional change. The idea of new implementation is not only to make it compatible with the new Apple API, but also to make it cleaner, more consistent and easier to support in the future.

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Mar 11, 2021

If it does not work on Windows then I suggest checking that the new functionality requested via CSR will help it as well.

I could not find any key shortcuts where JAWS will consume the key pressed and will be used to interact with the component like the VO is doing here. This similar functionality is present in JSpinner already and JAWS says similar thing when a Spinner is in focus "To set the value, press the arrow keys or type the value". When user presses the keys, it is consumed by Java and value is incremented/decremented as expected. But the value is not being set by JAWS like VO if used with VO keys. So I am not sure how to verify if the changes will be useful for windows as well. I think if there is a way user can interact with Spinner or Slider, then these changes should be useful and if it is not possible, it will not be useful in both. These changes does make the Slider more consistent with Spinner and easy to maintain and cleaner.
If you happen to have some idea about how can a user interact with Sliders or Spinners using JAWS, please let me know. I will definitely verify if the changes would be helpful for Windows.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 12, 2021

If you happen to have some idea about how can a user interact with Sliders or Spinners using JAWS, please let me know. I will definitely verify if the changes would be helpful for Windows.

How the native applications works under JAWS?

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Mar 14, 2021

If you happen to have some idea about how can a user interact with Sliders or Spinners using JAWS, please let me know. I will definitely verify if the changes would be helpful for Windows.

How the native applications works under JAWS?

I tried running a Windows native settings app like "Display Settings" with JAWS. It has a Slider "Brightness". JAWS says same thing when the Slider was brought in focus like "To increase or decrease, use the arrow keys". When I press the arrow keys, the values change. But I don't know if the value changed because the Windows Native dialog handles the keys and changed the value, or JAWS consumed the event and then changed the value. It looks like the Dialog must have handled the keys as there are no special keys mentioned for JAWS to interact with the component like the VO. I am not sure how I can verify this on Windows native apps.
I did try to run the JSpinner with JAWS and I see that if Java does not handle the keys, the value in Spinner does not change. This means JAWS does not interact with the component and the accessibility functionality was added for MacOS only. Same thing is being done for JSlider.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 17, 2021

It looks like the Dialog must have handled the keys as there are no special keys mentioned for JAWS to interact with the component like the VO

Did you check this list of hotkeys?
https://www.freedomscientific.com/training/jaws/hotkeys
https://freedomscientific.github.io/VFO-standards-support/aria.html
Something like:
RIGHT ARROW and UP ARROW increase the value of the slider.
LEFT ARROW and DOWN ARROW decrease the value of the slider.
HOME and END move to the minimum and maximum values of the slider.
PAGE UP and PAGE DOWN increment or decrement the slider by a given amount.

=======================
My main point is that if we will change some shared code we should care about both supported platforms.

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Mar 30, 2021

It looks like the Dialog must have handled the keys as there are no special keys mentioned for JAWS to interact with the component like the VO

Did you check this list of hotkeys?
https://www.freedomscientific.com/training/jaws/hotkeys
https://freedomscientific.github.io/VFO-standards-support/aria.html
Something like:
RIGHT ARROW and UP ARROW increase the value of the slider.
LEFT ARROW and DOWN ARROW decrease the value of the slider.
HOME and END move to the minimum and maximum values of the slider.
PAGE UP and PAGE DOWN increment or decrement the slider by a given amount.

=======================
My main point is that if we will change some shared code we should care about both supported platforms.

I tried the hot keys with the both native dialog ("Display Settings" with a "Brightness" Slider ) and java applications.
Both Java and native dialog are themselves handling the RIGHT ARROW, UP ARROW, LEFT ARROW, DOWN ARROW, HOME and END buttons for JSlider and user can change slider value using these keys without JAWS also. but they are not handling the PAGE UP and PAGE DOWN button.
PAGE UP and PAGE DOWN button don't work with JAWS either, meaning JAWS is not able to handle the keys and is not able to change component values for both native dialogs and Java application. Also, I am not able to find any component in Swing, where JAWS can change the component values using any hot key. Please correct me if I am wrong in this and JAWS can actually manipulate some component. Also, in the old implementation for MacOS , the VO can not change the Spinner/Slider values and we are adding this functionality now only.
In addition to this, the functionality we are adding in slider can be used with JAWS also if we decide to provide the support for the same later. Currently, I do not see JAWS being able to change any component value in swing, so it will not be able to change Slider value too.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 31, 2021

Ok, please file a similar bug for the windows platform.

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Apr 1, 2021

/csr

@openjdk openjdk bot added the csr label Apr 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 1, 2021

@pankaj-bansal has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@pankaj-bansal please create a CSR request and add link to it in JDK-8262981. This pull request cannot be integrated until the CSR request is approved.

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Apr 1, 2021

Ok, please file a similar bug for the windows platform.

I have created the bug for windows here https://bugs.openjdk.java.net/browse/JDK-8264598.
I have also created the CSR for the current issue.

@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Apr 5, 2021

@mrserb @azuev-java Any other comments on this? Can you please look at the CSR also?

@mrserb
Copy link
Member

@mrserb mrserb commented Apr 7, 2021

I have no additional comments.

@openjdk openjdk bot removed the csr label Apr 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2021

@pankaj-bansal 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:

8262981: Create implementation for NSAccessibilitySlider protocol

Reviewed-by: kizune

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

  • ea5c55a: 8265103: Remove unnecessary inclusion of oopMap.hpp
  • 26186ec: 8039261: [TEST_BUG]: There is not a minimal security level in Java Preferences and the TestApplet.html is blocked.
  • 283d64f: 8262896: [macos_aarch64] Crash in jni_fast_GetLongField
  • 55d5649: 8263157: [macos]: java.library.path is being set incorrectly
  • e80012e: 8264768: JFR: Allow events to be printed to the log
  • 3b576ed: 8265100: (fs) WindowsFileStore.hashCode() should read cached hash code once
  • 8df8512: 8265125: IGV: cannot edit forms with NetBeans GUI builder
  • 9cd5400: 8265138: Simplify DerUtils::checkAlg
  • c797511: 8264940: java/lang/invoke/6998541/Test6998541.java failed "guarantee(ik->is_initialized()) failed: java/lang/Byte$ByteCache must be initialized"
  • 943503e: 8265035: Remove unneeded exception check from refill_ic_stubs()
  • ... and 144 more: https://git.openjdk.java.net/jdk/compare/fdfa1dda08f0283422f78fb315f801025e1cc7d7...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.

@openjdk openjdk bot added the ready label Apr 14, 2021
@pankaj-bansal
Copy link
Author

@pankaj-bansal pankaj-bansal commented Apr 15, 2021

/integrate

@openjdk openjdk bot closed this Apr 15, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2021

@pankaj-bansal Since your change was applied there have been 183 commits pushed to the master branch:

  • abdff79: 8163086: java/awt/Window/TranslucentJAppletTest/TranslucentJAppletTest.java fails
  • b72d99e: 8233564: [TESTBUG] MouseComboBoxTest.java is failing
  • 2b5869a: 8233565: [TESTBUG] NullModalityDialogTest.java fails on MacOS
  • bba16f6: 8264818: G1: Improve liveness check for empty pinned regions after full gc marking
  • 75da1e9: 8264423: G1: Rename full gc attribute table states
  • 125a847: 8264788: Make SequentialSubTasksDone use-once
  • 0793fcb: 8260255: C1: LoopInvariantCodeMotion constructor can leave some fields uninitialized
  • b224b56: 8265225: jdk/jfr/tool/TestConfigure.java fails to cleanup the output files after the testing
  • 7c6e379: 8265120: hs_err improvement: align the output of Virtual space metadata
  • e7cbeba: 8259070: Add jcmd option to dump CDS
  • ... and 173 more: https://git.openjdk.java.net/jdk/compare/fdfa1dda08f0283422f78fb315f801025e1cc7d7...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9d669c9.

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

@pankaj-bansal pankaj-bansal deleted the JDK-8262981 branch Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants