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

Adds accDescription to Gantt, draws tags to svg #2912

Merged
merged 2 commits into from
Apr 12, 2022

Conversation

el-mapache
Copy link
Contributor

@el-mapache el-mapache commented Apr 7, 2022

📑 Summary

This PR adds an accDescription property to gantt charts, and creates a <title> tag from the gantt chart's title property.

📏 Design Decisions

Following the already set pattern for adding accessibility to charts, setup in #2732

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted develop branch

@@ -12,6 +12,7 @@ let includes = [];
let excludes = [];
let links = {};
let title = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, would it make sense to add the default "Gantt chart" text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't end up adding the default here because this is one of the charts that renders its title as a visible element. There probably aren't a ton of gantt charts without titles in the wild, but I didn't want existing ones to suddenly display differently.

This does make me wonder, however, if the title will get read twice by an assistive device, since it is both a visible element and a title tag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - good call! Hmm I'd have to take a look at the markup. If it's in the DOM twice then most likely it will be read twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really a pro at navigating with VoiceOver 😬 , but when loading gantt.html in the demos folder, and navigating to the svg, I hear 'in a gantt diagram ${description text}, image', which I think is what we expect?

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look as well, and you are correct! That is what we want. Since we have aria-labelledby="${title-id} ${description-id}" and role="image" it overrides everything else inside of the SVG. So, even though the text is in the DOM twice, it won't read the second one (which looks like this, right before the closing </svg> tag <text x="632" y="25" class="titleText">A Gantt Diagram</text>)

@el-mapache el-mapache changed the title Adds accDescription, draws tags to svg Adds accDescription to Gantt, draws tags to svg Apr 7, 2022
@gwincr11 gwincr11 mentioned this pull request Apr 7, 2022
9 tasks
@knsv
Copy link
Collaborator

knsv commented Apr 7, 2022

Hello!

Thank you working with this. We love accessibility so this is much appreciated!

However, keeping the security risk in mind, we would suggest adding sanitization to user input. See the link for your reference:

Once this is done, we will be happy to merge it.

@el-mapache
Copy link
Contributor Author

Of course, thanks for taking a look :)

* Adds accDescription to demos/gantt.html
Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

That looks great! Thanks!

@knsv knsv merged commit 266bce4 into mermaid-js:develop Apr 12, 2022
@knsv knsv mentioned this pull request May 6, 2022
3 tasks
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