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

Problem with accessibilityLayer in RTL #4214

Open
1 task done
arturjackowiak opened this issue Feb 21, 2024 · 7 comments
Open
1 task done

Problem with accessibilityLayer in RTL #4214

arturjackowiak opened this issue Feb 21, 2024 · 7 comments
Labels
accessibility Issues associated with accessibility enhancement Enhancement to a current API

Comments

@arturjackowiak
Copy link

arturjackowiak commented Feb 21, 2024

  • I have searched the issues of this repository and believe that this is not a duplicate.

Reproduction link

https://release--63da8268a0da9970db6992aa.chromatic.com/?path=/docs/api-accessibility--docs

Steps to reproduce

Open chart in RTL

What is expected?

On RTL the left arrow should focus the next point and the right arrow the previous one.

What is actually happening?

On RTL the left arrow focus the previous point and the right arrow the next one.

Environment Info
Recharts v2.12.1
React Any
System Any
Browser Any

Function keyboardEvent is not adapted to RTL

public keyboardEvent(e: any) {
    // The AccessibilityManager relies on the Tooltip component. When tooltips suddenly stop existing,
    // it can cause errors. We use this function to check. We don't want arrow keys to be processed
    // if there are no tooltips, since that will cause unexpected behavior of users.
    if (this.coordinateList.length === 0) {
      return;
    }

    switch (e.key) {
      case 'ArrowRight': {
        if (this.layout !== 'horizontal') {
          return;
        }
        this.activeIndex = Math.min(this.activeIndex   1, this.coordinateList.length - 1);
        this.spoofMouse();
        break;
      }
      case 'ArrowLeft': {
        if (this.layout !== 'horizontal') {
          return;
        }
        this.activeIndex = Math.max(this.activeIndex - 1, 0);
        this.spoofMouse();
        break;
      }
      default: {
        break;
      }
    }
  }
@ckifer
Copy link
Member

ckifer commented Feb 21, 2024

Hi, I'm confused. Why would the right arrow go to the previous and the left arrow go to the next? The code you pasted is how things behave unless I'm missing the point

@ckifer ckifer added the enhancement Enhancement to a current API label Feb 21, 2024
@ckifer
Copy link
Member

ckifer commented Feb 21, 2024

OH, "RTL" stands for "right to left" not "react testing library" 🙃

@ckifer
Copy link
Member

ckifer commented Feb 21, 2024

@julianna-langston FYI

@ckifer ckifer added the accessibility Issues associated with accessibility label Feb 21, 2024
@julianna-langston
Copy link
Contributor

How do you "open chart in rtl"? Is that a browser setting, or are we modifying the Rechart code?

@arturjackowiak
Copy link
Author

How do you "open chart in rtl"? Is that a browser setting, or are we modifying the Rechart code?

In my app, we use

const checkIsRtl = () => document.dir === 'rtl' || document.body.dir === 'rtl'

and then

<XAxis
                            reversed={isRtl}
<YAxis
                            orientation={isRtl ? 'right' : 'left'}

@arturjackowiak
Copy link
Author

Screenshot from my storybook
image

ckifer pushed a commit that referenced this issue Feb 29, 2024
<!--- Provide a general summary of your changes in the Title above -->

## Description

<!--- Describe your changes in detail -->

The AccessibilityManager (which manages the behavior for the
`accessibilityLayer` prop) will take into account if there is an `<XAxis
reversed={true} />` child. If so, the left/right arrow keys will
reverse, so that they align with the chart's visuals.

## Related Issue

<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an
issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps
to reproduce -->
<!--- Please link to the issue here: -->
#4214

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->

## How Has This Been Tested?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
Tested manually, and added test cases 

## Screenshots (if appropriate):

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [x] My code follows the code style of this project.
- [NA] My change requires a change to the documentation.
- [NA] I have updated the documentation accordingly.
- [x] I have added tests to cover my changes.
- [NA] I have added a storybook story or extended an existing story to
show my changes
- [x] All new and existing tests passed.

---------

Co-authored-by: Coltin Kifer <ckifer@amazon.com>
@ckifer
Copy link
Member

ckifer commented Feb 29, 2024

@arturjackowiak added support in PR #4225

This probably won't be backported to 2.x though, it'll be there in 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Issues associated with accessibility enhancement Enhancement to a current API
Projects
None yet
Development

No branches or pull requests

3 participants