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

Added accessibility for charts #11801

Open
wants to merge 6 commits into
base: bugfix
Choose a base branch
from

Conversation

littlesvensson
Copy link
Contributor

Fixes #11798

Description

Adding accessibility features for graphs and charts. Basically no visual difference in appearance just added some aria attributes, and tables, with description within the sr-only class

Before
Screenshot 2025-02-12 at 08 50 56

After

Screenshot 2025-02-12 at 09 20 45 Screenshot 2025-02-12 at 09 21 23

tables for complex graphics example
Screenshot 2025-02-12 at 09 22 27

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@github-actions github-actions bot added the ui label Feb 12, 2025
Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

👏 You fixed the issue(s)! Great work.

@littlesvensson littlesvensson marked this pull request as ready for review February 12, 2025 10:39
Copy link

dryrunsecurity bot commented Feb 12, 2025

DryRun Security Summary

The pull request adds accessibility features to templates while introducing potential security risks through XSS vulnerabilities in metrics.js and possible information exposure in metrics data.

Expand for full summary

The PR introduces accessibility improvements across multiple template files, focusing on enhancing screen reader support and providing alternative data representations for charts and graphs.

Security Vulnerabilities:

  1. In dojo/static/dojo/js/metrics.js:
    • XSS risk due to potential HTML tag removal via regex
    • Unsafe DOM manipulation using jQuery's .append()
    • Lack of comprehensive input validation for punchcardData and ticks
  2. Potential information exposure through detailed weekly breakdown of metrics

No other direct security vulnerabilities were identified in the reviewed files.

Code Analysis

We ran 9 analyzers against 6 files and 2 analyzers had findings. 7 analyzers had no findings.

Analyzer Findings
Cross-Site Scripting Analyzer 7 findings
Configured Codepaths Analyzer 9 findings

Overall Riskiness

🔴 Risk threshold exceeded.

We've notified @mtesauro, @grendel513.

View PR in the DryRun Dashboard.

@Maffooch
Copy link
Contributor

@littlesvensson we had some issues with unit tests a few days ago. Please update your branch with the latest version of bugfix and we'll approve shortly after!

@littlesvensson littlesvensson force-pushed the origin/accessibility-for-charts branch from 96a10e9 to e6bcc40 Compare February 15, 2025 09:01
@mtesauro
Copy link
Contributor

Closing and reopening to see if I can make AccessLint happy...

@mtesauro mtesauro closed this Feb 16, 2025
@mtesauro mtesauro reopened this Feb 16, 2025
@mtesauro
Copy link
Contributor

Whelp, sometimes AccessLint gets stuck. 🤷

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

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

Approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants