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

More Accessible Mermaid Charts #2732

Closed
9 tasks done
gwincr11 opened this issue Feb 16, 2022 · 15 comments · Fixed by #3008
Closed
9 tasks done

More Accessible Mermaid Charts #2732

gwincr11 opened this issue Feb 16, 2022 · 15 comments · Fixed by #3008
Labels
Good first issue! Status: Approved Is ready to be worked on Status: Triage Needs to be verified, categorized, etc

Comments

@gwincr11
Copy link
Contributor

gwincr11 commented Feb 16, 2022

👋 it would be nice to work towards more accessible features now that this library is in much wider use. I think a couple of low hanging fruit items would be to include <title> elements in the output svg and <description> so that people creating charts can offer text explanations of the charts.

There is lots of other work too be done in this area, but this seems like a simple place to start.

Charts to be completed:

  • Pie Chart PR
  • Flowchart PR
  • Sequence diagram PR
  • Class Diagram PR
  • State Diagram PR
  • Entity Relationship Diagram PR
  • User Journey PR
  • Gantt PR
  • Requirement Diagram PR

UPDATE 2-22-2022 (Twosday):

Ok I have a pr open with a new accessibility function that can add a title and a description to a chart. I set it up for Pie charts as they already had a title.

I am probably a blocker on this issue since I have been dealing with a bunch of family things and do not want to prevent someone else from picking this up. Feel free to either merge my pr and then add other charts or start work in some other way. If you would like to merge my pr and something needs to be cleaned up I will happily do it. cc @knsv

@github-actions github-actions bot added the Status: Triage Needs to be verified, categorized, etc label Feb 16, 2022
@knsv knsv added Good first issue! Status: Approved Is ready to be worked on labels Feb 16, 2022
@knsv
Copy link
Collaborator

knsv commented Feb 16, 2022

The syntax for this should be the same in all diagram types

@aardrian
Copy link

I want to add some more context to this issue.

Using the following flow chart:

  flowchart TD;
      A[Using a flow<br>chart in GitHub] --> B{Must it be<br>accessible?};
      B -- No --> C[Wrong,<br>try again];
      B -- Yes --> D{Into what format<br>does it render?};
      D -- JPG/PNG/GIF --> E{Does it have<br>an `alt`?};
      E -- No --> V[Wrong,<br>try again];
      E -- Yes, and it<br>is lengthy --> V[Wrong,<br>try again];
      E -- Yes, a<br>brief one --> F[Manually add<br>a plain text<br>description<br>of the chart.];
      D -- SVG --> G{What `role`<br>does it have?};
      G -- `none` --> N[Wrong,<br>try again];
      G -- `table` --> N[Wrong,<br>try again];
      G -- `grid` --> N[Wrong,<br>try again];
      G -- `presentation` --> Y[Manually add<br>a plain text<br>description<br>of the chart.];
      G -- `image` --> H{Does it have<br>an `aria-label`?};
      H -- No --> P[Wrong,<br>try again];
      H -- Yes, and<br>it is lengthy --> P[Wrong,<br>try again];
      H -- Yes, a<br>brief one --> Z[Manually add<br>a plain text<br>description<br>of the chart.];
      G -- `tree` --> I{Does it contain<br>any `treeitem`<br>roles for the<br>text nodes?};
      I -- No --> R[Wrong,<br>try again];
      I -- Yes --> J{Are they contained<br>in / owned by `group`<br>roles beyond the<br>first level?};
      J -- No --> S[Wrong,<br>try again];
      J -- Yes --> K{Have you<br>tested in<br>any screen<br>reader?};
      K -- No --> T[Wrong,<br>try again];
      K -- Yes --> L{Did it accurately<br>convey relationships<br>and structure from<br>the chart?};
      L -- No --> U[Wrong,<br>try again];
      L -- Yes --> M[You should write a<br>CSUN talk about it];

Screen readers announce it as nothing but unordered run-on text. There are no relationships, no structure, nothing to give context to the chart. It lacks necessary roles and properties as well.

Here is the output directly from the speech viewer of JAWS 2021 with Chrome (the line breaks are from the <br> elements in the text):

Unlabeled 0NoYesJPG/PNG/GIFNoYes, and it
is lengthyYes, a
brief oneSVGnone``table``grid``presentation``imageNoYes, and
it is lengthyYes, a
brief onetreeNoYesNoYesNoYesNoYesUsing a flow
chart in GitHubMust it be
accessible?Wrong,
try againInto what format
does it render?Does it have
an alt?Wrong,
try againManually add
a plain text
description
of the chart.What role
does it have?Wrong,
try againManually add
a plain text
description
of the chart.Does it have
an aria-label?Wrong,
try againManually add
a plain text
description
of the chart.Does it contain
any treeitem
roles for the
text nodes?Wrong,
try againAre they contained
in / owned by group
roles beyond the
first level?Wrong,
try againHave you
tested in
any screen
reader?Wrong,
try againDid it accurately
convey relationships
and structure from
the chart?Wrong,
try againYou should write a
CSUN talk about it

Here is output from the speech viewer of NVDA 2021.3 with Firefox (same about the line breaks):

No Yes JPG/PNG/GIF No Yes, and it
is lengthy Yes, a
brief one SVG none table grid presentation image No Yes, and
it is lengthy Yes, a
brief one tree No Yes No Yes No Yes No Yes Using a flow
chart in GitHub Must it be
accessible? Wrong,
try again Into what format
does it render? Does it have
an alt? Wrong,
try again Manually add
a plain text
description
of the chart. What role
does it have? Wrong,
try again Manually add
a plain text
description
of the chart. Does it have
an aria-label? Wrong,
try again Manually add
a plain text
description
of the chart. Does it contain
any treeitem
roles for the
text nodes? Wrong,
try again Are they contained
in / owned by group
roles beyond the
first level? Wrong,
try again Have you
tested in
any screen
reader? Wrong,
try again Did it accurately
convey relationships
and structure from
the chart? Wrong,
try again You should write a
CSUN talk about it

The announcement is a bit different than what is shown here (every ` announces as "grav", for example). As you read the preceding text, I am sure you can understand how that conveys nothing of the meaning of the chart.

Separately, the answer options in dark mode ("Yes", "No", "JPG/PNG/GIF", etc) all fail WCAG contrast checks. I did not check outside of dark mode. The Firefox accessibility inspector and dev tools identify these failures readily.

Finally, this sentence from the GitHub announcement reveals a complete misunderstanding of how screen readers parse content (they work fine with JS, but the raw MarkDown is just as meaningless as the text I pasted):

…clients requesting content with embedded Mermaid in a non-JavaScript environment (such as a screen reader or an API request) will see the original Markdown code.

Some reference material for working on making the SVGs more accessible:

@gwincr11
Copy link
Contributor Author

gwincr11 commented Feb 19, 2022

Ok I started playing with a pr to understand some of the decisions that will need discussion. So far I think that the path of using role=image is the best path forward. To that end a title and a description seems to be the things that can help here.

A few charts already have a title syntax, I think we add the title to all charts most charts would likely benefit from its visual inclusion as well. This can be used as the label. Then we need a description.

Adding a description syntax to the chart is a bit weird from the perspective of a non-screen reader user, in this I mean that I think most people would expect it to be displayed. Given this syntax

pie 
   title My title
   description Some description 

I would personally expect description to be rendered in the graph like every other language feature. The puzzle here is what to call this field. Its seems natural to me to have it be aria-description however the user base of this is so wide that I am willing to bet a large set of the user base will not know what this is. My next thought was to use alt however I think this has the same issue. We could display the description, however I would imagine that users would use it in un-intended ways like a graph footnote that does not actually describe the graph. I am wondering if anyone has any thoughts about what to call this.

@aardrian
Copy link

I assume these properties you are naming are something to be added in the MarkDown configuration syntax. If so, perhaps…

  • accName or something like it (over alt);
  • accDesc or something like it (over aria-description).

The alt attribute is meant for the <img> element only and the flow for coming up with good alternative text may not apply very well in this context. Avoiding that HTML attribute as a name means its easier to disconnect from that process.

The aria-description property does not exist in ARIA. By using the aria- prefix it implies the attribute exists and can lead to confusion when looking through rendered code (worse when a dev tries to drop an aria-description into other code).

The terms accName and accDesc are generally understood by accessibility practitioners and have some similarity to mobile platform property names as well.

So my example might start as:

```mermaid 
  flowchart TD;
  accName: GitHub format decision tree.;
  accDesc: A series of questions walking through establishing first the file
           format, then the accessible name/description, and finally the
           appropriate properties to use for creating an accessible flow chart.;

It might render an SVG that looks like:

<svg xmlns="http://www.w3.org/2000/svg" aria-labelledby="T1" aria-describedby="D1" >
  <title id="T1">GitHub format decision tree.</title>
  <desc id="D1">A series of questions walking through establishing first the file
        format, then the accessible name/description, and finally the appropriate
        properties to use for creating an accessible flow chart.</desc></svg>

Despite all this, since many of the charts that can be generated do not have the relatively simple narrative flow of the one I added above, I still recommend an auto-generated plain text equivalent alongside the chart. Especially when the chart is small, the contrast is poor, or rendering breaks.

For example, the alternative text I wrote for the chart above:

The decision tree: Using a flow chart in GitHub → Must it be accessible? If no: Wrong, try again. If yes → Into what format does it render? If JPG/PNG/GIF: Does it have an alt? If no: Wrong, try again. If Yes, and it is lengthy: Wrong, try again. If yes: Manually add a plain text description of the chart. Back up to Into what format does it render? If SVG → What role does it have? If none: Wrong, try again. If table: Wrong, try again. If grid: Wrong, try again. If presentation: Manually add a plain text description of the chart. If image → Does it have an aria-label? If no: Wrong, try again. If Yes, and it is lengthy: Wrong, try again. If yes: Manually add a plain text description of the chart. Back up to What role does it have? If tree → Does it contain any treeitem roles for the text nodes? If no: Wrong, try again. If yes → Are they contained in / owned by group roles beyond the first level? If no: Wrong, try again. If yes → Have you tested in any screen reader? Have you tested in any screen reader? If no: Wrong, try again. If yes → Did it accurately convey the relationships and structure of the chart? If no: Wrong, try again. If yes → You should write a CSUN talk about it.

As you can see, that is a bit verbose for a plain text description and lacks the nuance and structure you could represent with things like lists.

@gwincr11
Copy link
Contributor Author

gwincr11 commented Feb 23, 2022

Thanks @aardrian

accName or something like it (over alt);

I am tempted to just re-use the title, it is already on some charts and I think that if the element is actually used for something that is used for multiple purposes like the visual title of the chart it is likely to be added more frequently then something that is just for accessibility, I frequently see images without alt text.

accDesc or something like it (over aria-description).

I went with accDescription on my pr, thanks for the suggestion.

Despite all this, since many of the charts that can be generated do not have the relatively simple narrative flow of the one I added above, I still recommend an auto-generated plain text equivalent alongside the chart. Especially when the chart is small, the contrast is poor, or rendering breaks.

I generally agree, it would be good to move this issue first as an enhancement and not get stuck on the creation of such an automated feature, that would be a great follow up enhancement. Maybe open another issue?

@aardrian
Copy link

@gwincr11

I am tempted to just re-use the title, it is already on some charts and I think that if the element is actually used for something that is used for multiple purposes like the visual title of the chart it is likely to be added more frequently then something that is just for accessibility, I frequently see images without alt text.

That's fine for moving this along. Bear in mind that what authors think of as a good title is not always appropriate as an image alternative or accessible name. I say this because down the road you may want to allow authors to add / override / supplement the title to provide something that is a better fit.

I went with accDescription on my pr, thanks for the suggestion.

thumbs up emoji

I generally agree, it would be good to move this issue first as an enhancement and not get stuck on the creation of such an automated feature, that would be a great follow up enhancement. Maybe open another issue?

Agreed it is out of scope for a quick hit for which this issue is asking.

@gwincr11
Copy link
Contributor Author

Thanks @aardrian would you be open to giving my pr for pie charts a review? #2747

@aardrian
Copy link

@gwincr11 I took a quick look, but it has been merged and I really need to see the SVG output to know if all the right bits end up in all the right spots.

That being said, nothing jumped out at me.

@bee-san
Copy link

bee-san commented Feb 25, 2022

Heads up, this ticket is also an A11Y issue:
#2395

Just to tie it all together.

@aardrian
Copy link

@bee-san I left a comment.

It is frustrating that the bug report was opened after another bug report covering the same issue was closed due to inactivity from the project maintainers. The person who closed it also locked it from follow-up comments or re-opening.

Bugs don't stop being bugs because you ignore them. Though it was oddly marked as a feature enhancement instead of a bug.

@gwincr11
Copy link
Contributor Author

Thanks @lindseywild @el-mapache @khiga8 and @therzka! All the charts now have the accesiblity features added as outlined in this issue!

Mario Dance

@aardrian
Copy link

I missed that this was closed.

The Live Editor does not appear to support the new title or accDescription values yet. And the flow chart still does not convey any of connections or content order (it is still disordered run-on text). I tried with the sample flow chart from above.

Are any of those on the radar?

@gwincr11
Copy link
Contributor Author

The Live Editor does not appear to support the new title or accDescription values yet.

The work to add accDescription and title to all charts is now on the development branch and should go out with the next release. I am not sure when or how the live editor gets updated but once they update to the new version it should be there.

And the flow chart still does not convey any of connections or content order (it is still disordered run-on text). I tried with the sample flow chart from above.

This was not in scope for this issue, as it was meant to address title and description elements. I would suggest moving that particular issue out into its own issue, thank you ❤️ for keeping an eye on it.

A number of new issues have been opened as well:
#2904
#2905
#2906

@knsv
Copy link
Collaborator

knsv commented Apr 28, 2022

Hi guys!

This one is trickier then you would think... During regression tests we found that this causes issues and will affect existing diagrams out there.

Take for instance this flowchart:

flowchart TB
   a[I am the title]

This will break due the title keyword appearing in a node text. I suggest we return to the initial suggestion of using accName and accDesc as keywords:

  accName: GitHub format decision tree.;
  accDesc: A series of questions walking through establishing first the file
           format, then the accessible name/description, and finally the
           appropriate properties to use for creating an accessible flow chart.;

We can still keep the same rendering though. I will take a look and adjust the existing work.

@knsv knsv reopened this Apr 28, 2022
@knsv
Copy link
Collaborator

knsv commented Apr 30, 2022

An update... The description will have these two forms:

accTitle: The title
accDesc: The description...

or

accDesc {
 The multiline 
 description
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue! Status: Approved Is ready to be worked on Status: Triage Needs to be verified, categorized, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants