Skip to content

Conversation

@jperals
Copy link
Member

@jperals jperals commented Nov 30, 2022

…g keyboard navigation

Description

Issue: AWSUI-19201

Context: mouse hover on line chart allows to highlight not only one single point but also all points (from all series) at a given X coordinate, but this behavior was missing for keyboard users. This PR adds it.

The visual appearance is the same as when the points are highlighted with the mouse, but with the focus indicator around the points and the vertical marker:
Screenshot 2022-11-30 at 09 00 09

How has this been tested?

  • Added unit tests and integration tests
  • Manually tested on local server
  • Validated new aria-label with VoiceOver
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jperals jperals requested a review from a team as a code owner November 30, 2022 08:05
@jperals jperals requested review from Al-Dani and removed request for a team November 30, 2022 08:05
tsconfig.json Outdated
"compilerOptions": {
"lib": ["es5", "es2015.collection", "dom"],
"types": [],
"types": ["jest"],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was necessary for WebStorm to interpret test files correctly. Opened the project with VSCode as well to make sure nothing was broken.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in latest commit after offline conversation, since this would affect non-test files as well.

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Base: 92.40% // Head: 92.35% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (b9d44e7) compared to base (8034c0b).
Patch coverage: 96.80% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #538      +/-   ##
==========================================
- Coverage   92.40%   92.35%   -0.06%     
==========================================
  Files         569      571       +2     
  Lines       16218    16255      +37     
  Branches     4436     4457      +21     
==========================================
+ Hits        14987    15013      +26     
- Misses       1148     1159      +11     
  Partials       83       83              
Impacted Files Coverage Δ
src/app-layout/index.tsx 92.30% <ø> (ø)
src/app-layout/visual-refresh/split-panel.tsx 92.98% <ø> (ø)
...ernal/components/cartesian-chart/bottom-labels.tsx 97.50% <ø> (ø)
...l/components/chart-plot/application-controller.tsx 100.00% <ø> (ø)
src/internal/components/chart-plot/index.tsx 100.00% <ø> (ø)
src/internal/context/split-panel-context.ts 50.00% <ø> (ø)
src/mixed-line-bar-chart/internal.tsx 91.00% <ø> (-3.00%) ⬇️
src/table/use-sticky-header.ts 82.92% <ø> (-2.79%) ⬇️
src/mixed-line-bar-chart/chart-container.tsx 85.78% <80.64%> (-4.72%) ⬇️
src/button-dropdown/internal.tsx 100.00% <100.00%> (ø)
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jperals jperals requested a review from avinashbot November 30, 2022 11:31

function VerticalMarker(
{ height, showPoints = true, showLine = true, points }: VerticalMarkerProps,
{ height, showPoints = true, showLine = true, points, ariaLabel }: VerticalMarkerProps,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it looks like this does nothing, because it's placed on an element with aria-hidden.

Inside chart, it looks like all announcements are being made with a live region inside ChartPlot, so I don't think we don't need to pass an aria label.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, the way this works is that the aria-label is copied over to another element which is the one that is actually focused. But on the other hand, we decided that we don't want this extra information to be announced so I've removed this prop in the latest commit.

Al-Dani
Al-Dani previously approved these changes Dec 6, 2022
@jperals jperals requested a review from avinashbot December 7, 2022 13:44
@jperals jperals merged commit 9404812 into main Dec 7, 2022
@jperals jperals deleted the fix/line-chart-keyboard-navigation branch December 7, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants